Add DISCOVERY_ENVIRONMENT option to catch_discover_tests - #3200
Add DISCOVERY_ENVIRONMENT option to catch_discover_tests#3200itz-puneet wants to merge 2 commits into
Conversation
Test discovery runs the test executable in a separate process that does not inherit anything set through `PROPERTIES ENVIRONMENT`, since those properties are applied by CTest to the tests themselves. An executable that needs a particular environment merely to start up and list its test cases therefore fails at discovery time, even though the tests would run correctly. Reported cases include a missing shared library search path, a Qt platform plugin that has to be set to `offscreen` on a headless machine, and an instrumentation library whose extra output corrupts the JSON listing. `DL_PATHS` already solves this for the specific case of dynamic linker search paths. This adds `DISCOVERY_ENVIRONMENT`, which does the same for arbitrary `<var>=<value>` pairs. The variables are applied only for the duration of discovery, and any previous values are restored afterwards. That restore matters in PRE_TEST mode, where discovery runs inside the CTest process, and without it the variables would leak into the environment of the tests - which is explicitly not what the option promises. Closes catchorg#1810
There was a problem hiding this comment.
🟡 Changes recommended
The DISCOVERY_ENVIRONMENT save/restore logic can restore incorrect values when the same variable is specified multiple times, and the new helper test currently depends on the caller’s ambient environment (potential flakiness).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a CMake-facing way to inject environment variables specifically for Catch2 test discovery (the --list-tests run), addressing cases where the test executable needs a special environment just to start and emit JSON.
Changes:
- Adds
DISCOVERY_ENVIRONMENT <var>=<value>...tocatch_discover_testsand threads it through tocatch_discover_tests_impl. - Implements temporary environment application + restoration around the discovery run in
CatchAddTests.cmake. - Adds a pure-CMake helper test (and a fake listing “executable”) plus documentation updates for the new option.
File summaries
| File | Description |
|---|---|
extras/Catch.cmake |
Adds the public DISCOVERY_ENVIRONMENT option and forwards it into the discovery script for POST_BUILD and PRE_TEST modes. |
extras/CatchAddTests.cmake |
Applies TEST_DISCOVERY_ENVIRONMENT during discovery and restores environment afterward. |
docs/cmake-integration.md |
Documents the new option, its scope (discovery-only), and provides usage examples. |
tests/CMakeLists.txt |
Registers the new CMake helper test. |
tests/TestScripts/DiscoverTests/TestDiscoveryEnvironment.cmake |
Adds a unit test verifying propagation and non-leak/restoration semantics. |
tests/TestScripts/DiscoverTests/FakeListingExecutable.cmake |
Adds a fake listing script used by the new helper test to reflect environment state in the discovered test name. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A repeated entry such as `FOO=a;FOO=b` stashed the pre-discovery value on every iteration. By the second one `FOO` had already been modified, so the stash held `a` rather than the original value, and the restore put back `a` instead of what was there before discovery. The value is now stashed only the first time a name is seen, which leaves last-one-wins behaviour during discovery unchanged. The unit test asserted on a variable it inherited from the caller, so it could fail spuriously for anyone who happened to have that variable set, and its cleanup unset the variable unconditionally, clobbering a value the caller may have wanted. It now stashes and clears the variable up front and restores it at the end. Also adds a regression case covering the repeated-entry behaviour.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #3200 +/- ##
==========================================
+ Coverage 91.17% 91.21% +0.03%
==========================================
Files 206 206
Lines 9031 9031
==========================================
+ Hits 8234 8237 +3
+ Misses 797 794 -3 🚀 New features to boost your workflow:
|
Closes #1810
Description
Test discovery runs the test executable in a separate process, which does not inherit anything set through
PROPERTIES ENVIRONMENT— those are applied by CTest to the tests themselves, not to the listing run. An executable that needs a particular environment just to start up and list its test cases therefore fails at discovery time, even though the tests would run fine.The cases reported in #1810 are a missing shared library search path, a Qt platform plugin needing
offscreenon a headless machine, and an instrumentation library whose extra output corrupts the JSON listing.DL_PATHSalready solves this for dynamic linker search paths specifically, and its documented behaviour ("set when retrieving the list of test cases ... and when the tests are executed") is exactly the semantic people want here. This addsDISCOVERY_ENVIRONMENTfor arbitrary<var>=<value>pairs.Scope decision
The option applies to discovery only, not to test execution. Every case in the issue is discovery-side — the tests themselves already ran correctly via
PROPERTIES ENVIRONMENT. Making it discovery-scoped keeps the naming unambiguous and changes no existing behaviour. Happy to widen it to cover both if you would rather it mirrorDL_PATHSexactly.Restoring the previous environment
The variables are applied only for the duration of discovery and previous values are restored afterwards, including unsetting variables that were not previously defined. This matters in
PRE_TESTmode, wherecatch_discover_tests_implruns inside the CTest process — without the restore the variables would leak into the tests' environment, which is the opposite of what the option promises.The saved values are kept in per-variable CMake variables rather than parallel lists, because an environment variable can be unset or hold an empty string and CMake lists cannot represent those two cases distinctly.
Testing
Added
CMakeHelper::DiscoveryEnvironment, a pure-CMake unit test alongside the existingTestPrepareCommandFragment/TestDecomposeJsonArrayones. It drivescatch_discover_tests_implagainst a fake listing executable that reports a sentinel environment variable back through the test name, and asserts:The fake executable is a
cmake -Pscript rather than a shell script, so the test runs unchanged on Windows.All five assertions pass locally, and the two existing CMake helper tests still pass. Note that I ran these via
cmake -Pdirectly; I have not built the full test suite, so the wider CI run is the first check of thetests/CMakeLists.txtregistration.Worth flagging:
DL_PATHSandDL_FRAMEWORK_PATHSare documented in theCatch.cmakeheader comment but missing from the option list indocs/cmake-integration.md. I only added the new option there rather than fix that separately — happy to do so in another PR if useful.