Register one ctest test per Catch2 test case - #13658
Conversation
Registering whole executables means a failure names the binary rather than the case that broke, a single case cannot be run on its own, and ctest can only schedule at executable granularity. Use catch_discover_tests so ctest sees each TEST_CASE, which takes the suite from ~100 entries to 1022. That only works if cases stop depending on each other, since each now runs in its own process concurrently with the rest. Fixed the fallout: - test_http2 relied on whichever case ran first calling url_init() and friends; moved that into the test-run listener. - The plugin.yaml, storage/plugin/ssl_multicert config, and cache shm tests each shared one fixed temp file or POSIX shm name across every case, so concurrent cases deleted each other's state. Made those per process. The config tests get a directory per process rather than unique file names, which preserves the names they depend on. - test_RefCountCache has its own main() and is not a Catch2 runner, so it stays a single add_test. add_catch2_test grows an ENVIRONMENT argument. It applies the variables via a `cmake -E env` prefix instead of ctest's ENVIRONMENT property because catch_discover_tests flattens PROPERTIES into one ;-joined string, which silently dropped all but the first variable and clobbered SKIP_RETURN_CODE. --order decl is gone: with one case per process there is no order left to pin.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Switches Catch2-based unit tests from “one ctest entry per executable” to “one ctest entry per Catch2 test case” using catch_discover_tests, and updates a handful of tests/build rules to be safe when cases run concurrently in separate processes.
Changes:
- Introduce
add_catch2_test()support for per-case discovery and per-test environment injection. - Move HTTP2 header subsystem initialization into the Catch2 listener, and harden tests that previously shared global temp/shm names.
- Convert
test_RefCountCacheto a plainadd_test()since it can’t be discovered as Catch2 cases.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/proxy/unit_tests/test_PluginYAML.cc | Make plugin YAML temp file names unique per process/instance. |
| src/proxy/http2/unit_tests/test_HTTP2.cc | Remove per-test-case initialization now handled in main listener. |
| src/proxy/http2/unit_tests/main.cc | Initialize URL/MIME/HTTP/HTTP2 subsystems once per process in listener. |
| src/proxy/http/remap/unit-tests/CMakeLists.txt | Pass ASan/LSan environment via add_catch2_test(ENVIRONMENT ...). |
| src/mgmt/rpc/server/unit_tests/test_rpcserver.cc | Improve diagnostics when JSONRPC test server fails to start. |
| src/iocore/net/CMakeLists.txt | Apply ASan env via add_catch2_test(ENVIRONMENT ...) on non-Apple. |
| src/iocore/hostdb/unit_tests/CMakeLists.txt | Use add_test() for RefCountCache test runner. |
| src/iocore/cache/unit_tests/test_CacheShmShutdown.cc | Make shm prefix unique per process to avoid cross-test interference. |
| src/iocore/cache/unit_tests/test_CacheShm.cc | Make purge shm prefix unique per process to avoid cross-test interference. |
| src/config/unit_tests/test_storage.cc | Use per-process temp directory so concurrent processes don’t collide. |
| src/config/unit_tests/test_ssl_multicert.cc | Use per-process temp directory so concurrent processes don’t collide. |
| src/config/unit_tests/test_plugin_config.cc | Use per-process temp directory so concurrent processes don’t collide. |
| plugins/experimental/uri_signing/unit_tests/CMakeLists.txt | Move LSAN suppression into add_catch2_test(ENVIRONMENT ...). |
| plugins/esi/test/CMakeLists.txt | Move LSAN suppression into add_catch2_test(ENVIRONMENT ...). |
| lib/CMakeLists.txt | Implement add_catch2_test() using catch_discover_tests and env prefixing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Reviewed the per-case CTest registration and supporting isolation changes. Found one reproducible parallel-execution regression, detailed inline. Validation used the PR's bundled Catch2, libswoc, and add_catch2_test macro in a standalone CMake build; I did not run the full ATS suite.
Follow-ups from review of the per-case ctest discovery change: - add_catch2_test prepended a `cmake -E env` wrapper by overwriting CROSSCOMPILING_EMULATOR, which would drop an emulator a cross-compiling build had already put there. Preserve and prepend instead. - The macro derived its target from the first COMMAND element and silently dropped the rest. No caller passes more than one, and per-case discovery has nowhere to put runner args, so reject that at configure time rather than lose it quietly. - The per-process temp directory the config tests use was never removed, leaving an empty directory per test process behind. It now owns its cleanup, and it and TempFile move into a shared header rather than being duplicated verbatim in three test files. - test_RefCountCache linked Catch2::Catch2WithMain despite having its own main() and using no Catch2; drop the dependency.
file::path::create_directories and ts_file::path::remove both built temp_directory_path()/"dir1" and recursively removed it, asserting on the removal counts. That was safe only because they ran serially in one executable; per-case ctest entries let them run at the same time and delete each other's tree. Name them per case, as file::path::canonical and file::path::copy in the same file already do. Reproduced with `ctest -R '^test_libswoc\..*(create_directories|path::remove)' -j2 --repeat until-fail:100`: fails without this, clean with it.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
- TempFile/TempYAML destructors called the throwing std::filesystem::remove overload, so a cleanup failure during unwinding would terminate the process instead of failing the test. Use the error_code overload. - Shorten the cache shm test prefixes. "/<word>-control" has to fit in MAX_SHM_NAME_LEN, leaving 22 characters for the word; "atspurgetest" plus a pid left only two to spare, which is thin for a platform with wider pids. Note the budget alongside them. - Factor the ASan/LSan environment the three remap dlopen tests share into one variable rather than repeating the string. - Fix an "ORD violation" typo (ODR).
bneradt
left a comment
bneradt
left a comment
There was a problem hiding this comment.
Re-reviewed the updates through 0993e14; no new actionable findings. The earlier libswoc directory collision remains fixed (previously verified with 100 parallel repetitions per affected case). The latest cleanup, shared-memory prefix, and sanitizer-environment refactoring changes look sound. I also verified the native-build environment behavior with a standalone smoke test using the PR registration macro and vendored Catch2: both variables in a semicolon-separated environment list reached the test, and a pre-existing emulator prefix was preserved (6 assertions passed). All 15 reported CI checks are green. I did not rerun the full ATS suite locally.
Resurrecting #12703 but with help from Claude
Registering whole executables means a failure names the binary rather than the case that broke, a single case cannot be run on its own, and ctest can only schedule at executable granularity. Use catch_discover_tests so ctest sees each TEST_CASE, which takes the suite from ~100 entries to 1022.
That only works if cases stop depending on each other, since each now runs in its own process concurrently with the rest. Fixed the fallout:
add_catch2_test grows an ENVIRONMENT argument. It applies the variables via a
cmake -E envprefix instead of ctest's ENVIRONMENT property because catch_discover_tests flattens PROPERTIES into one ;-joined string, which silently dropped all but the first variable and clobbered SKIP_RETURN_CODE. --order decl is gone: with one case per process there is no order left to pin.