From 8807f14e6c9831a9a400a83bdd346817620c4d49 Mon Sep 17 00:00:00 2001 From: "irons.du" Date: Fri, 18 Sep 2026 06:39:13 +0800 Subject: [PATCH 1/2] test: cover the web layer, which the sanitizer build only appeared to 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). --- .github/workflows/code-quality.yml | 11 +- CHANGELOG.md | 11 ++ CMakeLists.txt | 15 +++ CONTRIBUTING.md | 11 ++ scripts/check-sanitizers.sh | 7 +- tests/test_web_layer.cpp | 208 +++++++++++++++++++++++++++++ 6 files changed, 258 insertions(+), 5 deletions(-) create mode 100644 tests/test_web_layer.cpp diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index d514fcc..cac5719 100644 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -107,7 +107,12 @@ jobs: run: | ctest --output-on-failure --verbose env: - ASAN_OPTIONS: detect_leaks=1:halt_on_error=1 + # exitcode=86 distinguishes a leak from a failing assertion: LSan's default + # is 1, the same code a test returns when it fails, so the two are otherwise + # indistinguishable in the log. halt_on_error=1 matters just as much as + # detect_leaks=1 -- with detect_leaks alone the leak is reported but the + # process still exits 0, which CI reads as a pass. + ASAN_OPTIONS: detect_leaks=1:halt_on_error=1:exitcode=86 LSAN_OPTIONS: suppressions=../lsan.supp # Undefined Behavior Sanitizer @@ -359,7 +364,7 @@ jobs: run: | ctest -L fuzz --output-on-failure -E FuzzSmoke env: - ASAN_OPTIONS: detect_leaks=1:halt_on_error=1 + ASAN_OPTIONS: detect_leaks=1:halt_on_error=1:exitcode=86 LSAN_OPTIONS: suppressions=../lsan.supp - name: Fuzz smoke test @@ -367,7 +372,7 @@ jobs: run: | ctest -L fuzz --output-on-failure -R FuzzSmoke env: - ASAN_OPTIONS: detect_leaks=1:halt_on_error=1 + ASAN_OPTIONS: detect_leaks=1:halt_on_error=1:exitcode=86 LSAN_OPTIONS: suppressions=../lsan.supp # A crash leaves a reproducer behind; upload it so the failure is actionable diff --git a/CHANGELOG.md b/CHANGELOG.md index 90db617..a40e0d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- `tests/test_web_layer.cpp` covers the Drogon adapter and the embedded pages end to + end: it starts the real server, drives it over raw-socket HTTP and shuts it down. + Until now every test target was core-only, so `drogon_adapter.cpp` and + `web_resources.cpp` were compiled under ASan but executed by nothing that ctest + ran -- the sanitizer build only looked like it covered the web layer. The server is + started once per suite because `drogon::app()` is a bind-once singleton: a + per-test SetUp/TearDown segfaults in `ListenerManager::addListener` on the second + test. Port 0 plus `getListeners()` keeps parallel runs from colliding. +- The sanitizer steps set `ASAN_OPTIONS=...:exitcode=86`, so a leak is distinguishable + from a failed assertion in the log. LSan exits 1 by default, which is the same code + a test returns when an assertion fails. - libFuzzer targets for the parsers, behind `-DREMOTE_PROFILER_BUILD_FUZZERS=ON` (clang only, off by default): `fuzz_renderer_output`, `fuzz_result_parsing`, `fuzz_http_params`, with a committed seed corpus each. `ctest -L fuzz` replays the diff --git a/CMakeLists.txt b/CMakeLists.txt index 23dfa6a..62a6956 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -308,6 +308,21 @@ if(REMOTE_PROFILER_BUILD_TESTS) ) add_test(NAME ThreadStacksTest COMMAND test_thread_stacks) + # End-to-end web layer (Drogon adapter + embedded resources). Only built when + # the web layer is enabled, since it is the only test that links it. + if(REMOTE_PROFILER_ENABLE_WEB) + add_executable(test_web_layer tests/test_web_layer.cpp) + target_link_libraries(test_web_layer + PRIVATE + profiler_web + Drogon::Drogon + GTest::gtest + GTest::gtest_main + pthread + ) + add_test(NAME WebLayerTest COMMAND test_web_layer) + endif() + message(STATUS "Tests will be built") else() message(STATUS "Tests disabled") diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 20e19f7..e71604c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -314,6 +314,17 @@ build/fuzz/fuzz_renderer_output -max_total_time=300 tests/fuzz/corpus/fuzz_rende - **Sanitizer**：`ctest` 默认构建不会报内存泄漏与未定义行为。曾经有一个 `stopHeapProfiler()` 的 LeakSanitizer 报告只被 CI 发现，因为本地从没跑过 `-fsanitize=address`。 + + ⚠️ 想靠**退出码**判断有没有泄漏，两个参数都要给，只给第一个不够： + + | 设置 | 泄漏报告 | 退出码 | + |------|----------|--------| + | `detect_leaks=1`（`halt_on_error` 默认 0） | ✅ 打印 | **0** ← 泄漏了但 CI 视为通过 | + | `detect_leaks=1:halt_on_error=1` | ✅ 打印 | 非零 ✅ | + | 再加 `exitcode=86` | ✅ 打印 | **86** ← 与断言失败（1）可区分 | + + 另外两条限制：进程被 `_exit()`/信号杀死时 LSan 没机会运行；被抑制规则覆盖的泄漏 + （如 `lsan.supp` 里的 tcmalloc 初始化）不会让退出码非零。 - **Web UI**：`curl` **无法执行页面 JavaScript**，因此看不到"按钮点击报 ReferenceError""按钮卡在 disabled""下载其实没开始"这类问题。前端改动必须用 真实浏览器验证后才算完成。 diff --git a/scripts/check-sanitizers.sh b/scripts/check-sanitizers.sh index e5f7215..253fe9f 100755 --- a/scripts/check-sanitizers.sh +++ b/scripts/check-sanitizers.sh @@ -58,8 +58,11 @@ run_asan() { -DCMAKE_MODULE_LINKER_FLAGS="-fsanitize=address" \ "${extra_configure_args[@]}" cmake --build "$dir" -j"$JOBS" - # detect_leaks + halt_on_error: a leak must fail the run, as it does in CI. - ( cd "$dir" && ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 \ + # detect_leaks reports a leak; halt_on_error is what makes the process fail + # because of it (with detect_leaks alone the report is printed and the exit code + # stays 0). exitcode=86 then separates a leak from a failing assertion, since + # LSan's default 1 is the same code a test uses when an assertion fails. + ( cd "$dir" && ASAN_OPTIONS=detect_leaks=1:halt_on_error=1:exitcode=86 \ LSAN_OPTIONS="suppressions=$PROJECT_ROOT/lsan.supp" \ ctest --output-on-failure "${ctest_parallel[@]}" ) } diff --git a/tests/test_web_layer.cpp b/tests/test_web_layer.cpp new file mode 100644 index 0000000..ef5b1bc --- /dev/null +++ b/tests/test_web_layer.cpp @@ -0,0 +1,208 @@ +/// @file test_web_layer.cpp +/// @brief End-to-end test of the Drogon adapter and the embedded web resources +/// +/// Why this exists: every other test target is core-only (no Drogon), so +/// `drogon_adapter.cpp` and `web_resources.cpp` were compiled under ASan but never +/// executed by anything except the example binary, which ctest does not run. The +/// sanitizer build therefore looked like it covered the web layer while nothing +/// actually ran. This test starts the real server, drives it over HTTP and shuts it +/// down, so route registration, request delivery (including the async path through +/// AsyncExecutor and queueInLoop), the embedded pages and the shutdown sequence all +/// execute under the running sanitizer configuration. +/// +/// Requests are issued with a raw socket rather than libcurl, to avoid adding a +/// dependency purely for tests. A detail that makes the assertions meaningful: the +/// response writing happens on a *different* thread, so a response that arrives at +/// all proves the cross-thread hand-off worked, not merely that a handler ran. + +#include "../include/profiler/drogon_adapter.h" +#include "../include/profiler_manager.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +/// An HTTP response reduced to what the assertions need. +struct HttpResponse { + int status = 0; + std::string body; + bool received = false; +}; + +/// Send one request over a fresh connection and read the whole reply. +/// +/// Deliberately minimal: HTTP/1.1, `Connection: close`, so the reply ends when the +/// peer closes and no content-length parsing is needed. +HttpResponse request(uint16_t port, const std::string& method, const std::string& target, + const std::string& body = {}) { + HttpResponse result; + + const int fd = ::socket(AF_INET, SOCK_STREAM, 0); + if (fd < 0) + return result; + sockaddr_in addr{}; + addr.sin_family = AF_INET; + addr.sin_port = htons(port); + inet_pton(AF_INET, "127.0.0.1", &addr.sin_addr); + if (::connect(fd, reinterpret_cast(&addr), sizeof(addr)) != 0) { + ::close(fd); + return result; + } + + std::string req = method + " " + target + " HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n"; + if (!body.empty()) + req += "Content-Length: " + std::to_string(body.size()) + "\r\n"; + req += "\r\n" + body; + if (::write(fd, req.data(), req.size()) < 0) { + ::close(fd); + return result; + } + + std::string raw; + char buf[4096]; + ssize_t n = 0; + while ((n = ::read(fd, buf, sizeof(buf))) > 0) + raw.append(buf, static_cast(n)); + ::close(fd); + + const auto header_end = raw.find("\r\n\r\n"); + if (header_end == std::string::npos) + return result; + + result.received = true; + const auto status_line = raw.substr(0, raw.find("\r\n")); + const auto first_space = status_line.find(' '); + if (first_space != std::string::npos) + result.status = std::atoi(status_line.c_str() + first_space + 1); + result.body = raw.substr(header_end + 4); + return result; +} + +/// A Drogon server with the profiler routes, on an ephemeral port. +/// +/// The server is started once per *suite*, not once per test. `drogon::app()` is a +/// process-global singleton whose lifecycle is bind-once: calling addListener() a +/// second time, after a previous run() has finished, segfaults inside +/// ListenerManager. Structuring this as per-test SetUp/TearDown looked natural and +/// crashed on the second test. +/// +/// Port 0 lets the OS pick a free port and getListeners() reports which one, so +/// parallel ctest runs cannot collide. +class WebLayerTest : public ::testing::Test { +protected: + static void SetUpTestSuite() { + profiler_ = new profiler::ProfilerManager(); + profiler::registerDrogonHandlers(*profiler_); + + drogon::app().addListener("127.0.0.1", 0); + server_ = std::async(std::launch::async, []() { drogon::app().run(); }); + + // Wait for the listener to be bound rather than sleeping a fixed amount. + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); + while (std::chrono::steady_clock::now() < deadline) { + const auto listeners = drogon::app().getListeners(); + if (!listeners.empty()) { + port_ = listeners.front().toPort(); + break; + } + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + } + if (port_ == 0) + throw std::runtime_error("server never bound a port"); + } + + static void TearDownTestSuite() { + if (server_.valid()) { + drogon::app().getLoop()->queueInLoop([]() { drogon::app().quit(); }); + server_.wait(); + } + delete profiler_; + profiler_ = nullptr; + } + + static uint16_t port_; + +private: + static profiler::ProfilerManager* profiler_; + static std::future server_; +}; + +uint16_t WebLayerTest::port_ = 0; +profiler::ProfilerManager* WebLayerTest::profiler_ = nullptr; +std::future WebLayerTest::server_; + +} // namespace + +TEST_F(WebLayerTest, ServesTheEmbeddedControlPanel) { + // Exercises WebResources::getIndexPage(): the page is embedded as a C++ string + // literal, so nothing else checks that it is well-formed or that the route + // wiring returns it. + const auto resp = request(port_, "GET", "/"); + + ASSERT_TRUE(resp.received); + EXPECT_EQ(resp.status, 200); + EXPECT_NE(resp.body.find(""), std::string::npos); + // The panel must reference the current API surface, not a removed path. + EXPECT_NE(resp.body.find("/api/pprof/"), std::string::npos); +} + +TEST_F(WebLayerTest, ServesStatusAsJson) { + const auto resp = request(port_, "GET", "/api/status"); + + ASSERT_TRUE(resp.received); + EXPECT_EQ(resp.status, 200); + EXPECT_NE(resp.body.find("\"cpu\""), std::string::npos); + EXPECT_NE(resp.body.find("\"heap\""), std::string::npos); +} + +TEST_F(WebLayerTest, ServesThreadStacksThroughTheSignalPath) { + const auto resp = request(port_, "GET", "/api/thread/stacks"); + + ASSERT_TRUE(resp.received); + EXPECT_EQ(resp.status, 200); + // The capture machinery ran, which means the signal handler and the shared + // stack buffer were exercised on a request path. + EXPECT_NE(resp.body.find("Thread Call Stacks"), std::string::npos); +} + +TEST_F(WebLayerTest, ServesAAnalysisRequestThroughTheAsyncExecutor) { + // The analysis endpoints are the ones that go through AsyncExecutor and come + // back via queueInLoop, i.e. the response is written from another thread. That + // makes this the case most likely to leak or race, and growth is used because + // it needs neither TCMALLOC_SAMPLE_PARAMETER nor a sampling window. + const auto resp = request(port_, "GET", "/api/pprof/growth?renderer=flamegraph"); + + ASSERT_TRUE(resp.received); + // Either a rendered graph or a reported failure, but the reply itself proves the + // cross-thread hand-off completed. + EXPECT_TRUE(resp.status == 200 || resp.status == 500) << "status: " << resp.status; + if (resp.status == 200) + EXPECT_NE(resp.body.find(" Date: Fri, 18 Sep 2026 06:56:44 +0800 Subject: [PATCH 2/2] fix(tests): make the test suite build under -Wall -Wextra -Werror on 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. --- tests/test_full_flow.cpp | 20 +++++++++++++++----- tests/test_logger.cpp | 12 ++++++++---- tests/test_web_layer.cpp | 6 +++++- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/tests/test_full_flow.cpp b/tests/test_full_flow.cpp index a26452a..f57dce2 100644 --- a/tests/test_full_flow.cpp +++ b/tests/test_full_flow.cpp @@ -16,14 +16,20 @@ namespace { /// profile must therefore generate real CPU work for the whole window instead of /// sleeping through it -- otherwise they fail whenever the machine happens to be /// idle, which is exactly how this test failed under parallel ctest. -void burnCPUFor(int ms) { +uint64_t burnCPUFor(int ms) { const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(ms); - volatile uint64_t sink = 0; + uint64_t sink = 0; while (std::chrono::steady_clock::now() < deadline) { for (int i = 0; i < 20000; ++i) { sink += static_cast(i) * 2654435761u; } } + // Returning it rather than marking it unused: callers assert it is non-zero, so + // the accumulation is observable and the compiler cannot elide it. A `volatile` + // local would silence the optimizer but still trip -Wunused-but-set-variable + // (GCC sees through volatile for that warning, and the warnings-check job builds + // with -Wall -Wextra -Werror). + return sink; } } // namespace @@ -38,7 +44,7 @@ TEST(FullFlowTest, GperftoolsGeneratesValidProfile) { // 运行足够的工作负载 std::cout << "Running workload...\n"; - volatile long long result = 0; + long long result = 0; for (int i = 0; i < 10000; ++i) { result += i * i; // 一些函数调用 @@ -47,7 +53,11 @@ TEST(FullFlowTest, GperftoolsGeneratesValidProfile) { } // Keep sampling real work rather than idling: see burnCPUFor(). - burnCPUFor(500); + const uint64_t burned = burnCPUFor(500); + // Used, not merely set: without an observation the loop can be elided, and both + // this and `result` trip -Wunused-but-set-variable under -Werror. + EXPECT_GT(burned, 0u); + EXPECT_GT(result, 0); // 停止 profiler ProfilerStop(); @@ -189,7 +199,7 @@ TEST(FullFlowTest, GetRawCPUProfile) { // from executing threads, so a 1s window with nothing to sample returns an // empty profile by design. The workload used to be absent here, which made // the non-empty assertion below depend on machine load. - std::thread burner{[]() { burnCPUFor(1200); }}; + std::thread burner{[]() { EXPECT_GT(burnCPUFor(1200), 0u); }}; // 获取原始 profile 数据（采样 1 秒） std::string profile_data = profiler.getRawCPUProfile(1); diff --git a/tests/test_logger.cpp b/tests/test_logger.cpp index ed82b00..d190579 100644 --- a/tests/test_logger.cpp +++ b/tests/test_logger.cpp @@ -166,10 +166,14 @@ TEST_F(LoggerTest, ThreadSafety) { std::vector threads; for (int t = 0; t < num_threads; ++t) { - threads.emplace_back([this, t, messages_per_thread]() { - for (int i = 0; i < messages_per_thread; ++i) { - mock_sink_->log(LogLevel::Info, "thread_test.cpp", t * messages_per_thread + i, "threadFunc", - ("Thread " + std::to_string(t) + " message " + std::to_string(i)).c_str()); + // The capture is const-foldable, so it is not captured; and the loop + // variable is "msg" rather than "i" because an "i" parameter would shadow + // the captured messages_per_thread, leaving it unused inside the lambda + // (clang rejects that under -Wunused-lambda-capture with -Werror). + threads.emplace_back([this, t]() { + for (int msg = 0; msg < messages_per_thread; ++msg) { + mock_sink_->log(LogLevel::Info, "thread_test.cpp", t * messages_per_thread + msg, "threadFunc", + ("Thread " + std::to_string(t) + " message " + std::to_string(msg)).c_str()); } }); } diff --git a/tests/test_web_layer.cpp b/tests/test_web_layer.cpp index ef5b1bc..be09dd3 100644 --- a/tests/test_web_layer.cpp +++ b/tests/test_web_layer.cpp @@ -187,8 +187,12 @@ TEST_F(WebLayerTest, ServesAAnalysisRequestThroughTheAsyncExecutor) { // Either a rendered graph or a reported failure, but the reply itself proves the // cross-thread hand-off completed. EXPECT_TRUE(resp.status == 200 || resp.status == 500) << "status: " << resp.status; - if (resp.status == 200) + // Braces are required here, not stylistic: the EXPECT_TRUE above expands to an + // if/else, so an unbraced if would dangle and GCC rejects it under + // -Wall -Wextra -Werror (the warnings-check job). + if (resp.status == 200) { EXPECT_NE(resp.body.find("