test: add end-to-end tests for CConnman and PeerManager #26812
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26812. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
|
|
|
|
|
|
|
|
|
There was a problem hiding this comment.
Light ACK 752fbab per git range-diff 489e5aa 612ba17 752fbab since my previous ACK at #26812 (comment) and a quick re-read of the full changes rebased to current master. I like that the changes here are essentially limited to tests only.
|
The pull descriptions claims to add a fuzz test, but I can't see anything related to fuzzing |
I removed the fuzz test due to #26812 (comment) and forgot to update the PR description. Updated now, thanks! |
|
I still wonder how useful this is overall. I understand that it is adding a bunch of test-framework code to mimic the functional tests at a lower level and offers a way to write tests this way in C++. However, developers may still prefer to write the tests in Python going forward, because they are used to it, and because it is more flexible, albeit a bit more expensive, because full processes need to be spun up each time. If this is merged, it will add a few redundant test cases (redundant in the sense of not increasing line coverage over the existing one), but may then end up otherwise unused/stale. Conceptually I like the changes, but it reminds me of my attempt to "port" the functional test to C++, starting by providing functions for each RPC (fa8685d#diff-663b6e7cfa474d453c20efa49be6e3e9f5066f7fed5586e98da98f2b3dea5653R24), but that never took off either. Often when it comes to testing P2P behavior, you'd want to spin up several peers/nodes anyway, and the functional tests have the full framework for that, so going forward developers will likely still use that for testing. |
I agree. This PR does not intend to replace the functional testing framework, but complement it. For example, from C++ one has greater control and observability - the test can read any global or member variable and can call arbitrary functions (repeating #26812 (comment)). Also, when used from within C++ one does not have to re-implement the transport code in Python. From C++ we can use the existent code to parse or craft the socket data. Wrt "how useful is this": the first part of this PR (also isolated in #30205) is already used in Sv2Transport tests. |
For these reasons, along with the speed of running the tests and the simplicity and robustness of using the same language for tests and code, I prefer to write test code in C++ when I can, and run the C++ tests much more often locally than the functional tests. I recall there were initiatives to move functional test code to C++ as @maflcko suggests, and would be supportive of these. |
I followed the links but only found a closed pull requests: #30332 (comment) I think it is easy to say that something will be used in the future, but I think it would be easier if the framework commits were added with non-redundant tests at the same time. |
…nd/or CNetMessages b448b01 test: add a mocked Sock that allows inspecting what has been Send() to it (Vasil Dimov) f186414 test: put the generic parts from StaticContentsSock into a separate class (Vasil Dimov) 4b58d55 test: move the implementation of StaticContentsSock to .cpp (Vasil Dimov) Pull request description: Put the generic parts from `StaticContentsSock` into a separate class `ZeroSock` so that they can be reused in other mocked `Sock` implementations. Add a new `DynSock` whose `Recv()` and `Send()` methods can be controlled after the object is created. To achieve that, the caller/creator of `DynSock` provides to its constructor two pipes (FIFOs) - recv-pipe and send-pipe. Whatever data is written to recv-pipe is later received by `DynSock::Recv()` method and whatever data is written to the socket using `DynSock::Send()` can later be found in the send-pipe. For convenience there are also two methods to send and receive `CNetMessage`s. --- This is used in #26812 (first two commits from that PR). Extracting as a separate PR suggested here: #30043 (comment). ACKs for top commit: Sjors: re-ACK b448b01 jonatack: re-ACK b448b01 pinheadmz: ACK b448b01 Tree-SHA512: 4a36f038192ec4ef63366cbe1a38ae70e7e015630c9f7c44926b756b20ab8c08138acae41801f23b30f6629c7059c1f81e001806e86584ff1bf1fa5b44d9caec
|
This PR is now smaller because part of it was merged via #30205, thanks! |
Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in "exception during the exception". This would terminate the program without any messages. Instead print the message to the standard error output and call `std::abort()`.
Extend `DebugLogHelper::~DebugLogHelper()` to be able to optionally wait for messages, possibly logged from another thread, to arrive before performing the final check.
…anager Add tests that use a mocked socket to similate network IO and cover the full `CConnman`, `PeerManager` and the interaction between them.
|
|
Add unit tests that write data to a mocked socket and inspect what CConnman/PeerManager have written back to the socket, or check the internal state to verify that the behavior is as expected.
This is now possible, after most of #21878 has been merged - we don't do any syscalls (e.g.
connect(),recv()) from the high level code and using a mocked socket allows testing the entire networking stack without opening actual network connections.