Skip to content

fix(tests): fix unit test bugs and migrate coverage script to Qt6#125

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
pengfeixx:feat/migrate-tests-to-qt6
Jun 4, 2026
Merged

fix(tests): fix unit test bugs and migrate coverage script to Qt6#125
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
pengfeixx:feat/migrate-tests-to-qt6

Conversation

@pengfeixx

@pengfeixx pengfeixx commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

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:

  • Add crash signal handlers and exit callbacks to flush gcov coverage data on abnormal and normal test exits.

Bug Fixes:

  • Prevent double-free of the BMInterface singleton in unit test teardown.
  • Reset LogManager system log state in tests to avoid leaking configuration between test cases.

Enhancements:

  • Adjust the test coverage script to auto-detect qmake6/qmake, build in release mode for coverage, and improve ASAN log handling.

Tests:

  • Ensure test runner main program explicitly flushes coverage data at shutdown to avoid losing .gcda files.

@sourcery-ai

sourcery-ai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Reviewer's Guide

Updates 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

Change Details Files
Adapt the coverage shell script to Qt6/qmake6 and make the build/coverage collection more robust and ASAN-friendly.
  • Auto-detect qmake6 vs qmake and echo the selected tool and its version.
  • Change directory handling to derive SCRIPT_DIR from the test script location and perform all operations from there.
  • Recreate the build directory under src/tests, using qmake in release mode on tests.pro for coverage builds to avoid ASAN teardown crashes that lose .gcda files.
  • Improve ASAN log handling by conditionally moving asan_loader.log* to asan_dde-boot-maker.log or creating an empty log file if none exist.
  • Run lcov/genhtml from the new build layout to generate the coverage report while excluding tests and system headers.
src/tests/test-recoverage.sh
Ensure gcov coverage data (.gcda) is flushed even on abnormal test process termination.
  • Declare __gcov_dump() and add a flushCoverage helper that calls it.
  • Introduce crashHandler that calls __gcov_dump() and then exits with a signal-based status code.
  • Register signal handlers for SIGSEGV, SIGABRT, and SIGBUS that use crashHandler at process startup.
  • Register an atexit callback to flush coverage data on normal exits.
  • Call __gcov_dump() explicitly after RUN_ALL_TESTS completes, while preserving existing ASAN report path configuration in debug builds.
src/tests/main.cpp
Fix unit test state leakage in LogManager tests by restoring global LogManager state.
  • After enabling system log in setSystemlog test, immediately reset system log back to false so subsequent tests run with default paths and permissions.
  • Document that systemLog=true switches log path to /var/log/deepin/ where regular users lack write permission, motivating the reset.
src/tests/logmanagertest.cpp
Fix BMInterface lifetime management in tests to avoid double-free of a DSingleton-managed object.
  • Stop deleting the BMInterface pointer in UnitTestObj::TearDownTestCase and instead just null out the cached pointer.
  • Document that BMInterface::ref() returns a DSingleton-managed instance whose lifecycle is handled by DSingleton, so manual delete is invalid.
src/tests/unittestobj.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/tests/main.cpp
#endif

// 覆盖率数据刷新:确保程序退出(含异常退出)时 .gcda 文件被正确写入
extern "C" void __gcov_dump();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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%)
@pengfeixx pengfeixx force-pushed the feat/migrate-tests-to-qt6 branch from 4cdcedc to 71638d3 Compare June 4, 2026 08:46
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff,本次修改主要集中在单元测试的稳定性提升覆盖率数据采集的可靠性优化以及构建脚本的现代化改造

整体来看,这些改动非常实用,解决了很多C++单元测试和CI流程中的常见痛点(如单例误删、覆盖率丢失、ASAN冲突等)。但代码在安全性、逻辑严谨性和规范性上还有一些需要改进的地方。

以下是详细的审查意见:


1. src/tests/main.cpp - 覆盖率与信号处理(重点审查)

语法与逻辑

  • __gcov_dump()__gcov_flush() 的区别:代码中使用了 __gcov_dump()。需要注意的是,__gcov_dump() 会将覆盖率数据写入文件但不会重置计数器,而 __gcov_flush() 会写入并重置。在 atexit 和正常退出流程中调用 __gcov_dump() 是没问题的,但如果你后续有动态卸载模块或多次刷新的需求,需注意两者的区别。
  • 信号处理函数的异步安全性crashHandler 中调用了 __gcov_dump()。虽然 __gcov_dump() 通常是安全的,但在 POSIX 标准中,信号处理函数中只能调用异步信号安全的函数。文件 I/O 操作通常不在安全列表中。如果 __gcov_dump() 内部进行了非安全系统调用,可能会导致死锁。不过,在崩溃处理中为了挽救覆盖率数据,这是一种可以接受的折中方案,但建议加上注释说明此风险。
  • 重复注册 atexitatexit(flushCoverage) 和最后主动调用 __gcov_dump() 会导致程序正常退出时 __gcov_dump() 被调用两次。虽然 __gcov_dump() 重复调用通常不会导致严重错误(只是多写一次文件),但这在逻辑上是冗余的。

代码性能

  • 正常退出时双重刷新会带来微小的性能损耗,建议移除其中一处。

代码安全

  • _exit() vs abort():在 crashHandler 中使用 _exit(128 + sig) 是正确的,因为 exit() 会触发 atexit 回调,在信号崩溃上下文中执行 atexit 可能导致无限递归或死锁。但请注意,_exit() 不会刷新标准输出缓冲区,如果此时有未写出的日志,将会丢失。这是崩溃处理的常态,但需留意。

💡 改进建议:

// 移除 atexit 注册,因为 main 函数正常返回时也会调用 __gcov_dump()
// 或者保留 atexit,移除 main 函数末尾的 __gcov_dump()。推荐后者,利用 RAII/atexit 更优雅。

int main(int argc, char *argv[])
{
    // 注册信号处理器,确保崩溃时覆盖率数据不丢失
    // 注意:在信号处理函数中调用 __gcov_dump() 并非绝对异步信号安全,
    // 但为了挽救覆盖率数据,这是可接受的折中方案。
    signal(SIGSEGV, crashHandler);
    signal(SIGABRT, crashHandler);
    signal(SIGBUS, crashHandler);

    // 注册 atexit 回调,在程序正常退出时刷新覆盖率数据
    atexit(flushCoverage);

    QCoreApplication a(argc, argv);
    a.setOrganizationName("deepin");
    a.setApplicationName("deepin-boot-maker");

    ::testing::InitGoogleTest(&argc,argv);
    int ret = RUN_ALL_TESTS();

    // 移除此处的 __gcov_dump(),交由 atexit(flushCoverage) 统一处理
    // __gcov_dump();

#ifdef QT_DEBUG
    __sanitizer_set_report_path("asan_loader.log");
#endif
    return ret;
}

2. src/tests/test-recoverage.sh - 构建与脚本逻辑

语法与逻辑

  • ASAN 与 Debug 模式的冲突:注释中写道“覆盖率构建不用 CONFIG+=debug,避免 ASAN 导致退出时崩溃、.gcda 文件丢失”。这个逻辑需要推敲:
    1. ASAN 通常只在 Debug 模式下生效,如果你使用 CONFIG+=release,默认不会开启 ASAN,也就不存在 ASAN 导致崩溃的问题。
    2. 覆盖率插桩(--coverage / -ftest-coverage)通常也建议在 Debug 模式下进行,因为 Release 模式的激进优化(如内联、死代码消除)会导致覆盖率数据不准确。
    3. 如果确实需要同时开启 ASAN 和 Coverage,正确的做法是在 qmake 中同时配置,并在代码中用 crashHandler 处理崩溃(正如你上面所做的),而不是退回到 Release 模式。
  • ls 命令的误用if ls asan_loader.log* 1>/dev/null 2>&1; then 这种写法不够严谨。如果当前目录下没有匹配的文件,ls 会向 stderr 输出错误信息(虽然被重定向了),且不同系统的 ls 行为可能不同。更安全的 Shell 写法是直接用通配符测试。

代码性能

  • make check -j$(nproc) 充分利用了多核,性能较好。

代码安全

  • 变量未加引号保护:脚本中 rm -rf $BUILD_DIR 非常危险。如果 BUILD_DIR 变量意外为空,这将变成 rm -rf /。必须加上双引号 rm -rf "$BUILD_DIR"

💡 改进建议:

# 1. 修复潜在的 rm -rf 灾难,给变量加上双引号
BUILD_DIR="build"
REPORT_DIR="report"

# 自动检测 qmake6 / qmake
if command -v qmake6 &>/dev/null; then
    QMAKE=qmake6
else
    QMAKE=qmake
fi

echo "Using qmake: $QMAKE ($($QMAKE --version 2>&1 | tail -1))"

SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
cd "$SCRIPT_DIR"

# 2. 变量加引号
rm -rf "$BUILD_DIR"
mkdir "$BUILD_DIR"
cd "$BUILD_DIR"

# 3. 建议重新评估是否真的需要 CONFIG+=release。
# 如果是为了避免 ASAN 冲突,可以尝试: $QMAKE CONFIG+=debug CONFIG+=coverage ../tests.pro
# 如果坚持用 release,保留原逻辑,但请注意覆盖率可能不准确。
$QMAKE CONFIG+=release ../tests.pro

TESTARGS="--gtest_output=xml:deepin_test_report_boot_maker.xml" make check -j$(nproc)

# 4. 使用更安全的 Shell 通配符判断文件是否存在
shopt -s nullglob
asan_files=(asan_loader.log*)
if [ ${#asan_files[@]} -gt 0 ]; then
    mv asan_loader.log* asan_dde-boot-maker.log
else
    touch asan_dde-boot-maker.log
fi
shopt -u nullglob

# 生成覆盖率报告
lcov -d ./ -c -o coverage_all.info
lcov --remove coverage_all.info "*/tests/*" "*/usr/include*" --output-file coverage.info
cd ..
genhtml -o "$REPORT_DIR" "$BUILD_DIR/coverage.info" # 变量加引号

exit 0

3. src/tests/unittestobj.cpp - 单例生命周期管理

语法与逻辑

  • 修复了严重的 UAF (Use-After-Free) / Double-Free 缺陷:原代码 delete m_pBMinterFace 是非常危险的。如果 BMInterface 是通过 DSingleton 实现的单例,其生命周期由静态局部变量或 Q_GLOBAL_STATIC 管理,强行 delete 会导致内存损坏或二次释放。改为 m_pBMinterFace = nullptr 是完全正确的。

代码质量

  • 注释解释了为什么只置空不 delete,这极大地提升了代码的可维护性,非常棒!

💡 改进建议:

  • 此处改动非常完美,无需修改。

4. src/tests/logmanagertest.cpp - 测试状态隔离

语法与逻辑

  • 修复了测试用例间的状态泄漏:在 setSystemlog 测试中,如果不恢复 setSystemLog(false),会导致全局状态被修改,后续依赖默认日志路径的测试(如 registerFileAppender)会因为 /var/log/deepin/ 无写权限而失败。这是单元测试中经典的“测试隔离”问题,修复得很好。

💡 改进建议:

  • 建议进一步利用 Google Test 的 SetUp()TearDown() 方法来确保状态的绝对隔离,而不是依赖测试用例内部的末尾还原。但如果项目目前运行稳定,当前的改法已经足够解决眼前的问题。

总结

你的修改方向非常正确,解决了很多工程痛点。请重点关注:

  1. main.cpp 中移除重复的 __gcov_dump() 调用,交由 atexit 处理即可。
  2. test-recoverage.sh 中的 rm -rf "$BUILD_DIR" 必须加双引号,防止变量为空时发生灾难性后果。
  3. 重新评估 CONFIG+=release 对覆盖率准确性的影响

@deepin-ci-robot

Copy link
Copy Markdown

[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.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@pengfeixx

Copy link
Copy Markdown
Contributor Author

/merge

@deepin-bot deepin-bot Bot merged commit d368bac into linuxdeepin:master Jun 4, 2026
22 checks passed
@pengfeixx pengfeixx deleted the feat/migrate-tests-to-qt6 branch June 4, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants