Skip to content

Pass CMake details to e2e tests via a configuration file #7713

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.

Sign up for GitHub

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

Draft
eddyashton wants to merge 2 commits into
base: main
Choose a base branch
from
+75 −21
Draft

Pass CMake details to e2e tests via a configuration file #7713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba65be2
Pass CMake details to e2e tests via a configuration file
eddyashton Mar 6, 2026
a2c9365
Merge branch 'main' of https://github.com/microsoft/CCF into cmake_pr…
eddyashton Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,20 +283,43 @@ set(WORKER_THREADS
CACHE STRING "Number of worker threads to start on each CCF node"
)

set(CCF_NETWORK_TEST_DEFAULT_CONSTITUTION
--constitution
# Default constitution file paths (used for both CLI args and config generation)
set(CCF_DEFAULT_CONSTITUTION_FILES
${CCF_DIR}/samples/constitutions/default/actions.js
--constitution
${CCF_DIR}/samples/constitutions/default/validate.js
--constitution
${CCF_DIR}/samples/constitutions/default/resolve.js
--constitution
${CCF_DIR}/samples/constitutions/default/apply.js
)

# Build the --constitution CLI args from the file list
set(CCF_NETWORK_TEST_DEFAULT_CONSTITUTION "")
foreach(f ${CCF_DEFAULT_CONSTITUTION_FILES})
list(APPEND CCF_NETWORK_TEST_DEFAULT_CONSTITUTION --constitution ${f})
endforeach()

set(CCF_NETWORK_TEST_ARGS --log-level ${TEST_LOGGING_LEVEL} --worker-threads
${WORKER_THREADS}
)

# Tick period: faster for non-instrumented builds
if(SAN)
set(NODE_TICK_MS 10)
else()
set(NODE_TICK_MS 1)
endif()

# 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
)
Comment on lines +311 to +321
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

# SNIPPET_START: JS generic application
add_ccf_app(
js_generic SRCS ${CCF_DIR}/src/apps/js_generic/js_generic.cpp
Expand Down
36 changes: 20 additions & 16 deletions cmake/common.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,6 @@ function(add_e2e_test)
"CONSTITUTION;ADDITIONAL_ARGS;CONFIGURATIONS"
)

if(NOT PARSED_ARGS_CONSTITUTION)
set(PARSED_ARGS_CONSTITUTION ${CCF_NETWORK_TEST_DEFAULT_CONSTITUTION})
endif()

if(BUILD_END_TO_END_TESTS)
if(PROFILE_TESTS)
set(PYTHON_WRAPPER
Expand All @@ -86,24 +82,32 @@ function(add_e2e_test)
set(PYTHON_WRAPPER ${PYTHON})
endif()

# For fast e2e runs, tick node faster than default value (except for
# instrumented builds which may process ticks slower).
if(SAN)
set(NODE_TICK_MS 10)
else()
set(NODE_TICK_MS 1)
endif()

if(NOT PARSED_ARGS_PERF_LABEL)
set(PARSED_ARGS_PERF_LABEL ${PARSED_ARGS_NAME})
endif()

# Build the command line. Global defaults (binary_dir, log_level,
# worker_threads, tick_ms, default_constitution) are now provided by the
# e2e_config.json generated at configure time and consumed by e2e_args.py.
# Only test-specific args remain here.
set(E2E_CMD
${PYTHON_WRAPPER} ${PARSED_ARGS_PYTHON_SCRIPT} --label
${PARSED_ARGS_NAME}
)

# Constitution: only pass on CLI when explicitly overridden for this test.
# Otherwise e2e_args.py falls back to default_constitution from config.
Copy link
Member

@achamayou achamayou Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if(PARSED_ARGS_CONSTITUTION)
list(APPEND E2E_CMD ${PARSED_ARGS_CONSTITUTION})
endif()

if(PARSED_ARGS_ADDITIONAL_ARGS)
list(APPEND E2E_CMD ${PARSED_ARGS_ADDITIONAL_ARGS})
endif()

add_test(
NAME ${PARSED_ARGS_NAME}
COMMAND
${PYTHON_WRAPPER} ${PARSED_ARGS_PYTHON_SCRIPT} -b . --label
${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}
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
CONFIGURATIONS ${PARSED_ARGS_CONFIGURATIONS}
CONFIGURATIONS ${PARSED_ARGS_CONFIGURATIONS}
WORKING_DIRECTORY ${CMAKE_BINARY_DIR}

Copilot uses AI. Check for mistakes.
Copy link
Member

@achamayou achamayou Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me

)

Expand Down
7 changes: 7 additions & 0 deletions cmake/e2e_config.json.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"binary_dir": "@CMAKE_BINARY_DIR@",
Copy link
Member

@achamayou achamayou Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"log_level": "@TEST_LOGGING_LEVEL@",
"worker_threads": @WORKER_THREADS@,
"tick_ms": @NODE_TICK_MS@,
"default_constitution": @DEFAULT_CONSTITUTION_JSON@
}
Comment on lines +1 to +7
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
20 changes: 20 additions & 0 deletions tests/infra/e2e_args.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the Apache 2.0 License.
import argparse
import json
import os
import infra.interfaces
import infra.path
Expand Down Expand Up @@ -384,13 +385,32 @@ def cli_args(
type=str,
default=infra.clients.API_VERSION_01,
)

add(parser)

# Look for e2e_config.json in cwd (the build dir when run via ctest).
# Values from the config are applied as parser defaults so that explicit
# CLI args still take precedence.
e2e_config = None
config_path = os.path.join(os.getcwd(), "e2e_config.json")
if os.path.isfile(config_path):
with open(config_path, encoding="utf-8") as f:
e2e_config = json.load(f)
config_defaults = {
k: v for k, v in e2e_config.items() if k != "default_constitution"
}
parser.set_defaults(**config_defaults)

if accept_unknown:
args, unknown_args = parser.parse_known_args()
else:
args = parser.parse_args()

# Constitution uses append action, so set_defaults can't help.
# Fall back to config value when nothing was passed on the CLI.
if e2e_config and not args.constitution:
args.constitution = e2e_config.get("default_constitution", [])

args.binary_dir = os.path.abspath(args.binary_dir)

if args.library_dir is None:
Expand Down
Loading