-
Notifications
You must be signed in to change notification settings - Fork 1.1k
cmake: Add dynamic test discovery 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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| # TODO: rework/remove once test discovery is implemented upstream: | ||
| # https://gitlab.kitware.com/cmake/cmake/-/issues/26920 | ||
| function(discover_tests target) | ||
| set(options "") | ||
| set(oneValueArgs DISCOVERY_MATCH TEST_NAME_REPLACEMENT TEST_ARGS_REPLACEMENT) | ||
| set(multiValueArgs DISCOVERY_ARGS PROPERTIES) | ||
| cmake_parse_arguments(PARSE_ARGV 1 arg "${options}" "${oneValueArgs}" "${multiValueArgs}") | ||
|
|
||
| set(file_base ${CMAKE_CURRENT_BINARY_DIR}/${target}) | ||
| set(include_file ${file_base}_include.cmake) | ||
|
|
||
| set(properties_content) | ||
| list(LENGTH arg_PROPERTIES properties_len) | ||
| if(properties_len GREATER "0") | ||
| set(properties_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") | ||
| endif() | ||
|
|
||
| string(CONCAT include_content | ||
| "set(runner [[$<TARGET_FILE:${target}>]])\n" | ||
| "set(launcher [[$<TARGET_PROPERTY:${target},TEST_LAUNCHER>]])\n" | ||
| "set(emulator [[$<$<BOOL:${CMAKE_CROSSCOMPILING}>:$<TARGET_PROPERTY:${target},CROSSCOMPILING_EMULATOR>>]])\n" | ||
| "\n" | ||
| "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" | ||
| " RESULT_VARIABLE result\n" | ||
| ")\n" | ||
| "\n" | ||
| "if(NOT result EQUAL 0)\n" | ||
| " add_test([[${target}_DISCOVERY_FAILURE]] \${launcher} \${emulator} \${runner} ${arg_DISCOVERY_ARGS})\n" | ||
| "else()\n" | ||
| " string(REPLACE \"\\n\" \";\" lines \"\${output}\")\n" | ||
| " foreach(line IN LISTS lines)\n" | ||
| " if(line MATCHES \"${arg_DISCOVERY_MATCH}\")\n" | ||
| " string(REGEX REPLACE \"${arg_DISCOVERY_MATCH}\" \"${arg_TEST_NAME_REPLACEMENT}\" test_name \"\${line}\")\n" | ||
| " 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" | ||
| ${properties_content} | ||
| " endif()\n" | ||
| " endforeach()\n" | ||
| "endif()\n" | ||
| ) | ||
|
|
||
| get_property(is_multi_config GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG) | ||
| if(is_multi_config) | ||
| file(GENERATE | ||
| OUTPUT ${file_base}_include-$<CONFIG>.cmake | ||
| CONTENT "${include_content}" | ||
| ) | ||
| file(WRITE ${include_file} | ||
| "include(\"${file_base}_include-\${CTEST_CONFIGURATION_TYPE}.cmake\")" | ||
| ) | ||
| else() | ||
| file(GENERATE | ||
| OUTPUT ${include_file} | ||
| CONTENT "${include_content}" | ||
| ) | ||
| endif() | ||
|
|
||
| set_property(DIRECTORY APPEND PROPERTY TEST_INCLUDE_FILES ${include_file}) | ||
| endfunction() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,15 +144,24 @@ if(SECP256K1_BUILD_TESTS) | |
| list(APPEND TEST_DEFINITIONS SUPPORTS_CONCURRENCY=1) | ||
| endif() | ||
|
|
||
| add_executable(noverify_tests tests.c) | ||
| target_link_libraries(noverify_tests secp256k1_precomputed secp256k1_asm) | ||
| target_compile_definitions(noverify_tests PRIVATE ${TEST_DEFINITIONS}) | ||
| add_test(NAME secp256k1_noverify_tests COMMAND noverify_tests) | ||
| function(add_executable_and_tests exe_name verify_definition) | ||
| 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}) | ||
| include(DiscoverTests) | ||
| discover_tests(${exe_name} | ||
| DISCOVERY_ARGS "--list_tests" | ||
| DISCOVERY_MATCH "^\\t\\\\[ *[0-9]+\\\\] ([^ ].*)$" | ||
| TEST_NAME_REPLACEMENT "secp256k1.${exe_name}.\\\\1" | ||
| TEST_ARGS_REPLACEMENT "--target=\\\\1 --log=1" | ||
| PROPERTIES | ||
| LABELS "secp256k1_${exe_name}" | ||
|
Comment on lines
+157
to
+158
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be nice to explain what LABELS are.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From Professional CMake: A Practical Guide 21st Edition:
|
||
| ) | ||
| endfunction() | ||
|
|
||
| add_executable_and_tests(noverify_tests "") | ||
| if(NOT CMAKE_BUILD_TYPE STREQUAL "Coverage") | ||
| add_executable(tests tests.c) | ||
| target_compile_definitions(tests PRIVATE VERIFY ${TEST_DEFINITIONS}) | ||
| target_link_libraries(tests secp256k1_precomputed secp256k1_asm) | ||
| add_test(NAME secp256k1_tests COMMAND tests) | ||
| add_executable_and_tests(tests VERIFY) | ||
| endif() | ||
| unset(TEST_DEFINITIONS) | ||
| endif() | ||
|
|
@@ -162,7 +171,10 @@ if(SECP256K1_BUILD_EXHAUSTIVE_TESTS) | |
| add_executable(exhaustive_tests tests_exhaustive.c) | ||
| target_link_libraries(exhaustive_tests secp256k1_asm) | ||
| target_compile_definitions(exhaustive_tests PRIVATE $<$<NOT:$<CONFIG:Coverage>>:VERIFY>) | ||
| add_test(NAME secp256k1_exhaustive_tests COMMAND exhaustive_tests) | ||
| add_test(NAME secp256k1.exhaustive_tests COMMAND exhaustive_tests) | ||
| set_tests_properties(secp256k1.exhaustive_tests PROPERTIES | ||
| LABELS secp256k1_exhaustive | ||
| ) | ||
| endif() | ||
|
|
||
| if(SECP256K1_BUILD_CTIME_TESTS) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes properties come in pairs, but someone could:
It would be good to check that the value exist before accessing it, and fail with a clear error if not: "Property 'name' has no associated value. Must be specified as name/value pair" or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every set property has a value. The syntax in the snippet above is simply incorrect. Compare it with the syntax of the
set_tests_propertiescommand.