Copilot commented on code in PR #13658:
URL: https://github.com/apache/trafficserver/pull/13658#discussion_r3972826728
##########
lib/CMakeLists.txt:
##########
@@ -43,14 +44,38 @@ if(BUILD_TESTING)
target_compile_options(Catch2 INTERFACE -Wno-error=parentheses)
target_compile_options(Catch2WithMain INTERFACE -Wno-error=parentheses)
- # By default, as of v3.9.0, Catch2 runs tests in random order. Our tests are
- # written with the expectation that they are run in declared order, with
setup
- # done in one TEST_CASE expected to carry over into the next TEST_CASE. This
- # macro allows us to add common command line arguments to all tests. For now,
- # we ensure declaration order execution via --order decl.
+ # Register one ctest test per Catch2 TEST_CASE/SCENARIO rather than one per
+ # executable. ctest then names the failing case instead of the binary, can
+ # schedule cases in parallel, and can run a single case by name. Because each
+ # case gets its own process, cases must not depend on state set up by an
+ # earlier case in the same executable.
+ #
+ # ctest names are "<NAME>.<case name>" so the NAME given here stays a usable
+ # -R filter prefix even where it differs from the target name.
+ #
+ # Discovery is PRE_TEST so that configuring/building does not require running
+ # the test executables.
macro(add_catch2_test)
- cmake_parse_arguments(CATCH2_TEST "" "NAME" "COMMAND" ${ARGN})
- add_test(NAME ${CATCH2_TEST_NAME} COMMAND ${CATCH2_TEST_COMMAND} --order
decl)
+ cmake_parse_arguments(CATCH2_TEST "" "NAME" "COMMAND;ENVIRONMENT" ${ARGN})
+
+ list(GET CATCH2_TEST_COMMAND 0 _catch2_command)
+ if(_catch2_command MATCHES "^\\$<TARGET_FILE:(.+)>$")
+ set(_catch2_target "${CMAKE_MATCH_1}")
+ else()
+ set(_catch2_target "${_catch2_command}")
+ endif()
+
+ if(CATCH2_TEST_ENVIRONMENT)
+ # Not ctest's ENVIRONMENT property: catch_discover_tests flattens
PROPERTIES
+ # into one ;-joined string, so a multi-variable ENVIRONMENT loses every
+ # variable but the first and turns the rest into junk property names. The
+ # emulator prefix survives intact and also applies to test discovery.
+ set_property(
+ TARGET ${_catch2_target} PROPERTY CROSSCOMPILING_EMULATOR
"${CMAKE_COMMAND}" -E env ${CATCH2_TEST_ENVIRONMENT}
+ )
Review Comment:
Overwriting `CROSSCOMPILING_EMULATOR` can break existing cross-compiling
setups (e.g., QEMU/SSH emulators) because this replaces any emulator already
configured on the target. A safer approach is to preserve any existing value
and prepend the `cmake -E env ...` wrapper ahead of it (or use a different
mechanism that doesn’t consume the emulator slot).
##########
lib/CMakeLists.txt:
##########
@@ -43,14 +44,38 @@ if(BUILD_TESTING)
target_compile_options(Catch2 INTERFACE -Wno-error=parentheses)
target_compile_options(Catch2WithMain INTERFACE -Wno-error=parentheses)
- # By default, as of v3.9.0, Catch2 runs tests in random order. Our tests are
- # written with the expectation that they are run in declared order, with
setup
- # done in one TEST_CASE expected to carry over into the next TEST_CASE. This
- # macro allows us to add common command line arguments to all tests. For now,
- # we ensure declaration order execution via --order decl.
+ # Register one ctest test per Catch2 TEST_CASE/SCENARIO rather than one per
+ # executable. ctest then names the failing case instead of the binary, can
+ # schedule cases in parallel, and can run a single case by name. Because each
+ # case gets its own process, cases must not depend on state set up by an
+ # earlier case in the same executable.
+ #
+ # ctest names are "<NAME>.<case name>" so the NAME given here stays a usable
+ # -R filter prefix even where it differs from the target name.
+ #
+ # Discovery is PRE_TEST so that configuring/building does not require running
+ # the test executables.
macro(add_catch2_test)
- cmake_parse_arguments(CATCH2_TEST "" "NAME" "COMMAND" ${ARGN})
- add_test(NAME ${CATCH2_TEST_NAME} COMMAND ${CATCH2_TEST_COMMAND} --order
decl)
+ cmake_parse_arguments(CATCH2_TEST "" "NAME" "COMMAND;ENVIRONMENT" ${ARGN})
+
+ list(GET CATCH2_TEST_COMMAND 0 _catch2_command)
+ if(_catch2_command MATCHES "^\\$<TARGET_FILE:(.+)>$")
+ set(_catch2_target "${CMAKE_MATCH_1}")
+ else()
+ set(_catch2_target "${_catch2_command}")
+ endif()
+
Review Comment:
`add_catch2_test()` currently infers the target from only the first element
of `COMMAND` and then calls `catch_discover_tests(${_catch2_target} ...)`
without preserving any additional command arguments. If any caller passes extra
runner args (beyond the executable), they will be silently dropped. Consider
capturing the remainder of `CATCH2_TEST_COMMAND` as extra args and forwarding
them via `catch_discover_tests(... EXTRA_ARGS ...)` (or introduce a dedicated
`ARGS` keyword and keep `COMMAND` strictly as the executable target).
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -185,12 +185,15 @@ struct RPCServerTestListener : Catch::EventListenerBase {
R"(", "backlog": 5,"max_retry_on_transient_errors": 64,
"incoming_request_max_size": 32000 }}})"};
YAML::Node configNode = YAML::Load(confStr);
serverConfig.load(configNode["rpc"]);
+ // Report this loudly: otherwise every socket test just fails later with a
+ // confusing "no such file" on the socket path, which is especially noisy
+ // now that each test case runs as its own process.
try {
jsonrpcServer = new rpc::RPCServer(serverConfig);
jsonrpcServer->start_thread();
} catch (std::exception const &ex) {
- Dbg(dbg_ctl, "Oops: %s", ex.what());
+ std::fprintf(stderr, "Failed to start the JSONRPC test server on %s:
%s\n", sockPath.c_str(), ex.what());
Review Comment:
If the server fails to start, the listener only prints an error and
continues, which will likely produce cascaded, harder-to-diagnose failures in
the actual test cases. Prefer to fail fast here (e.g., rethrow after logging,
or trigger a Catch2 failure in the listener) so the test process exits with a
clear root cause.
##########
src/config/unit_tests/test_storage.cc:
##########
@@ -37,12 +39,27 @@ using namespace config;
namespace
{
+// Test cases run as separate, concurrent ctest processes, so they cannot
share one
+// temp directory: a fixed file name would let one case delete another's file
mid-read.
+// A directory per process rather than a unique file name keeps the file names
these
+// tests depend on, e.g. a legacy storage.config finding its sibling
volume.config.
+std::filesystem::path const &
+per_process_temp_dir()
+{
+ static std::filesystem::path const dir = [] {
+ auto d = std::filesystem::temp_directory_path() / ("ats_config_test." +
std::to_string(getpid()));
+ std::filesystem::create_directories(d);
+ return d;
+ }();
Review Comment:
The per-process temp directory is created but (based on this diff) never
removed, which can accumulate stale directories under the system temp directory
across repeated test runs. Consider managing it with an RAII guard whose
destructor recursively removes the directory (or using a uniquely-named
directory per run and cleaning it up at process exit).
##########
src/config/unit_tests/test_storage.cc:
##########
@@ -37,12 +39,27 @@ using namespace config;
namespace
{
+// Test cases run as separate, concurrent ctest processes, so they cannot
share one
+// temp directory: a fixed file name would let one case delete another's file
mid-read.
+// A directory per process rather than a unique file name keeps the file names
these
+// tests depend on, e.g. a legacy storage.config finding its sibling
volume.config.
+std::filesystem::path const &
+per_process_temp_dir()
+{
+ static std::filesystem::path const dir = [] {
+ auto d = std::filesystem::temp_directory_path() / ("ats_config_test." +
std::to_string(getpid()));
+ std::filesystem::create_directories(d);
+ return d;
+ }();
+ return dir;
+}
Review Comment:
This `per_process_temp_dir()` helper is duplicated (with the same name and
implementation) across multiple test files in this PR. To avoid drift and
reduce copy/paste maintenance, consider extracting it into a small shared
unit-test helper header/source (or a test utility library) used by all affected
config tests.
##########
src/iocore/hostdb/unit_tests/CMakeLists.txt:
##########
@@ -35,4 +35,6 @@ target_include_directories(test_RefCountCache PRIVATE
"${CMAKE_CURRENT_SOURCE_DI
target_link_libraries(
test_RefCountCache PRIVATE ts::tscore ts::tsutil ts::inkevent configmanager
Catch2::Catch2WithMain
)
-add_catch2_test(NAME test_hostdb_RefCountCache COMMAND
$<TARGET_FILE:test_RefCountCache>)
+# test_RefCountCache has its own main() and is not a Catch2 runner, so it
cannot
+# be split into per-case ctest entries.
+add_test(NAME test_hostdb_RefCountCache COMMAND
$<TARGET_FILE:test_RefCountCache>)
Review Comment:
The comment says `test_RefCountCache` has its own `main()`, but the target
still links `Catch2::Catch2WithMain`, which also supplies a `main()` and can
cause duplicate-symbol link failures (or at least an unnecessary main object).
If it truly has a custom `main()`, link against `Catch2::Catch2` (without main)
instead; otherwise, update the comment to match reality.
##########
lib/CMakeLists.txt:
##########
@@ -43,14 +44,38 @@ if(BUILD_TESTING)
target_compile_options(Catch2 INTERFACE -Wno-error=parentheses)
target_compile_options(Catch2WithMain INTERFACE -Wno-error=parentheses)
- # By default, as of v3.9.0, Catch2 runs tests in random order. Our tests are
- # written with the expectation that they are run in declared order, with
setup
- # done in one TEST_CASE expected to carry over into the next TEST_CASE. This
- # macro allows us to add common command line arguments to all tests. For now,
- # we ensure declaration order execution via --order decl.
+ # Register one ctest test per Catch2 TEST_CASE/SCENARIO rather than one per
+ # executable. ctest then names the failing case instead of the binary, can
+ # schedule cases in parallel, and can run a single case by name. Because each
+ # case gets its own process, cases must not depend on state set up by an
+ # earlier case in the same executable.
+ #
+ # ctest names are "<NAME>.<case name>" so the NAME given here stays a usable
+ # -R filter prefix even where it differs from the target name.
+ #
+ # Discovery is PRE_TEST so that configuring/building does not require running
+ # the test executables.
macro(add_catch2_test)
- cmake_parse_arguments(CATCH2_TEST "" "NAME" "COMMAND" ${ARGN})
- add_test(NAME ${CATCH2_TEST_NAME} COMMAND ${CATCH2_TEST_COMMAND} --order
decl)
+ cmake_parse_arguments(CATCH2_TEST "" "NAME" "COMMAND;ENVIRONMENT" ${ARGN})
+
+ list(GET CATCH2_TEST_COMMAND 0 _catch2_command)
+ if(_catch2_command MATCHES "^\\$<TARGET_FILE:(.+)>$")
+ set(_catch2_target "${CMAKE_MATCH_1}")
+ else()
+ set(_catch2_target "${_catch2_command}")
+ endif()
+
+ if(CATCH2_TEST_ENVIRONMENT)
+ # Not ctest's ENVIRONMENT property: catch_discover_tests flattens
PROPERTIES
+ # into one ;-joined string, so a multi-variable ENVIRONMENT loses every
+ # variable but the first and turns the rest into junk property names. The
+ # emulator prefix survives intact and also applies to test discovery.
+ set_property(
+ TARGET ${_catch2_target} PROPERTY CROSSCOMPILING_EMULATOR
"${CMAKE_COMMAND}" -E env ${CATCH2_TEST_ENVIRONMENT}
+ )
+ endif()
+
+ catch_discover_tests(${_catch2_target} TEST_PREFIX "${CATCH2_TEST_NAME}."
DISCOVERY_MODE PRE_TEST)
Review Comment:
`add_catch2_test()` currently infers the target from only the first element
of `COMMAND` and then calls `catch_discover_tests(${_catch2_target} ...)`
without preserving any additional command arguments. If any caller passes extra
runner args (beyond the executable), they will be silently dropped. Consider
capturing the remainder of `CATCH2_TEST_COMMAND` as extra args and forwarding
them via `catch_discover_tests(... EXTRA_ARGS ...)` (or introduce a dedicated
`ARGS` keyword and keep `COMMAND` strictly as the executable target).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
