CMake: Add dynamic test discovery

purpleKarrot commented at 5:21 pm on September 26, 2025: contributor

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 added the label Build system on Sep 26, 2025

DrahtBot commented at 5:21 pm on September 26, 2025: contributor

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
Approach ACK	janb84

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 commented at 7:09 pm on September 26, 2025: member

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 (https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/.github/workflows/ci.yml#L388)
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 commented at 9:51 am on September 29, 2025: contributor

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

Master : 150 This PR: 368

maflcko commented at 9:56 am on September 29, 2025: member

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)

in cmake/module/DiscoverTests.cmake:6 in 6318195b8e outdated

0@@ -0,0 +1,69 @@
1+# Copyright (c) 2025-present The Bitcoin Core developers
2+# Distributed under the MIT software license, see the accompanying
3+# file COPYING or https://opensource.org/license/mit/.
4+
5+# https://gitlab.kitware.com/cmake/cmake/-/issues/26920

janb84 commented at 10:35 am on September 29, 2025:

This comment line, is this intended as a reminder to remove this module if the code is added upstream ?

if so, Nit: please make the intent more explicit. e.g

0#TODO remove file if fixed upstream: 
1# https://gitlab.kitware.com/cmake/cmake/-/issues/26920

janb84 commented at 10:41 am on September 29, 2025: contributor

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.

hebasto commented at 2:48 pm on September 30, 2025: member

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:

 0$ ctest --test-dir build -j 20
 1<snip>
 273/148 Test  [#88](/bitcoin-bitcoin/88/): script_assets_tests ..................***Skipped   0.13 sec
 3<snip>
 4
 5100% tests passed, 0 tests failed out of 148
 6
 7Total Test time (real) =  37.05 sec
 8
 9The following tests did not run:
10	 88 - script_assets_tests (Skipped)

this PR @ 6318195b8ecea6d4ced57c8aac97e1ca713d9822:

 0$ ctest --test-dir build -j 20
 1<snip>
 273/148 Test  [#88](/bitcoin-bitcoin/88/): bitcoin.test.script_assets_tests ..................   Passed    0.14 sec
 3<snip>
 4
 5100% tests passed, 0 tests failed out of 148
 6
 7Label Time Summary:
 8test    =  96.99 sec*proc (142 tests)
 9
10Total Test time (real) =  38.24 sec

CMake: Add dynamic test discovery 2bedad4559

in cmake/module/DiscoverTests.cmake:11 in 6318195b8e

 6+function(discover_tests target)
 7+  set(oneValueArgs DISCOVERY_MATCH TEST_NAME_REPLACEMENT TEST_ARGS_REPLACEMENT)
 8+  set(multiValueArgs DISCOVERY_ARGS PROPERTIES)
 9+  cmake_parse_arguments(PARSE_ARGV 1 arg "" "${oneValueArgs}" "${multiValueArgs}")
10+
11+  get_property(has_counter TARGET ${target} PROPERTY CTEST_DISCOVERED_TEST_COUNTER SET)

hebasto commented at 2:49 pm on September 30, 2025:

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

purpleKarrot force-pushed on Oct 1, 2025

CMake: Add dynamic test discovery #33483

Code Coverage & Benchmarks

Reviews

Conflicts