Copilot commented on code in PR #13649:
URL: https://github.com/apache/trafficserver/pull/13649#discussion_r3957505927
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -81,6 +81,54 @@ 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> explicit 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;
+ }
+ // Non-fatal assertions throughout: this runs during stack unwinding when
a SECTION failed,
+ // and a fatal one 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
+ // a destructor ends the whole test binary rather than the one assertion.
+ try {
+ INFO("handler: " << _name);
+ CHECK(rpc::test_remove_handler(_name));
+ } catch (std::exception const &ex) {
+ FAIL_CHECK("exception while removing handler '" << _name << "': " <<
ex.what());
+ } catch (...) {
+ FAIL_CHECK("unknown exception while removing handler '" << _name << "'");
+ }
+ }
+
+ ScopedMethodHandler(ScopedMethodHandler const &) = delete;
+ ScopedMethodHandler &operator=(ScopedMethodHandler const &) = delete;
+
+ [[nodiscard]] bool
+ registered() const
Review Comment:
`registered()` is a trivial accessor and can be marked `noexcept` to better
reflect its behavior and strengthen exception guarantees (especially since it’s
used in assertions). Suggested change: `registered() const noexcept`.
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -424,8 +472,10 @@ TEST_CASE("Sending 'concurrent' requests to the rpc
server.", "[thread]")
{
SECTION("A registered handlers")
Review Comment:
The SECTION title is grammatically incorrect; consider renaming it for
clarity.
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -81,6 +81,54 @@ 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> explicit ScopedMethodHandler(std::string name, Func
&&call) : _name{std::move(name)}
+ {
+ _registered = rpc::add_method_handler(_name, std::forward<Func>(call));
Review Comment:
Registration success currently relies on each call site remembering to
assert `registered()`. To reduce repetition and prevent future omissions,
consider making `ScopedMethodHandler` enforce success at construction (e.g., by
performing the assertion internally) or providing a factory/helper that both
constructs and asserts. This keeps the guard harder to misuse in new tests.
--
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]
