p2p: break out tests into separate crate

[xla]

In order to explore a path forward where testing utilities as well as
the assertions for a single and across crates, this change moves the
secret connection tests into the newly introduced test crate.

[codecov-commenter]

Codecov Report

Merging #898 (30fb847) into master (4e0e61c) will increase coverage by 0.6%.
The diff coverage is 90.4%.

❗ Current head 30fb847 differs from pull request most recent head 0cb4e80. Consider uploading reports for the commit 0cb4e80 to get more accurate results

@@           Coverage Diff            @@
##           master    #898     +/-   ##
========================================
+ Coverage    69.7%   70.4%   +0.6%     
========================================
  Files         200     203      +3     
  Lines       16319   16189    -130     
========================================
+ Hits        11383   11400     +17     
+ Misses       4936    4789    -147     

Impacted Files	Coverage Δ
p2p/src/error.rs	0.0% <ø> (ø)
p2p/src/secret_connection/protocol.rs	65.0% <55.5%> (ø)
p2p/src/secret_connection.rs	84.8% <71.1%> (-5.7%)	⬇️
test/src/pipe.rs	76.8% <83.3%> (ø)
test/src/test/unit/p2p/secret_connection.rs	98.8% <98.8%> (ø)
p2p/src/secret_connection/kdf.rs	100.0% <100.0%> (ø)
p2p/src/secret_connection/nonce.rs	100.0% <100.0%> (ø)
p2p/src/secret_connection/public_key.rs	96.1% <100.0%> (+12.3%)	⬆️
test/src/lib.rs	100.0% <100.0%> (ø)
test/src/test/unit/p2p/secret_connection/nonce.rs	100.0% <100.0%> (ø)
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e0e61c...0cb4e80. Read the comment docs.

[thanethomson]

So I understand that having a separate test crate for cross-crate integration testing makes sense, but I'm not totally clear on the reasoning behind moving (what appear to be, at least) unit tests into a separate crate.

[thanethomson]

These would be considered API-breaking changes, right? If so, please add an entry to .changelog/unreleased/breaking-changes.

[xla]

Add an entry, could you double check that this correctly formatted?

[thanethomson]

Seems good - we can always tweak formatting during the next release.

[thanethomson]

Just so I understand, why are these unit tests broken out into a separate crate? If they're unit tests, shouldn't they stay in the tendermint-p2p crate, alongside the secret connection code?

[xla]

@thanethomson A main source of reconsidering the testing story in Rust comes from this article: https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html

Esp. the later parts where where it goes into detail how code including unit test code is treated during compilation.

[thanethomson]

@thanethomson A main source of reconsidering the testing story in Rust comes from this article: https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html

Esp. the later parts where where it goes into detail how code including unit test code is treated during compilation.

Interesting! So this is effectively an optimization?

It does seem like a good idea to test all publicly exposed APIs from within a single crate. I suppose then that the only tests that would be located outside of the tests crate would be those that test private functionality?

[xla]

Interesting! So this is effectively an optimization?

You can look at it that way for sure, for me that is a means to an end, in this case developer experience. Where we optimise for tight feedback loops by being adamant about how quickly things fail/pass.

It does seem like a good idea to test all publicly exposed APIs from within a single crate. I suppose then that the only tests that would be located outside of the tests crate would be those that test private functionality?

Exactly, I'd be surprised that there are more of a handful of cases for private functionality to be tested.