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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33483.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | janb84, hebasto, willcl-ark |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Seems nice, but in the great picture there is still room for improvement:
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.
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
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)
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
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
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.
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:
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)
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
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)
CTEST_ seems to be reserved, no?
21+ math(EXPR value_index "${i} + 1")
22+ list(GET arg_PROPERTIES ${i} name)
23+ list(GET arg_PROPERTIES ${value_index} value)
24+ string(APPEND properies_content " \"${name}\" \"${value}\"\n")
25+ endforeach()
26+ string(APPEND properies_content " )\n")
NIT small typo in variable :
properies_content -> properties_content
0 set(properties_content)
1 list(LENGTH arg_PROPERTIES properties_len)
2 if(properties_len GREATER "0")
3 set(properies_content " set_tests_properties(\"\${test_name}\" PROPERTIES\n")
4 math(EXPR num_properties "${properties_len} / 2")
5 foreach(i RANGE 0 ${num_properties} 2)
6 math(EXPR value_index "${i} + 1")
7 list(GET arg_PROPERTIES ${i} name)
8 list(GET arg_PROPERTIES ${value_index} value)
9 string(APPEND properties_content " \"${name}\" \"${value}\"\n")
10 endforeach()
11 string(APPEND properties_content " )\n")
46+ " if(line MATCHES [[${arg_DISCOVERY_MATCH}]])\n"
47+ " string(REGEX REPLACE [[${arg_DISCOVERY_MATCH}]] [[${arg_TEST_NAME_REPLACEMENT}]] test_name \"\${line}\")\n"
48+ " string(REGEX REPLACE [[${arg_DISCOVERY_MATCH}]] [[${arg_TEST_ARGS_REPLACEMENT}]] test_args \"\${line}\")\n"
49+ " separate_arguments(test_args)\n"
50+ " add_test(\"\${test_name}\" \${launcher} \${emulator} \${runner} \${test_args})\n"
51+ ${properies_content}
0 ${properties_content}
Concept ACK 2bedad455934c9b407329d0bf58521cf24510465
Thanks for incorporating my suggestion (and those form the other reviewers) !
Found one typo in the cmake file
32+ "set(emulator [[$<$<BOOL:${CMAKE_CROSSCOMPILING}>:$<TARGET_PROPERTY:${target},CROSSCOMPILING_EMULATOR>>]])\n"
33+ "\n"
34+ "execute_process(\n"
35+ " COMMAND \${launcher} \${emulator} \${runner} ${arg_DISCOVERY_ARGS}\n"
36+ " OUTPUT_VARIABLE output OUTPUT_STRIP_TRAILING_WHITESPACE\n"
37+ " ERROR_VARIABLE output ERROR_STRIP_TRAILING_WHITESPACE\n"
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.
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 (https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3340123470), 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.