fix(tests): fix unit test bugs and migrate coverage script to Qt6#125
Conversation
Reviewer's GuideUpdates the Qt test coverage script for Qt6 and safer ASAN/coverage behavior, adds robust gcov coverage flushing via signal handlers and atexit callbacks in the test main, and fixes two unit-test-related bugs (LogManager state leakage and an invalid delete of a DSingleton-managed BMInterface). File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The unconditional use of
__gcov_dump()inmain.cpp(including in the signal handler) will cause link errors or undefined behavior in non-coverage builds; consider wrapping all coverage-related declarations/registrations in a compile-time guard (e.g.#ifdef ENABLE_GCOVor a check on profiling flags) so normal builds are unaffected. - The crash signal handler currently calls
__gcov_dump()directly, which is not guaranteed to be async-signal-safe; if you rely on this in production-like runs, consider limiting this behavior to dedicated coverage builds or using a minimal handler that records a flag and flushes coverage on next safe point instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The unconditional use of `__gcov_dump()` in `main.cpp` (including in the signal handler) will cause link errors or undefined behavior in non-coverage builds; consider wrapping all coverage-related declarations/registrations in a compile-time guard (e.g. `#ifdef ENABLE_GCOV` or a check on profiling flags) so normal builds are unaffected.
- The crash signal handler currently calls `__gcov_dump()` directly, which is not guaranteed to be async-signal-safe; if you rely on this in production-like runs, consider limiting this behavior to dedicated coverage builds or using a minimal handler that records a flag and flushes coverage on next safe point instead.
## Individual Comments
### Comment 1
<location path="src/tests/main.cpp" line_range="13" />
<code_context>
#endif
+// 覆盖率数据刷新:确保程序退出(含异常退出)时 .gcda 文件被正确写入
+extern "C" void __gcov_dump();
+
+static void flushCoverage()
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard `__gcov_dump` usage so the test binary still links when coverage is not enabled
Unconditional declaration/calls to `__gcov_dump()` will fail to link when tests are built without gcov instrumentation (e.g. non-coverage CI or local debug builds). Please wrap the declaration and all uses in a feature macro (e.g. `#ifdef USE_GCOV_DUMP`, `#ifdef __GNUC__`, or a project-specific `#ifdef ENABLE_COVERAGE`) and enable it only in coverage builds so tests remain runnable in all configurations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| #endif | ||
|
|
||
| // 覆盖率数据刷新:确保程序退出(含异常退出)时 .gcda 文件被正确写入 | ||
| extern "C" void __gcov_dump(); |
There was a problem hiding this comment.
issue (bug_risk): Guard __gcov_dump usage so the test binary still links when coverage is not enabled
Unconditional declaration/calls to __gcov_dump() will fail to link when tests are built without gcov instrumentation (e.g. non-coverage CI or local debug builds). Please wrap the declaration and all uses in a feature macro (e.g. #ifdef USE_GCOV_DUMP, #ifdef __GNUC__, or a project-specific #ifdef ENABLE_COVERAGE) and enable it only in coverage builds so tests remain runnable in all configurations.
Fix double-free on DSingleton BMInterface, test state leak in LogManager, and add gcov dump with signal handler for coverage. Add __gcov_dump() with SIGSEGV/SIGABRT handlers and atexit callback to ensure .gcda files are written even on crash during teardown. 修复 BMInterface 单例误 delete 导致的 ASAN double-free, LogManager 测试间状态泄漏问题,以及覆盖率脚本适配 Qt6。 添加信号处理器确保覆盖率数据在崩溃时也能正确写入。 Log: 修复单元测试bug,适配Qt6覆盖率脚本 Influence: 单元测试在Qt6下19/19全部通过,覆盖率报告可正常生成(行覆盖36.8%)
4cdcedc to
71638d3
Compare
deepin pr auto review你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff,本次修改主要集中在单元测试的稳定性提升、覆盖率数据采集的可靠性优化以及构建脚本的现代化改造。 整体来看,这些改动非常实用,解决了很多C++单元测试和CI流程中的常见痛点(如单例误删、覆盖率丢失、ASAN冲突等)。但代码在安全性、逻辑严谨性和规范性上还有一些需要改进的地方。 以下是详细的审查意见: 1.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lzwind, pengfeixx The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/merge |
Fix double-free on DSingleton BMInterface, test state leak in LogManager, and add gcov dump with signal handler for coverage. Add __gcov_dump() with SIGSEGV/SIGABRT handlers and atexit callback to ensure .gcda files are written even on crash during teardown.
修复 BMInterface 单例误 delete 导致的 ASAN double-free,
LogManager 测试间状态泄漏问题,以及覆盖率脚本适配 Qt6。
添加信号处理器确保覆盖率数据在崩溃时也能正确写入。
Log: 修复单元测试bug,适配Qt6覆盖率脚本
Influence: 单元测试在Qt6下19/19全部通过,覆盖率报告可正常生成(行覆盖36.8%)
Please do not send pull requests to the linuxdeepin/*
see https://github.com/linuxdeepin/developer-center/wiki/Contribution-Guidelines-for-Developers
Thanks!
Summary by Sourcery
Update unit test infrastructure and coverage collection to be more robust and compatible with Qt6.
New Features:
Bug Fixes:
Enhancements:
Tests: