Copilot commented on code in PR #13649:
URL: https://github.com/apache/trafficserver/pull/13649#discussion_r3955648458
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -77,6 +77,42 @@ 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));
+ }
Review Comment:
Template declarations in this file are consistently split onto their own
line (e.g., add_method_handler() and chunk_impl()). For consistency and
readability, consider formatting the ScopedMethodHandler constructor template
the same way.
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -77,6 +77,42 @@ 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) {
+ rpc::test_remove_handler(_name);
+ }
+ }
Review Comment:
ScopedMethodHandler::~ScopedMethodHandler() calls test_remove_handler() but
ignores its return value, which can hide cleanup failures and allow leftover
handlers to leak into later SECTION runs or other TEST_CASEs. Since we can’t
use REQUIRE in a destructor, consider using CHECK so failures are still
reported without throwing during stack 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]
