Pass CMake details to e2e tests via a configuration file#7713
Pass CMake details to e2e tests via a configuration file#7713eddyashton wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces the amount of CMake-derived CLI flags passed to end-to-end tests by generating an e2e_config.json at configure time and having e2e_args.py consume it as argument defaults.
Changes:
- Generate
e2e_config.jsonfrom CMake configuration and populate it with common e2e parameters. - Update
e2e_args.pyto load config values as argparse defaults (while keeping CLI args higher precedence). - Simplify
add_e2e_testcommand lines so only test-specific args are passed explicitly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/infra/e2e_args.py | Loads e2e_config.json from CWD and applies it as argparse defaults, with special handling for constitution. |
| cmake/e2e_config.json.in | New JSON template filled by CMake configure-time substitution. |
| cmake/common.cmake | Removes common CLI args from add_test() invocations, relying on config instead. |
| CMakeLists.txt | Produces default constitution lists and generates e2e_config.json into the build directory. |
| # Build JSON array of default constitution paths for e2e config | ||
| list(TRANSFORM CCF_DEFAULT_CONSTITUTION_FILES REPLACE "(.+)" "\"\\1\"" | ||
| OUTPUT_VARIABLE _quoted | ||
| ) | ||
| list(JOIN _quoted ", " DEFAULT_CONSTITUTION_JSON) | ||
| set(DEFAULT_CONSTITUTION_JSON "[${DEFAULT_CONSTITUTION_JSON}]") | ||
|
|
||
| # Generate e2e test config consumed by tests/infra/e2e_args.py | ||
| configure_file( | ||
| ${CCF_DIR}/cmake/e2e_config.json.in ${CMAKE_BINARY_DIR}/e2e_config.json @ONLY | ||
| ) |
There was a problem hiding this comment.
The JSON generation here does not escape file paths correctly. On Windows (and any environment where paths may contain backslashes), the resulting DEFAULT_CONSTITUTION_JSON can produce invalid JSON (e.g., \\n, \\t, \\uXXXX escapes, etc.). Consider generating JSON using a JSON-aware mechanism (e.g., CMake string(JSON ...) if available) or at minimum normalizing paths to forward slashes (e.g., file(TO_CMAKE_PATH ...)) and escaping backslashes/quotes before inserting into JSON.
| { | ||
| "binary_dir": "@CMAKE_BINARY_DIR@", | ||
| "log_level": "@TEST_LOGGING_LEVEL@", | ||
| "worker_threads": @WORKER_THREADS@, | ||
| "tick_ms": @NODE_TICK_MS@, | ||
| "default_constitution": @DEFAULT_CONSTITUTION_JSON@ | ||
| } |
There was a problem hiding this comment.
binary_dir is injected as a raw string without JSON escaping. If @CMAKE_BINARY_DIR@ contains backslashes or quotes (notably on Windows), the generated e2e_config.json may be invalid JSON. Update generation so binary_dir is JSON-escaped (or normalized to forward slashes before substitution) to ensure json.load() consistently succeeds.
| ${PARSED_ARGS_NAME} ${CCF_NETWORK_TEST_ARGS} ${PARSED_ARGS_CONSTITUTION} | ||
| ${PARSED_ARGS_ADDITIONAL_ARGS} --tick-ms ${NODE_TICK_MS} | ||
| COMMAND ${E2E_CMD} | ||
| CONFIGURATIONS ${PARSED_ARGS_CONFIGURATIONS} |
There was a problem hiding this comment.
e2e_args.py discovers e2e_config.json via os.getcwd(), but this test definition does not pin the working directory. If the working directory differs (custom runners/IDEs, direct invocation, or future CTest behavior changes), the config won’t be found and defaults will silently differ from configured values. Prefer setting an explicit WORKING_DIRECTORY for the test (to the build dir where e2e_config.json is generated), or pass the config path via an env var/property that e2e_args.py can read.
| CONFIGURATIONS ${PARSED_ARGS_CONFIGURATIONS} | |
| CONFIGURATIONS ${PARSED_ARGS_CONFIGURATIONS} | |
| WORKING_DIRECTORY ${CMAKE_BINARY_DIR} |
| ) | ||
|
|
||
| # Constitution: only pass on CLI when explicitly overridden for this test. | ||
| # Otherwise e2e_args.py falls back to default_constitution from config. |
There was a problem hiding this comment.
I think there's a question mark over whether we want test-specific overrides in the config as defaults, for when one decides to manually uv run this_test.py.
My sense is "yes, that would be a nice QoL improvement", but I can see that it means the defaults will vary test to test also, for the same CLI, which can be surprising.
Another scheme may be to still capture a single configuration for all tests, but only grab generic defaults there that must come from CMake (directories, san_is_enabled), and have each test configure its defaults explicitly in the Python file, using the config.
I think this is probably the better scheme in the longer run, because it minimises the size of the CMake-to-Python interface, as well as the amount of test-related-CMake. The more stuff lives on the Python side, the easier it is to tweak without a round-trip through ninja.
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "binary_dir": "@CMAKE_BINARY_DIR@", | |||
There was a problem hiding this comment.
I do not know if we need an explicit binary_dir, because I think it's always the same as the directory where we discover the configuration. I was thinking of something like adding a --cfg switch to e2e_args, which probably defaults to ../build, and have binary_dir default to what args.cfg is set to.
Another idea from @achamayou towards eliminating ctest.
Each
add_e2e_testcurrently passes many common args, describing CMake-derived parameters for the e2e tests. These can be dumped to file and read bye2e_args.py, rather than always passed explicitly as CLI args.This doesn't do anything for test-specific args, but I think we should separately do a pass making them non-CLI args, or at least having the baked value as default (deciding whether it's conceivably manually overridable or not).