Repository navigation
Test: Register Boost.Test suites discovered from the test binaries - #655
Conversation
…ilt binary The hand-maintained contract suite registry failed configure whenever a source declared a suite it did not list, so two pull requests that each pass CI, one adding a suite and one changing the registry, break configure on master once both merge, as #618 and #634 did. Its hand-written contract labels had also drifted: eight entries lacked contracts their suites deploy. After contracts_unit_test links, contracts/tools/discover_contract_suites.py reads the executable's --list_content=DOT listing and writes one CTest entry per top-level unit, so a new suite, or a test case declared outside any suite, is registered without a CMake edit. Each entry is labelled with the contracts referenced by the files that declare the unit, its test cases and their dependencies, and by the local headers those files include: accessor calls resolved through the accessor headers, and contract header includes. The sysio.bios and sysio.roa contracts that libtester deploys during default setup label every entry. If the executable cannot list itself, a single failing contract.DISCOVERY_FAILED entry carrying every label reports the error, so the build and the other tests still run. Before the executable is built, a failing contract.NOT_BUILT entry stands in. Editing the script relinks the executable, which reruns discovery.
Replace the Python discovery of contract suites with boost_test_discover_tests(), a plain-CMake module modeled on CMake's gtest_discover_tests(DISCOVERY_MODE PRE_TEST). When ctest starts, it lists a Boost.Test executable's top-level units with --list_content, caches the result until the executable changes, and registers one CTest entry per unit, so adding or removing a suite needs no CMake edit. Use it for every per-suite registration: unit_test and wasm_spec_test, which grepped their sources at configure time and never saw test cases declared outside a suite, test_chain_plugin, whose case list was maintained by hand, and contracts_unit_test. Every existing entry keeps its name, command, working directory, labels, environment and COST, except that the contract entries now carry only the contract label; nothing read the per-contract labels, and they had drifted. The file-scope chainbase_type_heap_alloc and chainbase_type_segment_alloc cases now run, the wasm-spec COST hints now apply (their loop iterated an undefined EOSIO_WASM_RUNTIMES), and the coverage target's selector, which matched nothing, now selects the unit_test entries. The unused unittest_tests_add macro is removed. A binary that cannot list itself fails a <target>_DISCOVERY_FAILED entry instead of the build, and a missing one a <target>_NOT_BUILT entry. boost_test_discovery_test covers the listing parser and, through a small project read back by ctest, the generated scripts.
…ite-discovery # Conflicts: # contracts/tests/CMakeLists.txt
heifner
changed the title
|
|
||
| # One ctest process discovers at a time, and one that waited finds the script already current. The script is stale | ||
| # when the executable or the include script is newer (a tie counts) or this implementation changed. | ||
| file(LOCK "${arg_TESTS_FILE}.lock" GUARD FUNCTION TIMEOUT ${_BOOST_TEST_LIST_TIMEOUT_SECONDS} |
There was a problem hiding this comment.
file(LOCK ... RESULT_VARIABLE lock_result) makes timeout and lock errors nonfatal, but lock_result is never checked. Discovery then removes and rewrites the shared .listing and .tmp files without owning the lock, so concurrent CTest processes can race or fail the rename. Please check for success before touching shared files, or let file(LOCK) fail fatally.
There was a problem hiding this comment.
Fixed in a1a0843. When the lock isn't acquired, discovery now leaves the shared files alone: it keeps the current list and registers a failing <target>_DISCOVERY_FAILED entry with the lock error, so the failure shows up in the test results rather than aborting ctest. The lock also waits 180 s now, longer than one discovery can take (the listing times out at 60 s), so a waiter outlasts a live holder. The regression test holds the lock from a second cmake -P process and checks that the attempt reports the error and leaves the existing tests file untouched.
| endif() | ||
| set(build "${work}/build") | ||
| run("configure" "${CMAKE_COMMAND}" -S "${project}" -B "${build}" -G "${GENERATOR}" ${compiler}) | ||
| listed_tests(tests "${build}") |
There was a problem hiding this comment.
When the top-level generator is multi-config, this nested project is multi-config too. listed_tests() omits -C, leaving CTEST_CONFIGURATION_TYPE empty and making the generated dispatcher include fake_discovery_1_include-.cmake. Please thread a configuration through the nested build and all CTest calls.
There was a problem hiding this comment.
Fixed in a1a0843. The test now gets -DCONFIG=$<CONFIG> and passes --config/-C to the nested build and every ctest call. The stand-in's output directory is pinned with a generator expression so file(TOUCH) finds the executable under multi-config generators too. I ran it with Ninja, Ninja Multi-Config (Debug and Release) and Unix Makefiles as the enclosing generator.
… its test A failed file(LOCK) was ignored, so a ctest process that timed out waiting for another's discovery went on to rewrite the shared listing and tests files without owning the lock. It now keeps the current list and registers a failing <target>_DISCOVERY_FAILED entry naming the lock error, and it waits 180 s, longer than one discovery can take, so a waiter outlasts a live holder. The regression test holds the lock from a second process and checks both. Under a multi-config generator the regression test's nested project was multi-config too, but it built and ran ctest without a configuration, so the generated dispatcher included <target>_discovery_1_include-.cmake. The test now receives $<CONFIG> and passes it to the nested build and every ctest call, and pins the stand-in executable's output directory so multi-config generators add no configuration subdirectory.
| set(build_config "") | ||
| set(ctest_config "") | ||
| if(CONFIG) | ||
| set(build_config --config "${CONFIG}") |
There was a problem hiding this comment.
CONFIG is only applied to cmake --build and CTest here; the nested configure still uses the generator defaults for CMAKE_CONFIGURATION_TYPES. When the outer multi-config build uses a valid custom configuration such as ASan, ctest -C ASan tries to include fake_discovery_1_include-ASan.cmake, which the nested configure never generated, and this self-test exits 8 before the build. I reproduced that with Ninja Multi-Config. Please pass the active configuration (or the parent configuration list) into the nested configure, for example with -DCMAKE_CONFIGURATION_TYPES=${CONFIG}.
There was a problem hiding this comment.
Fixed in 0836c39. The nested configure now gets -DCMAKE_CONFIGURATION_TYPES=${CONFIG}, so a custom configuration's include script is generated. I reproduced your ASan case with Ninja Multi-Config before the change; it passes now, as do Ninja, Ninja Multi-Config (Debug, Release) and Unix Makefiles.
Under a multi-config generator with a custom configuration such as ASan, the regression test's nested project kept the generator's default configurations, so ctest -C ASan included a <target>_discovery_1_include-ASan.cmake that was never generated. The nested configure now gets CMAKE_CONFIGURATION_TYPES from the configuration under test.
| # Lists arrive as single `;`-joined arguments; the ${ARGN} form splits them where PARSE_ARGV would not. | ||
| set(single_values EXECUTABLE TESTS_FILE INCLUDE_FILE FAILURE_NAME CMAKE_COMMAND WORKING_DIRECTORY NAME INCLUDE | ||
| EXCLUDE LOCK_TIMEOUT) | ||
| cmake_parse_arguments(arg "" "${single_values}" "NAME_STRIP;RUNTIMES;ARGS;PROPERTIES;COST_BY_NAME" ${ARGN}) |
There was a problem hiding this comment.
cmake_parse_arguments(... ${ARGN}) reparses every function argument through CMake list syntax, whose elements cannot contain unmatched [ or ]. A valid checkout/build path such as /tmp/pr655]review therefore corrupts the later argument boundaries; on this head arg_TESTS_FILE becomes empty, file(REMOVE) warns about an empty filename, and discovery registers no tests. Please preserve scalar arguments with positional/PARSE_ARGV handling and explicitly expand only the serialized multi-value options. _boost_test_append_command also iterates its ARGN as a list at line 28, so path-valued emitted arguments need the same lossless handling.
There was a problem hiding this comment.
Thanks, that's accurate, but I'd rather not handle it here. A checkout or build path with an unmatched [ or ] breaks CMake list handling generally, not just in this module: the file(GLOB UNIT_TESTS ...) → add_executable(... ${UNIT_TESTS}) pattern in unittests/ and contracts/tests/ merges every source path into one element and configure fails with "Cannot find source file" in such a directory, so discovery never runs. CMake's own GoogleTestAddTests.cmake parses with the same ${ARGN} form. Making this lossless would mean one argument per list element, PARSE_ARGV, and ARGV-indexed command building, all for paths the build can't use, so I'm leaving it as is.
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Re-reviewed current head 0836c39. The lock handling and multi-config issues are resolved, and targeted Ninja, Unix Makefiles, and Ninja Multi-Config validation (including a custom ASan configuration) passes. The unmatched-bracket path limitation is a non-blocking compatibility edge consistent with existing project and upstream helper behavior.
Summary
boost_test_discover_tests()(cmake/boost-test-discovery.cmake), modeled on CMake'sgtest_discover_tests(DISCOVERY_MODE PRE_TEST): when ctest starts it lists a Boost.Test executable's top-level units with--list_content, caches the result until the executable changes, and registers one CTest entry per unit.unit_test(per runtime, plus the-nofsgsvariants),wasm_spec_test,test_chain_pluginandcontracts_unit_test. Remove the unusedunittest_tests_addmacro.boost_test_discovery_test, which checks the listing parser and, through a small project read back by ctest, the generated scripts.Why
Each executable had its own scheme, and each could miss tests. #618's hand-maintained contract list broke configure on every platform when #634 added suites;
unit_testandwasm_spec_testgrepped their sources, sounit_testnever registered test cases declared outside a suite; andtest_chain_plugin's case list was edited by hand. Reading the units from the built executable, as CMake does for GoogleTest, leaves nothing to keep in step.Notes
chainbase_type_heap_allocandchainbase_type_segment_allocnow run on each runtime; the contract entries carry only thecontractlabel (nothing read Register contract suites as independent CTest tests #618's per-contract labels, and they had drifted); the wasm-spec COST hints now apply, since their loop iterated an undefinedEOSIO_WASM_RUNTIMES; and the coverage target's selector, which matched nothing, now selects theunit_testentries.<target>_DISCOVERY_FAILEDentry instead of the build. Before a binary is built, a failing<target>_NOT_BUILTentry stands in.