test_rpcserver: scope handler registration to the test case - #13649
test_rpcserver: scope handler registration to the test case#13649brbzull0 wants to merge 5 commits into
Conversation
A failing assertion inside a SECTION reports twice. Catch2 re-runs the TEST_CASE body once per SECTION, and the trailing test_remove_handler() never runs when a fatal REQUIRE unwinds first, so the handler survives into the next run and its add_method_handler() fails as well. The second failure points at a registration that was never the problem. Tie registration to a scope guard instead, so the handler is removed on the way out however the body exits. The concurrent-request case never removed its two handlers at all, and never checked that registering them worked; it uses the guard now as well.
There was a problem hiding this comment.
🟡 Changes recommended
The new RAII cleanup path currently drops the previous explicit assertion that handler removal succeeds, which can hide cleanup failures and reintroduce state leakage/flakiness across sections/tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the JSONRPC server unit tests to avoid misleading double-failures caused by Catch2 re-running TEST_CASE bodies per SECTION and fatal REQUIRE aborting before manual handler cleanup runs. It introduces a scoped RAII helper to ensure method handlers are unregistered reliably on scope exit (including during stack unwinding).
Changes:
- Added
rpc::ScopedMethodHandlerto register a JSONRPC method handler and automatically unregister it when leaving scope. - Updated multiple
TEST_CASEs to useScopedMethodHandler(including the concurrent-request test) and to assert registration succeeded.
File summaries
| File | Description |
|---|---|
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc |
Adds a scope-guard for method handler registration and updates tests to use it, improving cleanup behavior across Catch2 SECTION runs. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses review feedback. The guard's destructor discarded the result of test_remove_handler(), so a handler that went missing before scope exit would leave no trace. CHECK rather than REQUIRE because the destructor runs during unwinding when a SECTION failed, where a fatal assertion would abort instead of report.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Addresses review feedback. A failed removal reported only the expression, not which handler it was; INFO carries the name into the failure output. Mark registered() [[nodiscard]] so a caller cannot drop the registration result the guard's contract depends on.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Addresses review feedback. ScopedMethodHandler depends on Catch2 macros and has no reason to sit in the production rpc namespace, where it could collide with real RPC code. Only test_remove_handler needs to be there, because JsonRPCManager names it as a friend to reach a protected member. Guard the destructor as well. Taking the dispatcher lock or building the diagnostic can throw, and an exception leaving a destructor ends the whole test binary rather than the one assertion.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
The catch-all added for the guarded destructor only wrote to std::cerr, so an exception during cleanup left the test reporting a pass. FAIL_CHECK is non-fatal, so it records the failure without aborting during unwinding, and catching std::exception separately carries the message. Also make the constructor explicit; the guard is meant to be built deliberately at a scope.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
A failing assertion inside a SECTION reports twice. Catch2 re-runs the TEST_CASE body once per SECTION, and the trailing test_remove_handler() never runs when a fatal REQUIRE unwinds first, so the handler survives into the next run and its add_method_handler() fails as well. The second failure points at a registration that was never the problem.
Tie registration to a scope guard instead, so the handler is removed on the way out however the body exits. The concurrent-request case never removed its two handlers at all, and never checked that registering them worked; it uses the guard now as well.