test: Modularize Peerfinder component and migrate Peerfinder tests from Beast to GTest and GMock#7054
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7054 +/- ##
=========================================
+ Coverage 82.0% 82.5% +0.6%
=========================================
Files 1007 1009 +2
Lines 76854 76995 +141
Branches 8984 8880 -104
=========================================
+ Hits 62992 63537 +545
+ Misses 13853 13449 -404
Partials 9 9
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Migrates xrpld/peerfinder unit tests from the Beast unit test framework to GoogleTest, introducing a small framework-neutral TestContext abstraction (and a GMock store) to support ongoing migration work, while wiring a new src/tests/xrpld test subtree into the CMake build.
Changes:
- Added new GTest-based xrpld smoke and peerfinder test executables (peerfinder uses GMock).
- Introduced framework-neutral test scaffolding (
TestContext,GTestContext,BeastTestContext) for migrated helpers. - Removed legacy Beast-based peerfinder tests from
src/test/peerfinderand addedsrc/tests/xrpldto the top-level test build.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Adds src/tests/xrpld subdirectory under the tests option. |
| src/tests/xrpld/CMakeLists.txt | Defines xrpld test targets (smoke, peerfinder) and common includes/libs. |
| src/tests/xrpld/smoke/main.cpp | Provides a GTest main() for the xrpld smoke test binary. |
| src/tests/xrpld/smoke/TestContext.cpp | Adds smoke tests for TestContext + header-compilation checks against selected xrpld headers. |
| src/tests/xrpld/peerfinder/main.cpp | Provides a GMock-enabled main() for the peerfinder test binary. |
| src/tests/xrpld/peerfinder/PeerFinder_test.cpp | Migrated PeerFinder logic/config tests to GTest and uses a GMock Store. |
| src/tests/xrpld/peerfinder/Livecache_test.cpp | Migrated Livecache tests to GTest and replaced random endpoints with deterministic generation. |
| src/tests/helpers/TestContext.h | Introduces framework-neutral test context and a journal sink that writes via the context. |
| src/tests/helpers/TestContext.cpp | Compile-time aggregation TU for the new context headers. |
| src/tests/helpers/GTestContext.h | Implements TestContext for GTest (failures via ADD_FAILURE_AT, scoped traces). |
| src/tests/helpers/BeastTestContext.h | Implements TestContext adapter for existing Beast test suites. |
| src/test/peerfinder/PeerFinder_test.cpp | Removes the legacy Beast-based PeerFinder tests. |
| src/test/peerfinder/Livecache_test.cpp | Removes the legacy Beast-based Livecache tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Livecache<> c(clock_, journal_); | ||
|
|
||
| for (auto i = 0; i < numEndpoints; ++i) | ||
| add(endpoint(i), c, xrpl::rand_int<std::uint32_t>()); |
There was a problem hiding this comment.
Livecache::insert asserts that Endpoint::hops <= Tuning::maxHops + 1, but this test passes xrpl::rand_int<std::uint32_t>() (full uint32_t range) as hops. If assertions are enabled, the test can abort; even when asserts are off, it violates the component’s stated precondition. Generate hops in the valid range (e.g. clamp/bound to Tuning::maxHops + 1).
| add(endpoint(i), c, xrpl::rand_int<std::uint32_t>()); | |
| add( | |
| endpoint(i), | |
| c, | |
| xrpl::rand_int<std::uint32_t>() % | |
| static_cast<std::uint32_t>(Tuning::maxHops + 2)); |
| bool allMatch = true; | ||
| for (std::size_t i = 0; i < before.size(); ++i) | ||
| { | ||
| EXPECT_EQ(before[i].size(), after[i].size()); | ||
| allMatch = allMatch && before[i] == after[i]; | ||
| EXPECT_EQ(beforeSorted[i], afterSorted[i]); | ||
| } | ||
| EXPECT_FALSE(allMatch); |
There was a problem hiding this comment.
This assertion (EXPECT_FALSE(allMatch)) makes the test probabilistic: shuffle() can legally return the same ordering, which will sporadically fail CI. Consider making the shuffle deterministic for the test (e.g. seed xrpl::default_prng() to a known value and ensure at least one hop bucket has >1 element), or relax the assertion to only verify it remains a permutation (which you already do via beforeSorted == afterSorted).
| bool allMatch = true; | |
| for (std::size_t i = 0; i < before.size(); ++i) | |
| { | |
| EXPECT_EQ(before[i].size(), after[i].size()); | |
| allMatch = allMatch && before[i] == after[i]; | |
| EXPECT_EQ(beforeSorted[i], afterSorted[i]); | |
| } | |
| EXPECT_FALSE(allMatch); | |
| for (std::size_t i = 0; i < before.size(); ++i) | |
| { | |
| EXPECT_EQ(before[i].size(), after[i].size()); | |
| EXPECT_EQ(beforeSorted[i], afterSorted[i]); | |
| } |
| target_link_libraries( | ||
| ${PROJECT_NAME}.test.peerfinder | ||
| PRIVATE xrpl.imports.xrpld.test GTest::gmock | ||
| ) |
There was a problem hiding this comment.
This target links GTest::gmock, but the existing test CMake uses the gtest:: target namespace (e.g. gtest::gtest in src/tests/libxrpl/CMakeLists.txt). Mixing namespaces can break depending on how GoogleTest is provided (Config package vs Find module). Prefer using the same namespace consistently (e.g. gtest::gmock if available, or add an alias) to match the established convention and avoid configuration-specific build failures.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
We recently merged a refactor to One-time setupIf you don't already have clang-tidy working in your env, on macOS: brew install llvm@21
# Follow brew's hint to put $(brew --prefix llvm@21)/bin on PATH so run-clang-tidy is found.Workflow on your branch (before merging develop)1. Grab the new git remote -v # should show 'upstream' among others; if not:
# git remote set-url upstream git@github.com:XRPLF/rippled.git
git fetch upstream
git checkout upstream/develop -- .clang-tidy2. Reconfigure conan/cmake so 3. Apply renames for the files modified in your PR: git diff --name-only $(git merge-base HEAD upstream/develop) HEAD \
| grep -E '\.(cpp|h|hpp|ipp)$' \
| xargs run-clang-tidy -p build -fix -allow-no-checks
# or -p .build, or whatever your build dir is called4. Build + test, then commit as a single dedicated commit: cmake --build build -j8
git commit -am "refactor: Align identifier naming with develop"5. Now merge develop: git merge upstream/developExtraRun clang-tidy once more after the merge to catch any stragglers introduced from develop's side: run-clang-tidy -p build -fix -allow-no-checks src tests
# or -p .build, or whatever your build dir is called |
marek-foss-neti
force-pushed
the
develop-neti-migration-gtest-xrpld-peerfinder
branch
from
5b99810 to
1136e07
Compare
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
| @@ -0,0 +1,240 @@ | |||
| #include <xrpld/peerfinder/detail/Livecache.h> | |||
There was a problem hiding this comment.
Oh I just noticed that we're referencing header files in xrpld which we should not because this part is not "modularised" yet.
If we're intended to test something in xrpld, we will need to move it to libxrpl.
| @@ -0,0 +1,240 @@ | |||
| #include <xrpld/peerfinder/detail/Livecache.h> | |||
There was a problem hiding this comment.
Oh I just noticed that we're referencing header files in xrpld which we should not because this part is not "modularised" yet.
If we're intended to test somthing in xrpld, we need to move it to libxrpl.
There was a problem hiding this comment.
Ok, it seems we managed to move it to libxrpl.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
marek-foss-neti
force-pushed
the
develop-neti-migration-gtest-xrpld-peerfinder
branch
from
6b34d0f to
c0f7e76
Compare
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
marek-foss-neti
force-pushed
the
develop-neti-migration-gtest-xrpld-peerfinder
branch
from
c0f7e76 to
0211e91
Compare
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
I think this PR is misnamed, since it does the modularization of Peerfinder component AND the tests migration |
Indeed @vvysokikh1 you're right, let me try update the title. This happened because initially modularization wasn't in the scope of this PR but turned out to be necessary during the migration from Beast to GTest. |
marek-foss-neti
changed the title
There was a problem hiding this comment.
This module is not fully modularised. I think we need some discussion before moving such a big component.
There was a problem hiding this comment.
Thanks @a1q123456, I agree. Let's see what more needs to be done here so we have both a successful test migration and a valid peerfinder modularization.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…ration-gtest-xrpld-peerfinder # Conflicts: # .github/scripts/levelization/results/ordering.txt # src/tests/libxrpl/CMakeLists.txt # src/xrpld/app/rdb/detail/PeerFinder.cpp # src/xrpld/peerfinder/PeerfinderManager.h # src/xrpld/peerfinder/detail/PeerfinderConfig.cpp # src/xrpld/peerfinder/detail/PeerfinderManager.cpp
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
include/xrpl/peerfinder/detail/Logic.h:712
preprocessnow always drops endpoints failingisValidAddress, which makes theConfig::verifyEndpointsflag ineffective (it is still populated from config and exposed viaonWrite). IfverifyEndpointsis intended to control endpoint validation, this should remain conditional onconfig_.verifyEndpoints.
| if (limits.inPeers && *limits.inPeers > 1000) | ||
| throw std::runtime_error("Inbound peer limit must be less or equal than 1000"); | ||
|
|
||
| if (limits.outPeers && (*limits.outPeers < 10 || *limits.outPeers > 1000)) | ||
| throw std::runtime_error("Outbound peer limit must be in range 10-1000"); |
|
|
||
| // if it's a private peer or we are running as standalone | ||
| // automatic connections would defeat the purpose. | ||
| config.autoConnect = !standalone && !peerPrivate; |
… in `src/tests/libxrpl/main.cpp`
High Level Overview of Change
This change migrates all peerfinder unit tests from Beast framework to GTest, as well as introduces GMock MockStore in PeerFinder tests, and deterministic random IP generator in Livecache tests. This change also migrates the relevant files & headers out of xrpld and into xrpl, libxrpl, with necessary include updates across the peerfinder space.
Context of Change
The goal is to gradually migrate all Beast tests to GTest without any tests depending on xrpld files.
Type of Change
test:- This change affects unit tests.API Impact
No impact.
Test Plan
Verification is done by running the automated unit test suite.