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 CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
src/test/netmsg_tests.cpp
Outdated
| debug_log.EndAndThrowIfNotFound(30s); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(ping) |
There was a problem hiding this comment.
This is inspired by #25515
There was a problem hiding this comment.
Key difference in #25515 is that it is testing net processing in isolation (at least isolated from CConnman), which imo is something we definitely should be doing. We already have e2e tests i.e. the functional tests, so I don't see the benefit to your approach here (besides the c++ tests being faster than the python ones).
There was a problem hiding this comment.
Yes, both testing in isolation and e2e is good to have. The difference between the tests included in this PR and the functional tests is that when the test code runs inside the same process:
- it has greater control and observability - the test can read any global or member variable and can call arbitrary functions
- it can do fuzzing
I think all those tests are complementary, not mutually exclusive.
There was a problem hiding this comment.
The unit tests are what I run locally over and over, as they run so much more quickly. IIRC there was a project idea a few years ago to convert the functional tests in Python to C++. Having better unit test coverage, or shifting coverage from the Python tests to C++ ones, seems beneficial.
src/test/netmsg_tests.cpp
Outdated
| debug_log.EndAndThrowIfNotFound(30s); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(ping) |
There was a problem hiding this comment.
Key difference in #25515 is that it is testing net processing in isolation (at least isolated from CConnman), which imo is something we definitely should be doing. We already have e2e tests i.e. the functional tests, so I don't see the benefit to your approach here (besides the c++ tests being faster than the python ones).
|
|
|
|
|
|
|
@jonatack there you go! |
|
|
…lass This allows reusing them in other mocked implementations. Also move the implementation (method definitions) to `test/util/net.cpp` to make the header `test/util/net.h` easier to follow.
…o it And also allows gradually providing the data to be returned by `Recv()` and sending and receiving net messages (`CNetMessage`).
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.
|
|
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. |
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.