test: Migrate resource, shamap Beast tests to GTest by marek-foss-neti · Pull Request #7133 · XRPLF/rippled

marek-foss-neti · 2026-05-13T10:08:03Z

High Level Overview of Change

This change migrates all resource and shamap unit tests from Beast framework to GTest, including the use of the common TestSink from helpers.

Context of Change

The goal is to gradually migrate all Beast tests to GTest.

Type of Change

test: - This change only affects unit tests.

API Impact

No impact.

Test Plan

Verification is done by running the automated unit test suite.

codecov · 2026-05-13T10:57:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.9%. Comparing base (b6a1ad5) to head (63ec925).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #7133     +/-   ##
=========================================
- Coverage     82.0%   81.9%   -0.0%     
=========================================
  Files         1007    1007             
  Lines        76854   76854             
  Branches      8984    8983      -1     
=========================================
- Hits         62992   62980     -12     
- Misses       13853   13865     +12     
  Partials         9       9

see 9 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2026-05-15T16:02:42Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

marek-foss-neti · 2026-05-16T08:02:09Z

-        BEAST_EXPECT(delta.begin()->first == kH1);
-        BEAST_EXPECT(delta.begin()->second.first == nullptr);
-        BEAST_EXPECT(delta.begin()->second.second->key() == kH1);
+        ASSERT_TRUE(sMap.compare(*map2, delta, 100));


I'd like to know your opinion @a1q123456 on using assertions (ASSERTs) in tests, like for example in this case if delta compare fails then the ASSERT guards that consecutive EXPECTs would not run as they might give misleading results.

In GTests, I see that protocol_autogen uses ASSERTs extensively, but they are also used in basics/mulDiv and json. Do you have a preference of EXPECTs over ASSERTs?

This also relates to that task we talked about, of improving asserts exception handling.

In my opinion, if the rest of the code needs a condition to be true to make sense (e.g. file.is_opened(); file.read_byte() == 1), then we should use ASSERT_*

Ok, then it is done this way.

github-actions · 2026-05-18T10:55:49Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

…ce-shamap

a1q123456 · 2026-05-21T11:05:56Z


    void
-    run() override
+    run(bool backed)


Let's get rid of all run functions. If it requires a parameter like this, we can use testing::WithParamInterface<T> and a parameter generator. See: https://google.github.io/googletest/reference/testing.html#WithParamInterface

Done, and I tried to make it readable as imho the run functions were more readable.

Why do you think this is more readable? I don't think I'm able to tell what this method tests from the function name. Also, we're putting test code in a Fixture.

I agree the fixtures are pointless here. Anyway, the latest changes use TEST_P with a predefined constexpr to make it easier to read and see with the backed/unbacked param. Please check if it looks good to you.

…ce-shamap

a1q123456

lgtm

…ce-shamap

github-actions · 2026-06-05T20:05:33Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

…ration-gtest-resource-shamap # Conflicts: # src/tests/libxrpl/CMakeLists.txt

github-actions · 2026-06-08T12:21:12Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

…ce-shamap

github-actions · 2026-06-09T15:29:07Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

…ration-gtest-resource-shamap # Conflicts: # .github/scripts/levelization/results/ordering.txt # src/tests/libxrpl/shamap/common.h

github-actions · 2026-06-10T12:31:42Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

…ce-shamap

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

+
+    do
+    {
+        f.clock().advance(std::chrono::seconds(1));


+
+    sMap.dump();
+    {
+        constexpr std::array kEYS{


…ce-shamap

godexsoft

Leaving some improvement ideas. Overall it looks good as a migration 👍

godexsoft · 2026-06-15T09:02:40Z

+#include <cstdint>
+#include <string>
+
+namespace xrpl::Resource {


Same question about renaming CamelCase namespaces if that's not too much work. If it is then we can follow up with clang-tidy PR for all of them at once later on.

godexsoft · 2026-06-15T09:04:12Z

+        Consumer c(logic.newInboundEndpoint(addr));
+
+        // Create load until we get a warning
+        int n = 10000;


We try to use auto more. These could definitely be auto and get the exact same types for free.

godexsoft · 2026-06-15T09:05:41Z

+    bool readmitted = false;
+    {
+        using namespace std::chrono_literals;
+        // Give Consumer time to become readmitted.  Should never


These comments could probably fit one line now? Or split each sentence per line

godexsoft · 2026-06-15T09:07:00Z

+        Charge const fee(1000);
+        JLOG(j_.info()) << "Charging " << c.toString() << " " << fee << " per second";
+        c.charge(fee);
+        for (int i = 0; i < 128; ++i)


Let's try to use std::size_t when we do loops as a rule of thumb.

godexsoft · 2026-06-15T09:09:07Z

+
+namespace xrpl::tests {
+
+#ifndef __INTELLISENSE__


I wonder why this is needed? Is it some old hack required long ago that got migrated over or is this still an issue today?

godexsoft · 2026-06-15T09:12:55Z

+    {
+        constexpr std::array kEYS{
+            uint256(
+                "b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e"


Oh yikes. Do you think we can somehow make them fit on one line each? Possibly by specifying std::array above or something? not a huge deal but could be better 🥇

godexsoft · 2026-06-15T09:13:29Z

+                "5a772c6ca8"),
+            uint256(
+                "292891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e"
+                "5a772c6ca8")};


We also usually add a trailing comma to init lists.

godexsoft · 2026-06-15T09:15:10Z

+            map.invariants();
+        }
+
+        int h = 7;


Would be good to know what this magic number is.. or compute it from map perhaps.

godexsoft · 2026-06-15T09:16:49Z

+    uint256 rootHash;
+    std::vector<Blob> goodPath;
+
+    for (unsigned char c = 1; c < 100; ++c)


Can we extract 100 magic number and use it here and to compute 99 and 1 below?

godexsoft · 2026-06-15T09:17:49Z

+    for (int i = 0; i < items; ++i)
+    {
+        source.addItem(SHAMapNodeType::TnAccountState, makeRandomAS());
+        if (i % 100 == 0)


More magic numbers here and below that could be extracted to make things clearer.

…ce-shamap

marek-foss-neti marked this pull request as ready for review May 13, 2026 10:32

marek-foss-neti requested a review from a1q123456 May 13, 2026 13:35

a1q123456 requested changes May 14, 2026

View reviewed changes

Comment thread src/tests/libxrpl/shamap/SHAMap.cpp Outdated

Comment thread src/tests/libxrpl/shamap/FetchPack.cpp Outdated

Comment thread src/tests/libxrpl/shamap/FetchPack.cpp Outdated

marek-foss-neti commented May 15, 2026

View reviewed changes

Comment thread src/tests/libxrpl/shamap/SHAMap.cpp Outdated

marek-foss-neti requested a review from a1q123456 May 15, 2026 12:09

github-actions Bot added the PR: has conflicts label May 15, 2026

marek-foss-neti commented May 16, 2026

View reviewed changes

marek-foss-neti added 7 commits May 18, 2026 12:40

test: Copy resource, shamap Beast tests for GTest migration

f8bbc3e

test: Migrate resource, shamap Beast tests to GTest

95b646d

fix: Rename GTest test filenames, delete migrated Beast test files

9eef518

fix: Run levelization

fa0a440

fix: Requested changes

4162569

fix: Use fixture instead generic run

2f70628

fix: Better use of TEST_F

f6c22cb

marek-foss-neti force-pushed the develop-neti-migration-gtest-resource-shamap branch from 65facf7 to f6c22cb Compare May 18, 2026 10:55

github-actions Bot removed the PR: has conflicts label May 18, 2026

marek-foss-neti added 2 commits May 18, 2026 13:06

fix: Update contstants case

6dd0d99

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

dde09d5

…ce-shamap

a1q123456 requested changes May 21, 2026

View reviewed changes

marek-foss-neti added 4 commits May 21, 2026 13:49

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

1ac2dab

…ce-shamap

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

ba0c6d9

…ce-shamap

fix: Use TEST_P over run functions

362f1fb

fix: Remove useless helpers after commented code removal

7a71eae

marek-foss-neti requested a review from a1q123456 May 25, 2026 18:17

marek-foss-neti commented May 25, 2026

View reviewed changes

Comment thread src/tests/libxrpl/shamap/SHAMap.cpp

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

99bb339

…ce-shamap

a1q123456 approved these changes May 27, 2026

View reviewed changes

marek-foss-neti added 4 commits June 2, 2026 11:49

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

d00c72c

…ce-shamap

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

f610132

…ce-shamap

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

3e43bf1

…ce-shamap

fix: use maybe_unused instead of void

5607bc4

github-actions Bot added the PR: has conflicts label Jun 5, 2026

Merge remote-tracking branch 'upstream/develop' into develop-neti-mig…

6263049

…ration-gtest-resource-shamap # Conflicts: # src/tests/libxrpl/CMakeLists.txt

github-actions Bot removed the PR: has conflicts label Jun 8, 2026

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

cc5597e

…ce-shamap

marek-foss-neti requested a review from godexsoft June 9, 2026 14:52

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

e45c554

…ce-shamap

github-actions Bot added the PR: has conflicts label Jun 9, 2026

Merge remote-tracking branch 'upstream/develop' into develop-neti-mig…

fda33a9

…ration-gtest-resource-shamap # Conflicts: # .github/scripts/levelization/results/ordering.txt # src/tests/libxrpl/shamap/common.h

github-actions Bot removed the PR: has conflicts label Jun 10, 2026

marek-foss-neti added 3 commits June 10, 2026 15:01

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

1848f42

…ce-shamap

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

a8f95fd

…ce-shamap

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

ee4dd09

…ce-shamap

marek-foss-neti requested a review from TimothyBanks June 10, 2026 16:10

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

ded5a54

…ce-shamap

bthomee requested a review from Copilot June 11, 2026 15:49

Copilot started reviewing on behalf of bthomee June 11, 2026 15:49 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

Comment thread src/tests/libxrpl/shamap/SHAMapSync.cpp

do

{

f.clock().advance(std::chrono::seconds(1));

Comment thread src/tests/libxrpl/shamap/SHAMap.cpp

sMap.dump();

{

constexpr std::array kEYS{

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

7901151

…ce-shamap

godexsoft requested changes Jun 15, 2026

View reviewed changes

marek-foss-neti added 3 commits June 16, 2026 14:36

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

333ea1f

…ce-shamap

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

8090714

…ce-shamap

Merge branch 'XRPLF:develop' into develop-neti-migration-gtest-resour…

63ec925

…ce-shamap

Conversation

marek-foss-neti commented May 13, 2026

High Level Overview of Change

Context of Change

Type of Change

API Impact

Test Plan

Uh oh!

codecov Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a1q123456 May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

a1q123456 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 5, 2026

Uh oh!

github-actions Bot commented Jun 8, 2026

Uh oh!

github-actions Bot commented Jun 9, 2026

Uh oh!

github-actions Bot commented Jun 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codecov Bot commented May 13, 2026 •

edited

Loading

a1q123456 May 26, 2026 •

edited

Loading