Skip to content

cmake: Split test cases to improve parallelism #1760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

cmake: Split test cases to improve parallelism #1760

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a9dfed
cmake, refactor: De-duplicate test-related code
hebasto Oct 21, 2025
7600f5f
cmake: Split test cases to improve parallelism
hebasto Oct 21, 2025
2d8da55
cmake: Set `LABELS` property for tests
hebasto Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,16 +144,42 @@ if(SECP256K1_BUILD_TESTS)
list(APPEND TEST_DEFINITIONS SUPPORTS_CONCURRENCY=1)
endif()

function(add_executable_and_tests exe_name verify_definition)
set(test_targets "")
set(test_names "")
# Start with the longest-running tests.
list(APPEND test_targets ellswift)
list(APPEND test_names ellswift)
list(APPEND test_targets ecmult_constants)
list(APPEND test_names ecmult_constants)
list(APPEND test_targets "ecmult_pre_g wnaf point_times_order ecmult_near_split_bound ecmult_chain ecmult_gen_blind ecmult_const_tests ecmult_multi_tests ec_combine")
list(APPEND test_names ecmult)
# Continue with the remaining tests.
list(APPEND test_targets integer scalar field group ec ecdh ecdsa recovery extrakeys schnorrsig musig)
Comment on lines +154 to +157
Copy link
Contributor

@real-or-random real-or-random Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you have quotation marks in the first line here but not in the last?

Copy link
Member Author

@hebasto hebasto Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first line has quotes to combine the quoted test cases into a single ctest's test "ecmult".

In the last line, all mentioned test cases and modules are wrapped as its own ctest's tests.

Copy link
Contributor

@purpleKarrot purpleKarrot Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take @real-or-random's question as a proof that procedural logic in CMakeLists.txt is hard to reason about and should be avoided.

Copy link
Contributor

@purpleKarrot purpleKarrot Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we switch to automated test discovery in a follow-up, we will no longer have the ability to combine test cases. Maybe we should keep them separated so that the follow-up will not change the number of tests again?

Copy link
Contributor

@real-or-random real-or-random Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should keep them separated so that the follow-up will not change the number of tests again?

Sounds good, let's keep it simple.

For test ordering, we could perhaps simply make sure that automated discovery runs the tests in the order they are listed by tests -l.

list(APPEND test_names integer scalar field group ec ecdh ecdsa recovery extrakeys schnorrsig musig)
# Combine short, trivial tests for log brevity.
list(APPEND test_targets "general hash utils")
list(APPEND test_names "general,hash,utils")

function(add_executable_and_tests exe_name verify_definition label)
add_executable(${exe_name} tests.c)
target_link_libraries(${exe_name} secp256k1_precomputed secp256k1_asm)
target_compile_definitions(${exe_name} PRIVATE ${verify_definition} ${TEST_DEFINITIONS})
add_test(NAME secp256k1_${exe_name} COMMAND ${exe_name})
foreach(test_target test_name IN ZIP_LISTS test_targets test_names)
string(REPLACE " " ";" test_target "${test_target}")
list(TRANSFORM test_target PREPEND "--target=")
add_test(NAME ${label}::${test_name}
COMMAND ${exe_name} ${test_target} --log=1
COMMAND_EXPAND_LISTS
)
set_tests_properties(${label}::${test_name} PROPERTIES
SKIP_REGULAR_EXPRESSION "module disabled"
)
endforeach()
endfunction()

add_executable_and_tests(noverify_tests "")
add_executable_and_tests(noverify_tests "" secp256k1_noverify)
if(NOT CMAKE_BUILD_TYPE STREQUAL "Coverage")
add_executable_and_tests(tests VERIFY)
add_executable_and_tests(tests VERIFY secp256k1_verify)
Copy link
Contributor

@purpleKarrot purpleKarrot Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The target name tests is very generic and likely to cause clashes when the project is used as a subproject. This is orthogonal to the PR, but should be kept in mind for a follow up.

endif()
unset(TEST_DEFINITIONS)
endif()
Expand Down