Copilot commented on code in PR #13658:
URL: https://github.com/apache/trafficserver/pull/13658#discussion_r3973510014
##########
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:
This implements per-test environment by mutating the target’s
`CROSSCOMPILING_EMULATOR` property, which is a target-global setting. If
`add_catch2_test()` is invoked more than once for the same target with
different `ENVIRONMENT` values, the last call will win and can unintentionally
affect other discovered tests/uses of that target. A more isolated approach
would be to avoid mutating the target property globally (e.g., create a
dedicated wrapper/launcher for discovery+execution per test target, or
otherwise ensure the env prefix is applied without making it a shared
target-wide side effect).
##########
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 listener swallows the exception after printing, which can leave the rest
of the tests running in a broken state (and still failing later, just
differently). Consider failing fast here (e.g., rethrow after logging, or use a
Catch2 failure mechanism) so the test run stops with the root cause.
##########
src/config/unit_tests/config_test_temp_file.h:
##########
@@ -0,0 +1,99 @@
+/** @file
+
+ Shared helpers for the config unit tests.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+#pragma once
+
+#include <filesystem>
+#include <fstream>
+#include <string>
+
+#include <unistd.h>
+
+namespace config::testing
+{
+
+/** Temp directory owned by this process, removed when the process exits.
+ *
+ * Each test case runs as its own concurrent ctest process, so a directory
+ * shared between them 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.
+ */
+inline std::filesystem::path const &
+per_process_temp_dir()
+{
+ class ScopedDir
+ {
+ public:
+ ScopedDir()
+ {
+ _path = std::filesystem::temp_directory_path() / ("ats_config_test." +
std::to_string(getpid()));
+ std::filesystem::create_directories(_path);
+ }
+
+ ~ScopedDir()
+ {
+ std::error_code ec; // Best effort: a leftover temp directory must not
fail the run.
+ std::filesystem::remove_all(_path, ec);
+ }
+
+ std::filesystem::path const &
+ path() const
+ {
+ return _path;
+ }
+
+ private:
+ std::filesystem::path _path;
+ };
+
+ static ScopedDir const dir;
+
+ return dir.path();
+}
+
+/// A file in per_process_temp_dir(), removed when it goes out of scope.
+class TempFile
+{
+public:
+ TempFile(std::string const &filename, std::string const &content)
+ {
+ _path = per_process_temp_dir() / filename;
+ std::ofstream ofs(_path);
+ ofs << content;
+ }
+
+ ~TempFile() { std::filesystem::remove(_path); }
Review Comment:
`std::filesystem::remove` can throw; throwing from a destructor can
terminate the process (especially during stack unwinding). Consider switching
to the `error_code` overload in the destructor (best-effort cleanup, like
`ScopedDir` already does) so test failures don’t become hard aborts due to
cleanup.
##########
src/proxy/http/remap/unit-tests/CMakeLists.txt:
##########
@@ -94,7 +94,16 @@ if(NOT APPLE)
endif()
if(NOT APPLE)
- add_catch2_test(NAME test_PluginDso COMMAND $<TARGET_FILE:test_PluginDso>)
+ add_catch2_test(
+ NAME
+ test_PluginDso
+ COMMAND
+ $<TARGET_FILE:test_PluginDso>
+ # Disable ODR violation caused by double definition inside a stub file
libinknet_stub.cc
+ # see remap_test_dlopen_leak_suppression.txt for more info.
+ ENVIRONMENT
+
"ASAN_OPTIONS=detect_odr_violation=0;LSAN_OPTIONS=suppressions=${CMAKE_CURRENT_SOURCE_DIR}/remap_test_dlopen_leak_suppression.txt"
Review Comment:
The same ASan/LSan `ENVIRONMENT` string is duplicated across multiple
`add_catch2_test()` calls in this file. To reduce the risk of future
divergence, consider factoring it into a single CMake variable (or helper) and
reusing it for `test_PluginDso`, `test_PluginFactory`, and
`test_RemapPluginInfo`.
##########
src/iocore/net/CMakeLists.txt:
##########
@@ -222,12 +222,12 @@ if(BUILD_TESTING)
endif()
set(LIBINKNET_UNIT_TEST_DIR "${CMAKE_SOURCE_DIR}/src/iocore/net/unit_tests")
target_compile_definitions(test_net PRIVATE
LIBINKNET_UNIT_TEST_DIR=${LIBINKNET_UNIT_TEST_DIR})
- add_catch2_test(NAME test_net COMMAND test_net)
-
- if(NOT APPLE)
+ if(APPLE)
+ add_catch2_test(NAME test_net COMMAND test_net)
+ else()
# Disable ORD violation caused by double definition inside a stub file
libinknet_stub.cc
Review Comment:
Typo in comment: 'ORD violation' should be 'ODR violation' (One Definition
Rule).
--
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]
