Copilot commented on code in PR #13658:
URL: https://github.com/apache/trafficserver/pull/13658#discussion_r3973401674
##########
src/iocore/cache/unit_tests/test_CacheShm.cc:
##########
@@ -327,7 +334,7 @@ segment_size(const std::string &name)
// the name space, or every stripe segment leaks while `traffic_ctl cache shm
clear` reports success.
TEST_CASE("CacheShm purge sweeps by name when the control layout is foreign",
"[cache][shm]")
{
- const std::string prefix =
cache_shm::normalize_name_prefix(PURGE_PREFIX_WORD);
+ const std::string prefix =
cache_shm::normalize_name_prefix(purge_prefix_word());
Review Comment:
Same SHM-name-length concern as `test_CacheShmShutdown.cc`: adding the PID
increases the prefix length and may cause the derived SHM names to exceed
platform limits. It would be safer to keep uniqueness while enforcing a strict
maximum prefix length (e.g., fixed-width PID encoding or a short hash suffix)
to ensure `shm_open` cannot fail due to name length.
##########
src/iocore/cache/unit_tests/test_CacheShm.cc:
##########
@@ -246,7 +246,14 @@ namespace
{
// A prefix of our own so these tests can never touch a real instance's
segments.
-constexpr const char *PURGE_PREFIX_WORD = "atspurgetest";
+// Per process: POSIX shm names are system global and every TEST_CASE runs as
its own
+// ctest process, so a fixed word would let concurrent cases fight over one
segment.
+std::string const &
+purge_prefix_word()
+{
+ static std::string const word{"atspurgetest" + std::to_string(getpid())};
+ return word;
+}
Review Comment:
Same SHM-name-length concern as `test_CacheShmShutdown.cc`: adding the PID
increases the prefix length and may cause the derived SHM names to exceed
platform limits. It would be safer to keep uniqueness while enforcing a strict
maximum prefix length (e.g., fixed-width PID encoding or a short hash suffix)
to ensure `shm_open` cannot fail due to name length.
##########
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:
The exception is logged but then swallowed, which means the test process
will continue running even though the JSONRPC server failed to start. That
usually leads to confusing downstream failures (or potential hangs), and it
also risks leaking `jsonrpcServer` if `start_thread()` throws after allocation.
Prefer failing fast here (e.g., convert this into a Catch2 failure/abort for
the current process) and ensure the allocated server is cleaned up on the
exceptional path (e.g., use RAII/`std::unique_ptr` until startup succeeds).
##########
src/iocore/cache/unit_tests/test_CacheShmShutdown.cc:
##########
@@ -48,14 +48,22 @@ namespace
{
// Our own prefix so these can never touch a real instance's segments, short
enough to stay under the 31-char POSIX limit.
-constexpr const char *TEST_PREFIX_WORD = "atsunittest";
+// Per process: POSIX shm names are system global and every TEST_CASE runs as
its own
+// ctest process, so a fixed word would let concurrent cases fight over one
segment.
+std::string const &
+test_prefix_word()
+{
+ static std::string const word{"atsunittest" + std::to_string(getpid())};
+ return word;
+}
// False when shm is unavailable here, e.g. a sandbox forbidding shm_open, so
the test skips rather than fails.
bool
enable_shm()
{
REQUIRE(RecSetRecordInt("proxy.config.cache.shm.enabled", 1,
REC_SOURCE_EXPLICIT) == REC_ERR_OKAY);
- REQUIRE(RecSetRecordString("proxy.config.cache.shm.name_prefix",
TEST_PREFIX_WORD, REC_SOURCE_EXPLICIT) == REC_ERR_OKAY);
+ REQUIRE(RecSetRecordString("proxy.config.cache.shm.name_prefix",
test_prefix_word().c_str(), REC_SOURCE_EXPLICIT) ==
+ REC_ERR_OKAY);
Review Comment:
These tests previously called out a strict 31-character POSIX SHM name
limit; appending the PID increases the prefix length and can push the derived
SHM object names over the platform limit (especially if the final name adds
additional fixed suffixes). If the total name exceeds the limit, `shm_open`
will fail with `ENAMETOOLONG` and the test will become
environment-dependent/flaky. Consider keeping the uniqueness but bounding the
prefix length (e.g., truncate/encode the PID to a fixed small width, or hash
it) so the final derived SHM names stay within the documented maximum.
##########
src/iocore/cache/unit_tests/test_CacheShmShutdown.cc:
##########
@@ -48,14 +48,22 @@ namespace
{
// Our own prefix so these can never touch a real instance's segments, short
enough to stay under the 31-char POSIX limit.
-constexpr const char *TEST_PREFIX_WORD = "atsunittest";
+// Per process: POSIX shm names are system global and every TEST_CASE runs as
its own
+// ctest process, so a fixed word would let concurrent cases fight over one
segment.
+std::string const &
+test_prefix_word()
+{
+ static std::string const word{"atsunittest" + std::to_string(getpid())};
+ return word;
+}
Review Comment:
These tests previously called out a strict 31-character POSIX SHM name
limit; appending the PID increases the prefix length and can push the derived
SHM object names over the platform limit (especially if the final name adds
additional fixed suffixes). If the total name exceeds the limit, `shm_open`
will fail with `ENAMETOOLONG` and the test will become
environment-dependent/flaky. Consider keeping the uniqueness but bounding the
prefix length (e.g., truncate/encode the PID to a fixed small width, or hash
it) so the final derived SHM names stay within the documented maximum.
##########
lib/CMakeLists.txt:
##########
@@ -43,14 +44,53 @@ 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)
+ list(LENGTH CATCH2_TEST_COMMAND _catch2_command_length)
+ if(NOT _catch2_command_length EQUAL 1)
+ # Per-case discovery appends the case name to the command line, so there
is
+ # nowhere to put caller-supplied runner arguments. Fail rather than drop
them.
+ message(FATAL_ERROR "add_catch2_test(${CATCH2_TEST_NAME}): COMMAND must
be a single test executable")
+ endif()
+ 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.
+ #
+ # Prepend rather than replace: a cross-compiling build may already have
an
+ # emulator here, and it has to keep running the executable.
+ get_property(
+ _catch2_emulator
+ TARGET ${_catch2_target}
+ PROPERTY CROSSCOMPILING_EMULATOR
+ )
+ set_property(
+ TARGET ${_catch2_target} PROPERTY CROSSCOMPILING_EMULATOR
"${CMAKE_COMMAND}" -E env ${CATCH2_TEST_ENVIRONMENT}
+ ${_catch2_emulator}
+ )
+ endif()
+
+ catch_discover_tests(${_catch2_target} TEST_PREFIX "${CATCH2_TEST_NAME}."
DISCOVERY_MODE PRE_TEST)
Review Comment:
Using the target’s `CROSSCOMPILING_EMULATOR` property to inject `cmake -E
env ...` is likely ineffective for native (non-cross-compiled) builds, because
typical `catch_discover_tests` implementations only apply
`CROSSCOMPILING_EMULATOR` when `CMAKE_CROSSCOMPILING` is true. That would cause
`ENVIRONMENT` to be silently ignored on the common/native path, and ASan/LSan
knobs may not take effect. A more reliable approach is to apply the environment
via mechanisms that are honored unconditionally for the generated ctest entries
(e.g., generate a small launcher wrapper script/executable and discover/run
that, or adjust the discovery macro to prefix the command regardless of
cross-compiling state).
--
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]
