test: Add mockable steady clock, tests for PCP and NATPMP implementations #31022
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/31022. 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. |
DrahtBot
changed the title
laanwj
force-pushed
the
2024-10-pcp-tests
branch
3 times, most recently
from
a2420fa to
41d276d
Compare
laanwj
force-pushed
the
2024-10-pcp-tests
branch
from
41d276d to
85a41cb
Compare
dergoegge
left a comment
dergoegge
left a comment
There was a problem hiding this comment.
Concept ACK
Left some comments on the approach for the steady mock time.
laanwj
force-pushed
the
2024-10-pcp-tests
branch
from
85a41cb to
5016a07
Compare
laanwj
force-pushed
the
2024-10-pcp-tests
branch
from
5016a07 to
258b285
Compare
|
Concept ACK |
src/test/pcp_tests.cpp
Outdated
There was a problem hiding this comment.
This doesn't work, boost apparently does derive from it.
.../src/test/pcp_tests.cpp:260:22: error: cannot derive from ‘final’ base ‘PCPTestingSetup’ in derived type ‘pcp_tests::natpmp_ipv4’
260 | BOOST_AUTO_TEST_CASE(natpmp_ipv4)
| ^~~~~~~~~~~
.../src/test/pcp_tests.cpp:311:22: error: cannot derive from ‘final’ base ‘PCPTestingSetup’ in derived type ‘pcp_tests::pcp_ipv4’
311 | BOOST_AUTO_TEST_CASE(pcp_ipv4)
| ^~~~~~~~
f10c322 to
e4cc03c
Compare
|
Rebased for merge conflict, and updated for @sipa's comment (thanks) to add a constant for the initial mocked clock value. |
In almost all cases (the only exception is `getifaddrs`), we know the size of the data passed into SetSockAddr, so we can check this to be what is expected.
This adds a NodeSteadyClock, which is a steady_clock that can be mocked with millisecond precision.
This will be needed for the test harness.
laanwj
force-pushed
the
2024-10-pcp-tests
branch
from
a5c8da9 to
9cd8528
Compare
|
re-ACK 9cd85281c6cc8d3fb01930e53109fae97a108dd8 |
darosior
left a comment
darosior
left a comment
There was a problem hiding this comment.
Some more nits i noticed when reusing some of this code for the fuzz target. I don't think they warrant retouching, but figured i'd post them anyways.
laanwj
force-pushed
the
2024-10-pcp-tests
branch
from
9cd8528 to
2b4e54d
Compare
|
Updated for @darosior's suggestions. |
Add a mock for a simple scriptable UDP server, and use this to test various code paths (including successful mappings, timeouts and errors) in the PCP and NATPMP implementations.
laanwj
force-pushed
the
2024-10-pcp-tests
branch
from
2b4e54d to
0f716f2
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
The msan CI failure seems unrelated, so I've rerun it and left a comment here: https://github.com/bitcoin/bitcoin/pull/31397/files#r1918744397 |
|
re-ACK 0f716f2 |
Add a NodeSteadyClock, a steady_clock that can be mocked with millisecond precision. Use this in the PCP implementation.
Then add a mock for a simple scriptable UDP server,, which is used to test various code paths (including successful mappings, timeouts and errors) in the PCP and NATPMP implementations.
Includes "net: Add optional length checking to CService::SetSockAddr" from #31014 as a prerequisite.