CMake: Add dynamic test discovery #33483

purpleKarrot · 2025-09-26T17:21:02Z

Instead of parsing the test names from the source code at configure time, query the list of tests from the test executables at testing time.

This should be upstreamed to CMake: https://gitlab.kitware.com/cmake/cmake/-/issues/26920

Left to do: The test_bitcoin-qt executable should be fixed to support the -function option documented here: https://doc.qt.io/qt-6/qtest-overview.html#testing-options so that its test cases can be discovered as well.

DrahtBot · 2025-09-26T17:21:08Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33483.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	janb84, hebasto, willcl-ark, maflcko
Approach ACK	BrandonOdiwuor

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33929 (test: Remove system_tests/run_command runtime dependencies by hebasto)
#33142 (test: Run bench sanity checks in parallel with functional tests by maflcko)

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.

maflcko · 2025-09-26T19:09:13Z

Seems nice, but in the great picture there is still room for improvement:

This doesn't address the issue of running cross builds on the target (

bitcoin/.github/workflows/ci.yml

Line 388 in 200150b

# Can't use ctest here like other jobs as we don't have a CMake build tree.

)
The functional tests aren't integrated, so they are run sequentially after the unit tests

I am not going to block this pull, because it is a clear improvement over master, and the other issues should be fixed in separate pulls. However, I wanted to bring it up, so that it is clear that more work will be needed if we don't want to end up in a local minimum.

janb84 · 2025-09-29T09:51:06Z

Can you explain the difference in ctest tests-cases? Is the script double discovering tests ?

Master : 150
This PR: 368

See also:
https://github.com/bitcoin/bitcoin/actions/runs/18044688187/job/51352444037?pr=33483#step:8:4795
https://github.com/bitcoin/bitcoin/actions/runs/17995985600/job/51235033214?pr=33443#step:8:4357

maflcko · 2025-09-29T09:56:06Z

Can you explain the difference in ctest tests-cases? Is the script double discovering tests ?

Previously there was a single bench sanity check, now there is one bench sanity check for each bench target. (This is the goal of this pull request, but maybe it isn't obvious from the description)

janb84

Approach ACK 6318195b8ecea6d4ced57c8aac97e1ca713d9822

This PR changes how (en when) we do test discovery. From configure time -> Test time and by use of a CMake module.

NIT: Please extend the PR description of the intended changed behaviour of bench sanity check tests. From single bench sanity check to one bench sanity check for each bench target.

cmake/module/DiscoverTests.cmake

+# Distributed under the MIT software license, see the accompanying
+# file COPYING or https://opensource.org/license/mit/.
+
+# https://gitlab.kitware.com/cmake/cmake/-/issues/26920


hebasto

Instead of parsing the test names from the source code at configure time, query the list of tests from the test executables at testing time.

It might also be helpful to explain why this approach is an improvement.

The current implementation does not handle skipped tests correctly:

on the master branch:

$ ctest --test-dir build -j 20
<snip>
73/148 Test  #88: script_assets_tests ..................***Skipped   0.13 sec
<snip>

100% tests passed, 0 tests failed out of 148

Total Test time (real) =  37.05 sec

The following tests did not run:
	 88 - script_assets_tests (Skipped)

this PR @ 6318195b8ecea6d4ced57c8aac97e1ca713d9822:

$ ctest --test-dir build -j 20
<snip>
73/148 Test  #88: bitcoin.test.script_assets_tests ..................   Passed    0.14 sec
<snip>

100% tests passed, 0 tests failed out of 148

Label Time Summary:
test    =  96.99 sec*proc (142 tests)

Total Test time (real) =  38.24 sec

hebasto · 2025-09-30T14:49:49Z

cmake/module/DiscoverTests.cmake

+  set(multiValueArgs DISCOVERY_ARGS PROPERTIES)
+  cmake_parse_arguments(PARSE_ARGV 1 arg "" "${oneValueArgs}" "${multiValueArgs}")
+
+  get_property(has_counter TARGET ${target} PROPERTY CTEST_DISCOVERED_TEST_COUNTER SET)


The property name started with CTEST_ seems to be reserved, no?

janb84

Concept ACK 2bedad4

Thanks for incorporating my suggestion (and those form the other reviewers) !

Found one typo in the cmake file

janb84 · 2025-10-17T10:58:44Z

cmake/module/DiscoverTests.cmake

+  set(properies_content)
+  list(LENGTH arg_PROPERTIES properties_len)
+  if(properties_len GREATER "0")
+    set(properies_content "      set_tests_properties(\"\${test_name}\" PROPERTIES\n")
+    math(EXPR num_properties "${properties_len} / 2")
+    foreach(i RANGE 0 ${num_properties} 2)
+      math(EXPR value_index "${i} + 1")
+      list(GET arg_PROPERTIES ${i} name)
+      list(GET arg_PROPERTIES ${value_index} value)
+      string(APPEND properies_content "        \"${name}\" \"${value}\"\n")
+    endforeach()
+    string(APPEND properies_content "      )\n")


NIT small typo in variable :
properies_content -> properties_content

Suggested change

set(properies_content)

list(LENGTH arg_PROPERTIES properties_len)

if(properties_len GREATER "0")

set(properies_content " set_tests_properties(\"\${test_name}\" PROPERTIES\n")

math(EXPR num_properties "${properties_len} / 2")

foreach(i RANGE 0 ${num_properties} 2)

math(EXPR value_index "${i} + 1")

list(GET arg_PROPERTIES ${i} name)

list(GET arg_PROPERTIES ${value_index} value)

string(APPEND properies_content " \"${name}\" \"${value}\"\n")

endforeach()

string(APPEND properies_content " )\n")

set(properties_content)

list(LENGTH arg_PROPERTIES properties_len)

if(properties_len GREATER "0")

set(properies_content " set_tests_properties(\"\${test_name}\" PROPERTIES\n")

math(EXPR num_properties "${properties_len} / 2")

foreach(i RANGE 0 ${num_properties} 2)

math(EXPR value_index "${i} + 1")

list(GET arg_PROPERTIES ${i} name)

list(GET arg_PROPERTIES ${value_index} value)

string(APPEND properties_content " \"${name}\" \"${value}\"\n")

endforeach()

string(APPEND properties_content " )\n")

janb84 · 2025-10-17T11:00:57Z

cmake/module/DiscoverTests.cmake

+    "      string(REGEX REPLACE [[${arg_DISCOVERY_MATCH}]] [[${arg_TEST_ARGS_REPLACEMENT}]] test_args \"\${line}\")\n"
+    "      separate_arguments(test_args)\n"
+    "      add_test(\"\${test_name}\" \${launcher} \${emulator} \${runner} \${test_args})\n"
+    ${properies_content}


Suggested change

${properies_content}

${properties_content}

hebasto · 2025-10-21T18:15:55Z

Concept ACK.

hebasto · 2025-10-21T18:40:13Z

cmake/module/DiscoverTests.cmake

+    "execute_process(\n"
+    "  COMMAND \${launcher} \${emulator} \${runner} ${arg_DISCOVERY_ARGS}\n"
+    "  OUTPUT_VARIABLE output OUTPUT_STRIP_TRAILING_WHITESPACE\n"
+    "  ERROR_VARIABLE  output ERROR_STRIP_TRAILING_WHITESPACE\n"


Why is this needed?

hebasto

The following comments no longer seem needed:

bitcoin/src/test/CMakeLists.txt

Lines 7 to 8 in 2bedad4

    
           # Do not use generator expressions in test sources because the 
        
           # SOURCES property is processed to gather test suite macros.

bitcoin/src/wallet/test/CMakeLists.txt

Lines 5 to 6 in 2bedad4

    
           # Do not use generator expressions in test sources because the 
        
           # SOURCES property is processed to gather test suite macros.

willcl-ark · 2025-10-24T11:22:51Z

Concept ACK.

I have picked this commit into my master branch at https://github.com/willcl-ark/bitcoin along with a CDash config (and a nightly rebase job) so that I can have my nightly CI jobs upload to a demo cdash instance I configured: https://my.cdash.org/index.php?project=core

Seems to work pretty nicely so far :)

The cdash is currently open to all submissions so if anyone else would like to submit jobs, then feel free.

maflcko · 2025-10-24T12:00:52Z

CDash

Nice. I think all of this sounds great, but I still haven't seen the larger picture, as the cross-tests and functional tests are left out (#33483 (comment)), among many other things.

Again, not a blocker for this pull, and I am happy to continue discussion elsewhere. Though, generally my thinking is that the dashboard should not discriminate against tests written in other programming languages, or even tests written outside the tree of Bitcoin Core. For example, it would be great to have a dashboard with third party integrations of Bitcoin Core to answer the long standing QA problem of us (and the third parties) not knowing when an integration is broken and if it can be fixed. (Usually the rc phase or post-final phase is too late). Also, #18816 has been waiting for years with no one to implement it, as it is not trivial 5 minute task.

I am just saying this to clarify that this pull request is not the first and final step in getting a dashboard up. In fact, the benchmarks are already run through ctest, so this pull request here seems even optional on the topic of the dashboard.

So I'd love to see a larger design document around the dashboard. This may also help to get more people's wishlist items on it. (I've heard that corecheck could be fully or partially moved to the dashboard). Such a design document could help to ensure we are moving toward a shared goal (ideally with people already signed up to share the workload of moving there), instead of risking to end up with an even more fragmented CI/QA jungle.

maflcko · 2025-11-20T19:37:35Z

concept ACK, fwiw

maflcko · 2025-11-20T19:38:23Z

Looks like there are a bunch of code review comments. Are you still working on this?

maflcko · 2025-12-19T13:11:14Z

⌛ There hasn't been much activity lately. What is the status here?

* Is it still relevant? ➡️ Please address the review comments to make it ready for further review.

* Is it no longer relevant? ➡️ Please close.

* Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2026-01-05T23:49:28Z

🐙 This pull request conflicts with the target branch and needs rebase.

BrandonOdiwuor

Approach ACK 2bedad4, shifting test discovery from static source code parsing at configure time to dynamic runtime querying of the executables(e.g., via --list_content for Boost.Test-based unit tests and -list for benchmarks). This change facilitates integration with CTest, beginning with unit tests and now including benchmark tests, with clear potential extensions for Qt tests (noted as TODO: -function support) and even functional tests.

I have been running CTest nightly builds using Github actions against my fork that incorporate these changes, along with a custom solution for functional test discovery (inspired by the mechanism in this PR). I upload the results to a demo CDash instance at https://my.cdash.org/index.php?project=core. This approach has been effective for unified reporting and visualization, emphasizing the need for a comprehensive dashboard that could also include third-party integrations. However, as @maflcko pointed out, this PR is a positive step but not the final step in getting a dashboard. A larger design document around the dashboard would be valuable to incorporate more people's wishlist items, align on shared goals, and avoid ending up with a fragmented CI/QA.

I have a few questions

on test counts: CTest reports ~150 tests on master, but only ~142 on this branch in my local runs:

Tested on macOS 26.1 with ctest version 4.0.2

<branch: master>

<branch 2bedad455934c9b407329d0bf58521cf24510465>

script_assets_tests is reported as passed by CTest while it is shown as skipped when run directly by test_bitcoin

<using: CTest>

<using: test_bitcoin>

$ ./build/bin/test_bitcoin --run_test=script_assets_tests --log_level=test_suite

DrahtBot added the Build system label Sep 26, 2025

purpleKarrot mentioned this pull request Sep 26, 2025

test: Run bench sanity checks in parallel with functional tests #33142

Merged

janb84 reviewed Sep 29, 2025

View reviewed changes

hebasto reviewed Sep 30, 2025

View reviewed changes

CMake: Add dynamic test discovery

2bedad4

purpleKarrot force-pushed the ctest-test-discovery branch from 6318195 to 2bedad4 Compare October 1, 2025 13:16

janb84 reviewed Oct 17, 2025

View reviewed changes

hebasto mentioned this pull request Oct 21, 2025

cmake: Add dynamic test discovery to improve parallelism bitcoin-core/secp256k1#1760

Open

hebasto reviewed Oct 21, 2025

View reviewed changes

DrahtBot mentioned this pull request Oct 23, 2025

[wip] [nomerge] [draft] 2510 msan zero #33686

Closed

DrahtBot mentioned this pull request Nov 24, 2025

test: Remove system_tests/run_command runtime dependencies #33929

Open

DrahtBot added the Needs rebase label Jan 5, 2026

maflcko mentioned this pull request Jan 6, 2026

Slow unit tests delay functional tests and leave CPU unused #32770

Open

BrandonOdiwuor reviewed Jan 10, 2026

View reviewed changes

	# Do not use generator expressions in test sources because the
	# SOURCES property is processed to gather test suite macros.

CMake: Add dynamic test discovery #33483

Are you sure you want to change the base?

CMake: Add dynamic test discovery #33483

Conversation

purpleKarrot commented Sep 26, 2025

Uh oh!

DrahtBot commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

maflcko commented Sep 26, 2025

Uh oh!

janb84 commented Sep 29, 2025

Uh oh!

maflcko commented Sep 29, 2025

Uh oh!

janb84 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

janb84 left a comment

Choose a reason for hiding this comment

Uh oh!

janb84 Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

janb84 Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto commented Oct 21, 2025

Uh oh!

hebasto Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented Oct 24, 2025

Uh oh!

maflcko commented Oct 24, 2025

Uh oh!

maflcko commented Nov 20, 2025

Uh oh!

maflcko commented Nov 20, 2025

Uh oh!

maflcko commented Dec 19, 2025

Uh oh!

DrahtBot commented Jan 5, 2026

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Sep 26, 2025 •

edited

Loading