Copilot commented on code in PR #13649:
URL: https://github.com/apache/trafficserver/pull/13649#discussion_r3956953623
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -81,6 +82,53 @@ add_method_handler(const std::string &name, Func &&call)
namespace
{
+/// Registers a method handler and removes it when the scope ends.
+///
+/// Catch2 re-runs a TEST_CASE body once per leaf SECTION. Registering at the
top of the body and
+/// removing at the bottom only works while every assertion passes: REQUIRE is
fatal, so a failure
+/// inside a SECTION unwinds before the trailing removal and the handler
survives into the next
+/// SECTION's run, where re-registering it fails. One real failure then
reports as two, and the
+/// second points at a registration that was never the problem.
+class ScopedMethodHandler
+{
+public:
+ template <typename Func> ScopedMethodHandler(std::string name, Func &&call)
: _name{std::move(name)}
Review Comment:
Consider making this constructor `explicit` to avoid accidental implicit
conversions (e.g., from a single `std::string` argument in future call sites).
This is a small guardrail for a utility type intended to be constructed
deliberately at a specific scope.
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -81,6 +82,53 @@ add_method_handler(const std::string &name, Func &&call)
namespace
{
+/// Registers a method handler and removes it when the scope ends.
+///
+/// Catch2 re-runs a TEST_CASE body once per leaf SECTION. Registering at the
top of the body and
+/// removing at the bottom only works while every assertion passes: REQUIRE is
fatal, so a failure
+/// inside a SECTION unwinds before the trailing removal and the handler
survives into the next
+/// SECTION's run, where re-registering it fails. One real failure then
reports as two, and the
+/// second points at a registration that was never the problem.
+class ScopedMethodHandler
+{
+public:
+ template <typename Func> ScopedMethodHandler(std::string name, Func &&call)
: _name{std::move(name)}
+ {
+ _registered = rpc::add_method_handler(_name, std::forward<Func>(call));
+ }
+
+ ~ScopedMethodHandler() noexcept
+ {
+ if (!_registered) {
+ return;
+ }
+ // CHECK rather than REQUIRE: this runs during stack unwinding when a
SECTION failed, and a
+ // fatal assertion there would abort instead of reporting. The try/catch
is for the same
+ // reason -- taking the dispatcher lock or building the diagnostic can
throw, and an
+ // exception escaping here would take the whole test binary down with it.
+ try {
+ INFO("handler: " << _name);
+ CHECK(rpc::test_remove_handler(_name));
+ } catch (...) {
+ // CHECK is unavailable here: it may be what threw.
+ std::cerr << "ScopedMethodHandler: exception while removing '" << _name
<< "'\n";
+ }
+ }
Review Comment:
The catch-all path only writes to `std::cerr`, so a cleanup failure can be
missed by the test framework (and potentially leave state behind for later
tests) while still appearing as a pass. Consider reporting this via a non-fatal
Catch2 assertion in the `catch` (e.g., `FAIL_CHECK`/`WARN` with the handler
name, and optionally the exception message when catching `std::exception`) so
the failure is visible in test results while still avoiding termination during
unwinding.
--
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]
