Copilot commented on code in PR #13649:
URL: https://github.com/apache/trafficserver/pull/13649#discussion_r3956034876
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -77,6 +77,44 @@ add_method_handler(const std::string &name, Func &&call)
{
return rpc::JsonRPCManager::instance().add_method_handler(name,
std::forward<Func>(call), nullptr, {});
}
+
+/// 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()
+ {
+ if (_registered) {
+ // CHECK rather than REQUIRE: this runs during stack unwinding when a
SECTION failed, and a
+ // fatal assertion there would abort instead of reporting.
+ CHECK(rpc::test_remove_handler(_name));
+ }
+ }
Review Comment:
Consider marking `~ScopedMethodHandler()` as `noexcept`. Destructors are
implicitly `noexcept` in modern C++ unless marked otherwise, but making it
explicit documents the intention and helps avoid accidental exception
propagation (which would call `std::terminate`, especially risky during stack
unwinding in a failing test).
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -77,6 +77,44 @@ add_method_handler(const std::string &name, Func &&call)
{
return rpc::JsonRPCManager::instance().add_method_handler(name,
std::forward<Func>(call), nullptr, {});
}
+
+/// 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()
+ {
+ if (_registered) {
+ // CHECK rather than REQUIRE: this runs during stack unwinding when a
SECTION failed, and a
+ // fatal assertion there would abort instead of reporting.
Review Comment:
If handler removal fails, the current `CHECK` provides limited context about
which handler leaked. Consider using `CHECK_MESSAGE(...)` (or `INFO` + `CHECK`)
to include `_name` in the failure output so the failing cleanup is immediately
identifiable.
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -77,6 +77,44 @@ add_method_handler(const std::string &name, Func &&call)
{
return rpc::JsonRPCManager::instance().add_method_handler(name,
std::forward<Func>(call), nullptr, {});
}
+
+/// 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()
+ {
+ if (_registered) {
+ // CHECK rather than REQUIRE: this runs during stack unwinding when a
SECTION failed, and a
+ // fatal assertion there would abort instead of reporting.
+ CHECK(rpc::test_remove_handler(_name));
+ }
+ }
+
+ ScopedMethodHandler(ScopedMethodHandler const &) = delete;
+ ScopedMethodHandler &operator=(ScopedMethodHandler const &) = delete;
+
+ bool
+ registered() const
+ {
+ return _registered;
+ }
Review Comment:
Consider adding `[[nodiscard]]` to `registered()` to make it harder to
accidentally ignore the registration result in future tests (since correct
usage relies on calling `REQUIRE(handler.registered())`).
--
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]
