Repository navigation
test: cover the web layer, which the sanitizer build only appeared to… - #46
Merged
Merged
Conversation
… cover Every test target was core-only. test_full_flow links profiler_web, but that is a conditional link in CMake and the test references nothing from it, so drogon_adapter.cpp and web_resources.cpp were compiled under ASan and executed by nothing ctest ran -- the only thing that ran them was profiler_example. A sanitizer build that includes a translation unit and never executes it reports no leaks from it, which reads as coverage. tests/test_web_layer.cpp starts the real server, drives it over HTTP and shuts it down, so route registration, the async delivery path, the embedded pages and the shutdown sequence all execute under whatever sanitizer configuration is active. A detail that gives the async assertions their meaning: the response is written from another thread, so a reply arriving at all proves the AsyncExecutor/queueInLoop hand-off completed rather than merely that a handler ran. Two things learned while writing it, both now in the file: - The server starts once per suite, not per test. `drogon::app()` is a process-global singleton whose lifecycle is bind-once; a per-test SetUp/TearDown segfaults inside ListenerManager::addListener on the second test. I wrote it that way first and hit exactly that. - Binding port 0 and reading the port back from getListeners() keeps parallel ctest runs from colliding, so no fixed port and no RESOURCE_LOCK. Requests use a raw socket instead of libcurl, to avoid a dependency that exists only for tests. Also sets `exitcode=86` in the sanitizer environment, in CI and in scripts/check-sanitizers.sh. This is a reading aid, not a behaviour change: LSan exits 1 by default, which is also what a test returns on a failed assertion, so the log currently cannot say which of the two happened. Worth recording why the flag is needed at all -- `detect_leaks=1` alone prints the report and still exits 0, which CI reads as a pass; `halt_on_error=1` is what turns a leak into a failing run. Verified: 9/9 serial and with ctest -j6 in the normal build, 9/9 under ASan with detect_leaks=1:halt_on_error=1:exitcode=86 and the project's suppressions, no leak reported for the new test, and the exit-code semantics confirmed against a probe with an injected leak (86 with the leak, 0 without).
…both compilers CI failed on the new web layer test: test_web_layer.cpp:190:8: error: suggest explicit braces to avoid ambiguous 'else' [-Werror=dangling-else] `EXPECT_TRUE(...) << ...` expands to an if/else, so an unbraced if right after it dangles. Braces added. I had not seen this locally because the -Werror configuration exists only in the warnings-check job, which builds with the default GCC and -Wall -Wextra -Werror, while my local builds carried no warning flags at all. Reproducing that job locally turned up two more failures in the same configuration: - test_full_flow.cpp: `sink` in burnCPUFor() and the older `result` in the gperftools test are "set but not used". volatile does not suppress that warning -- GCC sees through it. Both now feed an assertion (burnCPUFor() returns the accumulation and callers EXPECT_GT it), so the work is observable and nothing can elide the loop, rather than being silenced with a cast or an attribute. - test_logger.cpp: clang rejects an unused lambda capture, [-Wunused-lambda-capture]. It was not actually unused: the lambda parameter `i` shadowed the captured `messages_per_thread`, so the capture never got read inside the body. Fixed by naming the loop variable `msg` and dropping the capture, which removes the shadowing too. Pre-existing, and invisible to the GCC job that exists today -- I found it only by running the same flags through clang. Verified under all four configurations: GCC and clang with -Wall -Wextra -Werror (0 warnings, 9/9 tests each), the normal build serial and with ctest -j6, and ASan with the project's leak settings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… cover
Every test target was core-only. test_full_flow links profiler_web, but that is a conditional link in CMake and the test references nothing from it, so drogon_adapter.cpp and web_resources.cpp were compiled under ASan and executed by nothing ctest ran -- the only thing that ran them was profiler_example. A sanitizer build that includes a translation unit and never executes it reports no leaks from it, which reads as coverage.
tests/test_web_layer.cpp starts the real server, drives it over HTTP and shuts it down, so route registration, the async delivery path, the embedded pages and the shutdown sequence all execute under whatever sanitizer configuration is active. A detail that gives the async assertions their meaning: the response is written from another thread, so a reply arriving at all proves the AsyncExecutor/queueInLoop hand-off completed rather than merely that a handler ran.
Two things learned while writing it, both now in the file:
drogon::app()is a process-global singleton whose lifecycle is bind-once; a per-test SetUp/TearDown segfaults inside ListenerManager::addListener on the second test. I wrote it that way first and hit exactly that.Requests use a raw socket instead of libcurl, to avoid a dependency that exists only for tests.
Also sets
exitcode=86in the sanitizer environment, in CI and in scripts/check-sanitizers.sh. This is a reading aid, not a behaviour change: LSan exits 1 by default, which is also what a test returns on a failed assertion, so the log currently cannot say which of the two happened. Worth recording why the flag is needed at all --detect_leaks=1alone prints the report and still exits 0, which CI reads as a pass;halt_on_error=1is what turns a leak into a failing run.Verified: 9/9 serial and with ctest -j6 in the normal build, 9/9 under ASan with detect_leaks=1:halt_on_error=1:exitcode=86 and the project's suppressions, no leak reported for the new test, and the exit-code semantics confirmed against a probe with an injected leak (86 with the leak, 0 without).