brbzull0 commented on code in PR #13649:
URL: https://github.com/apache/trafficserver/pull/13649#discussion_r3957371933
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -81,6 +82,53 @@ 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> 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;
+ }
+ // CHECK rather than REQUIRE: this runs during stack unwinding when a
SECTION failed, and a
+ // fatal assertion there 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 here would take the whole test binary down with it.
+ try {
+ INFO("handler: " << _name);
+ CHECK(rpc::test_remove_handler(_name));
+ } catch (...) {
+ // CHECK is unavailable here: it may be what threw.
+ std::cerr << "ScopedMethodHandler: exception while removing '" << _name
<< "'\n";
+ }
+ }
Review Comment:
Good catch, and this was a real hole rather than a style point -- fixed in
6c9e1399a.
The `std::cerr`-only catch I added last round made a failed cleanup report
as a
**pass**. Demonstrated by forcing a throw inside the guarded region:
```
ScopedMethodHandler: exception while removing 'do_nothing'
===============================================================================
All tests passed (4 assertions in 1 test case)
```
That defeats the point of the PR. `FAIL_CHECK` exists in the vendored Catch2
3.9.1
(`lib/catch2/src/catch2/catch_test_macros.hpp:162`, non-fatal via
`ContinueOnFailure`) and behaves correctly in a destructor during unwinding:
```
test_rpcserver.cc:114: FAILED:
explicitly with message:
exception while removing handler 'do_nothing': forced
test cases: 1 | 1 failed
```
Also took the suggestion to catch `std::exception` separately so the message
surfaces, with a `catch (...)` behind it.
My earlier reasoning for `std::cerr` -- that `CHECK` might be what threw --
was
speculative. The realistic throw is `std::mutex::lock()` inside
`test_remove_handler`, not the assertion machinery.
On the `-x`/`--abort` concern implied here: `Config::abortAfter()` defaults
to `-1`
stored into a `size_t` (`catch_config.hpp:62`), so `aborting()` compares
against
`SIZE_MAX` and is permanently false, and `add_catch2_test`
(`lib/CMakeLists.txt:51-54`) appends only `--order decl`. Even under `-x`,
`assertionEnded()` runs before `populateReaction`, so a genuine failure is
still
reported in full.
--
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]
