brbzull0 commented on code in PR #13649:
URL: https://github.com/apache/trafficserver/pull/13649#discussion_r3956806767
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -77,6 +77,45 @@ 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.
+ INFO("handler: " << _name);
+ CHECK(rpc::test_remove_handler(_name));
+ }
+ }
Review Comment:
Agreed, moved in 0c10ae00b to the anonymous namespace this file already has.
Checking the tree first, this is the convention rather than an exception:
139 of
141 unit test files under `src/**/unit_tests/` open no named namespace at
all, and
this is the only test file anywhere that reopens `namespace rpc`.
`namespace rpc` here now holds only the two functions with a reason to be in
it,
and `test_remove_handler` is the one that genuinely cannot move:
```cpp
// include/mgmt/rpc/jsonrpc/JsonRPCManager.h:160-161
protected: // For unit test.
bool remove_handler(std::string_view name);
friend bool test_remove_handler(std::string_view name);
```
`remove_handler` is protected and the friend declaration names
`rpc::test_remove_handler` specifically, so moving it out of the namespace
would
break the friendship and lose access.
`add_method_handler` is in the same position as the guard was -- a thin
wrapper
over a public call, no access requirement -- so it could move too. Left alone
here: it predates this change and moving it is unrelated to handler scoping.
Happy to do it separately if you would rather the namespace hold only the
friend.
--
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]
