chore: Unify build & test, add ctest to coverage by Bronek · Pull Request #6013 · XRPLF/rippled

Bronek · 2025-11-07T14:09:55Z

High Level Overview of Change

This PR unifies build and test jobs into a single job, and adds ctest to coverage reporting.

Context of Change

The mechanics of coverage reporting is slightly complex and most of it is encapsulated in the coverage target. The status quo way of preparing coverage reports involves running a single target cmake --build . --target coverage which does three things:

build the rippled binary (via target dependency)
prepare coverage reports:
- run ./rippled -u unit tests
- gather test output and build reports

This makes it awkward to add an additional ctest step between build and coverage reporting steps. The better solution is to split coverage target into separate build, followed by ctest, followed by test generation. Luckily, the coverage target has been designed specifically to support such case; it does not need to build rippled, it's just a dependency. Similarly it allows additional tests to be run before gathering test outputs; in principle we could even strip it from running tests and run them separately instead (a possible future PR ?)

This means we can keep build, ctest and generation of coverage reports as separate steps, as long as the state of build directory is fully (including file timestamps, additional coverage files etc) preserved between the steps.

This means that in order to run ctest for coverage reporting we need to integrate build and test into a single job.

This has the added benefit of halving the number of jobs which need to be scheduled and significantly reduces the number of files copied between the jobs. The on-trigger workflow is literary running hundreds of jobs (matrix of build types, unity and non-unity, two different CPU architectures, different distributions and distribution versions, different compilers) and even though we will cut this number down at some point, having the number of jobs doubled just to keep tests separate from build makes for very unpleasant experience troubleshooting test / build errors.

Type of Change

Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)

.github/workflows/reusable-build-test-config.yml

codecov · 2025-11-07T15:01:59Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.6%. Comparing base (c39f9c5) to head (dbb38c6).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6013     +/-   ##
=========================================
+ Coverage     78.3%   78.6%   +0.3%     
=========================================
  Files          816     817      +1     
  Lines        68887   69005    +118     
  Branches      8304    8233     -71     
=========================================
+ Hits         53942   54244    +302     
+ Misses       14945   14761    -184

see 16 files with indirect coverage changes

🚀 New features to boost your workflow:

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

ximinez

If the ctesttests are being counted in the coverage, why did the coverage not go up?

ximinez · 2025-11-07T16:52:03Z

.github/workflows/reusable-build-test-config.yml

+          ctest \
+            --output-on-failure \
+            -C "${BUILD_TYPE}"


ctest can run the jobs in parallel with -j $count. This is not important now, while the tests only take 1/4 second to run, but as we convert more beast-style tests, it could.

There's one caveat, which is that Windows (/sigh It's always Windows) locks some of the build files while running tests, and parallel jobs can collide. I have an inelegant workaround, which is to use --repeat until-pass:$num_tests. More details.

it would be simpler not to use -j on Windows, IMO. EDIT: or simply use -j 1

With the self-hosted Windows runners we set up, Windows is now one of the fastest to finish, so setting -j 1 would be more than ok for a while.

Bronek · 2025-11-07T17:26:39Z

If the ctesttests are being counted in the coverage, why did the coverage not go up?

It did go up - see https://app.codecov.io/github/XRPLF/rippled/pull/6013/indirect-changes

We simply do not have much under src/libxrpl to test at this time and the majority of affected code is covered by ./rippled -u anyway, so the total effect is only 0.3%

mathbunnyru

A few small changes.

There is one problem in cmake files with this approach:
https://github.com/XRPLF/rippled/actions/runs/19173584738/job/54812410544?pr=6013#step:16:15

1/8 Test #2: xrpl.test.basics.build ........... Passed 0.03 sec

It thinks .build targets are part of testing (which shouldn't be true), and that's why it also doubles number of tests (binaries) it shows as passed (it shows 8 instead of 4)

.github/workflows/reusable-build-test-config.yml

Bronek · 2025-11-07T18:39:33Z

A few small changes.

There is one problem in cmake files with this approach: https://github.com/XRPLF/rippled/actions/runs/19173584738/job/54812410544?pr=6013#step:16:15

1/8 Test #2: xrpl.test.basics.build ........... Passed 0.03 sec

It thinks .build targets are part of testing (which shouldn't be true), and that's why it also doubles number of tests (binaries) it shows as passed (it shows 8 instead of 4)

That's because cmake/XrplAddTest.cmake does this:

  add_test(
    NAME ${target}.build
    COMMAND
      ${CMAKE_COMMAND}
      --build ${CMAKE_BINARY_DIR}
      --config $<CONFIG>
      --target ${target}
  )
  set_tests_properties(${target}.build PROPERTIES
    FIXTURES_SETUP ${target}_fixture
  )

It as if the build targets for the unit tests were not defined as a normal binary targets on purpose. Any idea why ?

~~Anyway, fixing this belongs to a different PR.~~ EDIT removed in this PR

mathbunnyru · 2025-11-07T18:44:41Z

A few small changes.
There is one problem in cmake files with this approach: XRPLF/rippled/actions/runs/19173584738/job/54812410544?pr=6013#step:16:15

1/8 Test #2: xrpl.test.basics.build ........... Passed 0.03 sec

It thinks .build targets are part of testing (which shouldn't be true), and that's why it also doubles number of tests (binaries) it shows as passed (it shows 8 instead of 4)

That's because cmake/XrplAddTest.cmake does this:
  add_test(
    NAME ${target}.build
    COMMAND
      ${CMAKE_COMMAND}
      --build ${CMAKE_BINARY_DIR}
      --config $<CONFIG>
      --target ${target}
  )
  set_tests_properties(${target}.build PROPERTIES
    FIXTURES_SETUP ${target}_fixture
  )
It as if the build targets for the unit tests were not defined as a normal binary targets on purpose. Any idea why ?

Anyway, fixing this belongs to a different PR.

Was introduced by @vvysokikh1 here: https://github.com/XRPLF/rippled/pull/5383/files#diff-8f9256a178a57a974e7c72cd0e428ea351a5bfedc852982d9e7aae592c0daaa9

I don't know why it has been done this way

Bronek · 2025-11-10T10:19:43Z

It as if the build targets for the unit tests were not defined as a normal binary targets on purpose. Any idea why ?
Anyway, fixing this belongs to a different PR.

Was introduced by @vvysokikh1 here: https://github.com/XRPLF/rippled/pull/5383/files#diff-8f9256a178a57a974e7c72cd0e428ea351a5bfedc852982d9e7aae592c0daaa9

I don't know why it has been done this way

I've tried removing these dummy .build targets in dbb38c6 and we will see what happens. It appears to work just fine.

vvysokikh1 · 2025-11-10T10:30:27Z

It as if the build targets for the unit tests were not defined as a normal binary targets on purpose. Any idea why ?
Anyway, fixing this belongs to a different PR.

Was introduced by @vvysokikh1 here: https://github.com/XRPLF/rippled/pull/5383/files#diff-8f9256a178a57a974e7c72cd0e428ea351a5bfedc852982d9e7aae592c0daaa9
I don't know why it has been done this way

I've tried removing these dummy .build targets in dbb38c6 and we will see what happens. It appears to work just fine.

I'm not exactly sure about the change as of now (I don't remember tbh). I initially thought it was done to allow EXCLUDE_FROM_ALL (was removed earlier) binary target, so that they're not built unless you want to run the tests.

vvysokikh1 · 2025-11-10T10:33:15Z

src/tests/libxrpl/CMakeLists.txt

+# Network unit tests are currently not supported on Windows
+if(NOT WIN32)
+  xrpl_add_test(net)
+  target_link_libraries(xrpl.test.net PRIVATE xrpl.imports.test)
+endif()


what exactly is wrong with them on win?

The xrpl.test.net does not complete on Windows, instead it hangs indefinitely i.e. until the runner times out and terminates it. @ximinez created a draft #5944 to try to understand what's going on, but neither of us have time before the 3.0.0 release to actually work on it

Example failing test https://github.com/XRPLF/rippled/actions/runs/18801210581/job/53689528229?pr=5932

As a result of this find, xrpl.test.net were disabled on Windows in #5932 , although this test never worked on Windows.

Questions were addressed by the author.

ximinez · 2025-11-10T23:46:38Z

If the ctesttests are being counted in the coverage, why did the coverage not go up?

It did go up - see app.codecov.io/github/XRPLF/rippled/pull/6013/indirect-changes

We simply do not have much under src/libxrpl to test at this time and the majority of affected code is covered by ./rippled -u anyway, so the total effect is only 0.3%

Oh! Fantastic!

This change unifies the build and test jobs into a single job, and adds `ctest` to coverage reporting. The mechanics of coverage reporting is slightly complex and most of it is encapsulated in the `coverage` target. The status quo way of preparing coverage reports involves running a single target `cmake --build . --target coverage`, which does three things: * Build the `rippled` binary (via target dependency) * Prepare coverage reports: * Run `./rippled -u` unit tests. * Gather test output and build reports. This makes it awkward to add an additional `ctest` step between build and coverage reporting steps. The better solution is to split `coverage` target into separate build, followed by `ctest`, followed by test generation. Luckily, the `coverage` target has been designed specifically to support such case; it does not need to build `rippled`, it's just a dependency. Similarly it allows additional tests to be run before gathering test outputs; in principle we could even strip it from running tests and run them separately instead. This means we can keep build, `ctest` and generation of coverage reports as separate steps, as long as the state of build directory is fully (including file timestamps, additional coverage files etc.) preserved between the steps. This means that in order to run `ctest` for coverage reporting we need to integrate build and test into a single job, which this change does.

Unify build & test, add ctest to coverage

f29d5d5

Bronek requested a review from bthomee November 7, 2025 14:11

bthomee reviewed Nov 7, 2025

View reviewed changes

.github/workflows/reusable-build-test-config.yml Show resolved Hide resolved

Bronek added 4 commits November 7, 2025 14:21

Fix ENABLED_... checks

6f493c9

Fix ctest invocation

59344ac

Linux only: rippled upload and instrumentation check

609a181

Add build type for Windows

6452154

Bronek requested a review from bthomee November 7, 2025 15:02

Disable network unit tests on Windows

aae8491

bthomee approved these changes Nov 7, 2025

View reviewed changes

Fix tests directory for Windows

de445bd

ximinez previously requested changes Nov 7, 2025

View reviewed changes

Bronek requested a review from ximinez November 7, 2025 17:37

mathbunnyru requested changes Nov 7, 2025

View reviewed changes

.github/workflows/reusable-build-test-config.yml Outdated Show resolved Hide resolved

.github/workflows/reusable-build-test-config.yml Outdated Show resolved Hide resolved

.github/workflows/reusable-build-test-config.yml Outdated Show resolved Hide resolved

Added -j for ctest

f8a7aa0

Bronek force-pushed the Bronek/ctest_coverage_reporting branch from 17f5fcb to f8a7aa0 Compare November 7, 2025 17:56

Review feedback

ae99ea3

Bronek requested a review from mathbunnyru November 7, 2025 21:32

Remove dummy .build test targets

dbb38c6

Bronek requested a review from vvysokikh1 November 10, 2025 10:22

vvysokikh1 reviewed Nov 10, 2025

View reviewed changes

Bronek changed the title ~~Unify build & test, add ctest to coverage~~ chore: Unify build & test, add ctest to coverage Nov 10, 2025

mathbunnyru approved these changes Nov 10, 2025

View reviewed changes

Bronek added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Nov 10, 2025

bthomee merged commit 8d2dff2 into develop Nov 10, 2025
25 checks passed

bthomee deleted the Bronek/ctest_coverage_reporting branch November 10, 2025 15:32

Bronek mentioned this pull request Nov 11, 2025

Release 3.0.0-rc1 #5943

Merged

1 task

Conversation

Bronek commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

Uh oh!

Uh oh!

codecov bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ximinez left a comment

Choose a reason for hiding this comment

Uh oh!

ximinez Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Bronek Nov 7, 2025 • 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!

bthomee Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Bronek commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathbunnyru left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Bronek commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathbunnyru commented Nov 7, 2025

Uh oh!

Bronek commented Nov 10, 2025

Uh oh!

vvysokikh1 commented Nov 10, 2025

Uh oh!

vvysokikh1 Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Bronek Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Bronek Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Bronek Nov 10, 2025 • 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!

Uh oh!

ximinez commented Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Bronek commented Nov 7, 2025 •

edited

Loading

codecov bot commented Nov 7, 2025 •

edited

Loading

Bronek Nov 7, 2025 •

edited

Loading

Bronek commented Nov 7, 2025 •

edited

Loading

mathbunnyru left a comment •

edited

Loading

Bronek commented Nov 7, 2025 •

edited

Loading

Bronek Nov 10, 2025 •

edited

Loading