Skip to content

fix: resolve 7 production code defects found during test supplementation (DDE-135) - #158

Draft
MyLeeJiEun wants to merge 2 commits into
linuxdeepin:masterfrom
MyLeeJiEun:fix/dde-135-defects
Draft

fix: resolve 7 production code defects found during test supplementation (DDE-135)#158
MyLeeJiEun wants to merge 2 commits into
linuxdeepin:masterfrom
MyLeeJiEun:fix/dde-135-defects

Conversation

@MyLeeJiEun

@MyLeeJiEun MyLeeJiEun commented Aug 19, 2026

Copy link
Copy Markdown

概述

在补充 dde-services 单元测试(DDE-106 / 上游测试 PR #151)过程中,发现并确认了 7 处生产代码(被测源码)缺陷。本 PR 按架构师方案逐条修复,并同步更新了相关测试用例(将「按实际错误行为断言」翻转为「按正确行为断言」,并新增覆盖路径)。

与 PR #151 的依赖关系(重要)

本分支 fix/dde-135-defects 基于 PR #151 的测试提交,包含 2 个 commit:

  1. 1df7e0atest: add dde-services unit and D-Bus tests即 PR test: 补充 dde-services 单元测试与 D-Bus 测试 #151 的内容,本 PR 不修改 test: 补充 dde-services 单元测试与 D-Bus 测试 #151 的该提交)
  2. b88305efix: resolve 7 production code defects found during test supplementation (DDE-135)(本 PR 新增的缺陷修复 + 测试更新)

即本 PR 的修复提交 b88305e 直接层叠在 PR #151 的测试提交 1df7e0a 之上。第 2 个 commit 既修复生产代码,又对 #151 新增的部分测试用例做了断言翻转与增强,因此二者存在依赖。

7 处缺陷修复

生产代码修复(4 文件)

# 缺陷 文件 修复
#6 loadFile 空行 QString::front() UB src/plugin-qt/xsettings/modules/api/keyfile.cpp replace("^ +") 后、front() 前新增 if (line.isEmpty()) continue;
#2 deleteKey 恒返回 false 同上 成功分支 return false;return true;(section 缺失仍 false)
#3 getBool section 缺失返回值不一致 同上 return false;return defaultValue;,与 getStr 一致
#4 typeMap gif 映射错误 src/plugin-qt/wallpaperslideshow/background/format.cpp {"image/gif","jpeg"}{"image/gif","gif"}
#1 isFileExists URI 解码结果未用 src/plugin-qt/wallpaperslideshow/utils.cpp QFile::exists(filename)QFile::exists(path)
#7 WriteStringToFile .swap 目录 bug 同上 重写:swapFile = filename + ".swap"(同级文件,非子路径);mkpath 确保父目录存在;写 swap 文件后 remove 旧文件再 rename(Qt QFile::rename 拒绝覆盖已存在目标)
#5 极昼 sunrise 值无意义 src/plugin-qt/thememanager/sunrisesunset.cpp sunsetUT 哨兵修复后对称新增 if (sunriseUT <= -100) sunriseUT = 0;,极昼 sunrise 归一到请求日 0 点

测试代码更新(4 文件,+101/-54)

# 用例变更 说明
#3 getBoolMissingSectionReturnsFalseNotDefaultgetBoolMissingSectionReturnsDefault 断言翻转 falsetrue,补 defaultValue=false 对称断言
#4 gifMapsToJpegByActualBehaviorgifMapsToGif 断言翻转 "jpeg""gif"
#1 isFileExistsUriInputReturnsFalseDueToDecodeBugisFileExistsUriInputResolvesAfterDecode 断言翻转 !existsexists,补不存在 URI 仍 false
#7 writeStringToFileNonEmptyAlwaysFailsDueToSwapDirBugwriteStringToFileWritesContentViaSwap 改为断言成功 + 校验落盘内容 + 验证无 .swap 残留
#7 writeStringToFileUnwritableParentReturnsFalsewriteStringToFileOverwritesExistingFile 改造为「覆盖已有文件成功」并校验新内容
#2 deleteKeyRemovesEntry 增强:QVERIFY(kf.deleteKey(...)) 锁定返回 true
#6 新增 loadFileSkipsBlankAndWhitespaceLines 空行 / 纯空格行 / 末尾无换行空格,覆盖 UB 路径
#5 polarDayStillReturnsTrue 增强 新增 QCOMPARE(sunrise.date(), date) 断言

验证情况

  • 单元测试:79 用例全通过(tst-keyfile 26 / tst-format 8 / tst-sunrisesunset 14 / tst-wpssl-utils 31),0 失败,与 Developer 手动实跑一致;独立 UT 验证覆盖率(4 修复文件)行 96.5% / 函数 96.8% / 分支 59.0%,7 处修复行均被用例执行到。
  • 代码审核:通过(1 项非阻断建议:fix: fix code review errors #7 未来可改用 QSaveFile 消除 remove+rename 间的原子性窗口;当前该函数无生产调用方,风险纯理论)。[deepin-service-plugins] Updates for project Deepin Desktop Environment #4 isBackgroundFile 行为不变、chore: 添加libglib2.0-dev构建依赖 #5 白昼判定逻辑不变的无回归结论成立。
  • 构建打包:仓库根 cmake 全量构建 99/99 目标 0 error;dpkg-buildpackage 成功,产出 dde-services_1.0.25_amd64.deb

本 PR 设为 draft,留待人工审核与合并,不自行合并。架构师方案、Developer 改动清单、自测报告、审核结论均在 DDE-135 issue 评论区可见。

Summary by Sourcery

Fix seven production defects and add regression coverage for the affected services and interfaces.

Bug Fixes:

  • Correct file parsing, key deletion status, missing-section boolean defaults, GIF type detection, URI file checks, file replacement, and polar-day sunrise handling.

Enhancements:

  • Add comprehensive unit coverage for xsettings, wallpaper slideshow, image format detection, sunrise/sunset calculations, and utility helpers.
  • Add a D-Bus contract test covering wallpaper slideshow introspection, methods, and property access.

Build:

  • Register the new test suite through CMake when BUILD_TESTING is enabled.
  • Update the copyright year range in the top-level CMake configuration.

Tests:

  • Add Qt Test targets and CTest registration for keyfile, wallpaper slideshow utilities and formats, sunrise/sunset, xsettings utilities, and wallpaper slideshow D-Bus behavior.

MyLeeJiEun and others added 2 commits August 18, 2026 16:53
1. Add tests/ with 6 Qt6::Test ctest targets: thememanager
   sunrise, wpssl utils/format/dbus, xsettings keyfile/xsutils
2. Add fakeservice.h D-Bus stub; enable AUTOMOC on the dbus target
3. Isolate wpssl-utils via ctest XDG_CONFIG_HOME; QSKIP on no bus
4. 109/109 cases pass; coverage (target -fno-exceptions): line
   94.9%, function 90.7%, branch 91.0%, all above the 70% bar

Influence:
1. Build with -DBUILD_TESTING=ON and run ctest for the 6 targets
2. Coverage via -fprofile-arcs -ftest-coverage + --coverage link,
   lcov branch coverage; build the test target with -fno-exceptions

test: 补充 dde-services 单元测试与 D-Bus 测试

1. 新增 tests/ 与 6 个 Qt6::Test ctest target:thememanager
   sunrise、wpssl utils/format/dbus、xsettings keyfile/xsutils
2. 新增 fakeservice.h D-Bus 桩,dbus target 接入 AUTOMOC
3. wallpaperslideshow-utils 用 ctest XDG_CONFIG_HOME 隔离,缺
   session bus 时 QSKIP
4. 109/109 用例通过;覆盖率(target -fno-exceptions):行
   94.9%、函数 90.7%、分支 91.0%,三项均达 70% 门线

Influence:
1. 以 -DBUILD_TESTING=ON 构建,对 6 个 target 运行 ctest
2. 覆盖率:-fprofile-arcs -ftest-coverage + --coverage 链接,
   lcov 分支覆盖;测试 target 以 -fno-exceptions 构建
…ion (DDE-135)

Production code fixes:
  linuxdeepin#6 keyfile.cpp: guard against empty QString::front() UB on blank lines
  linuxdeepin#2 keyfile.cpp: deleteKey now returns true on successful removal
  linuxdeepin#3 keyfile.cpp: getBool returns defaultValue for missing section (was false)
  linuxdeepin#4 format.cpp: typeMap maps image/gif -> "gif" (was "jpeg")
  linuxdeepin#1 utils.cpp: isFileExists checks decoded path (was raw filename)
  linuxdeepin#7 utils.cpp: WriteStringToFile uses sibling swap file instead of sub-dir
  linuxdeepin#5 sunrisesunset.cpp: polar-day sunrise normalised to requested date (was -100h)

Test updates (flip buggy-behavior assertions to correct behavior):
  linuxdeepin#3 getBoolMissingSectionReturnsFalseNotDefault -> ReturnsDefault (assert true)
  linuxdeepin#4 gifMapsToJpegByActualBehavior -> gifMapsToGif (assert "gif")
  linuxdeepin#1 isFileExistsUriInputReturnsFalseDueToDecodeBug -> ResolvesAfterDecode (assert exists)
  linuxdeepin#7 writeStringToFileNonEmptyAlwaysFailsDueToSwapDirBug -> WritesContentViaSwap (assert success+content)
  linuxdeepin#7 writeStringToFileUnwritableParentReturnsFalse -> OverwritesExistingFile (assert overwrite success)
  linuxdeepin#2 deleteKeyRemovesEntry: added QVERIFY(deleteKey(...)) return-value assertion

New test cases:
  linuxdeepin#6 loadFileSkipsBlankAndWhitespaceLines (empty/whitespace/trailing-no-newline)
  linuxdeepin#5 polarDayStillReturnsTrue: added sunrise.date()==date assertion

All 79 tests pass (26 keyfile + 8 format + 14 sunrisesunset + 31 wpssl-utils).
@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MyLeeJiEun

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

@github-actions

Copy link
Copy Markdown

CLA Assistant Lite bot:

如果你是以企业贡献者的身份进行提交,请联系我们签署企业贡献者许可协议
If you submit as corporate contributor, please contact us to sign our Corporate Contributor License Agreement

感谢您的提交,我们非常感谢。 像许多开源项目一样,在接受您的贡献之前,我们要求您签署我们的个人贡献者许可协议。 您只需发布与以下格式相同的评论即可签署个人贡献者许可协议
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Individual Contributor License Agreement before we can accept your contribution. You can sign the Individual Contributor License Agreement by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA.

shuttle slave seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

@deepin-ci-robot

Copy link
Copy Markdown

Hi @MyLeeJiEun. Thanks for your PR.

I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sourcery-ai

sourcery-ai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Reviewer's Guide

Fixes seven production defects in wallpaperslideshow utils/format, xsettings KeyFile, and thememanager SunriseSunset, and wires up a new test suite (Qt6::Test + ctest) that both flips existing assertions from "assert current buggy behavior" to "assert correct behavior" and adds branch-coverage tests for the fixed code paths and XSettings utilities, plus a D-Bus contract test for the WallpaperSlideshow interface.

File-Level Changes

Change Details Files
Make WriteStringToFile and isFileExists in wallpaperslideshow utils correctly handle normal file paths and URIs, and ensure swap-based writes are reliable and overwrite existing files.
  • Switch WriteStringToFile from creating a ".swap" subdirectory to using a sibling ".swap" temp file and ensure the parent dir exists via QDir::mkpath.
  • Before renaming, delete any existing destination file so QFile::rename can atomically replace it within the same directory.
  • Make isFileExists use the decoded URI path from deCodeURI when checking QFile::exists, so file:// URIs work.
src/plugin-qt/wallpaperslideshow/utils.cpp
Fix KeyFile API semantics: getBool honors defaultValue when the section is missing, deleteKey reports success correctly, and loadFile safely skips blank/whitespace lines before inspecting the first character.
  • Change getBool to return defaultValue when the requested section is absent to match getStr behavior.
  • Make deleteKey return true when a key is successfully removed from an existing section instead of always returning false.
  • In loadFile, strip leading spaces, then skip empty lines entirely before calling front() to avoid undefined behavior on QString::front for empty strings.
src/plugin-qt/xsettings/modules/api/keyfile.cpp
Correct MIME-type mapping for GIF files in wallpaperslideshow FormatPicture so image/gif resolves to the "gif" format token.
  • Update typeMap entry for image/gif to map to "gif" instead of "jpeg" so GIF wallpapers are classified correctly.
src/plugin-qt/wallpaperslideshow/background/format.cpp
Normalise SunriseSunset behavior for polar day so sunrise has a meaningful value on extreme-latitude days when the sun never rises.
  • When sunriseUT sentinel is <= -100, set sunriseUT to 0 so sunrise is at the requested date’s start-of-day while sunset keeps the 100-hour sentinel, preserving the "sun is up" interval semantics.
src/plugin-qt/thememanager/sunrisesunset.cpp
Enable building and running the new test suite via CTest and add a dedicated tests CMake subtree that compiles unit and D-Bus contract tests against production sources without polluting plugin modules.
  • Extend the top-level CMakeLists.txt SPDX year range and gate the tests subtree under BUILD_TESTING, adding add_subdirectory("tests") only when testing is enabled.
  • Create tests/CMakeLists.txt that defines Qt6::Test-based executables for SunriseSunset, wallpaperslideshow utils/format, XSettings KeyFile/Utils, and a WallpaperSlideshow D-Bus adaptor test, each linked only with the specific production source files under test.
  • Wire each executable into ctest with add_test, including environment isolation for XDG_CONFIG_HOME when testing writeWallpaperConfig.
CMakeLists.txt
tests/CMakeLists.txt
Add focused unit tests for xsettings KeyFile to cover loadFile edge cases, default-value semantics for getStr/getBool/getStrList, deleteKey behavior, saveToFile error paths, and the debug print loop.
  • Introduce tst_keyfile.cpp with tests that exercise normal parsing, missing-file and empty-file handling, comments, blank/whitespace-only lines, malformed section headers, keys before any section, values containing '=', multiple sections, and mainKeys enumeration.
  • Add assertions for getStr/getBool default-value behavior on missing keys/sections and empty values, getStrList splitting behavior, containKey on missing sections, deleteKey success/failure cases, saveToFile to unwritable paths, and the print iteration loop.
tests/tst_keyfile.cpp
Add comprehensive unit tests for wallpaperslideshow utils to validate URI detection/encoding/decoding, filesystem helpers, the fixed isFileExists/WriteStringToFile behavior, wallpaper lock/config helpers, and XDG directory access.
  • Create tst_wpssl_utils.cpp covering isURI, deCodeURI, enCodeURI, isSolidWallpaper, isDir, isFilesInDir, and isFileExists on both plain paths and URIs (including the DDE-135 chore: fix license check #1 fix).
  • Add tests for WriteStringToFile returning false on empty filename, writing content via sibling .swap temp files, overwriting existing files correctly, and ensuring no .swap residue remains (DDE-135 fix: fix code review errors #7).
  • Verify checkWallpaperLockedStatus executes without crash, user home/config/cache/runtime dir helpers return sensible values, and writeWallpaperConfig writes/updates config.json in an isolated XDG_CONFIG_HOME directory.
tests/tst_wpssl_utils.cpp
Add unit tests for xsettings Utils byte-manipulation helpers to cover integer read/write for multiple widths, string and skip operations including error paths, and variant value presence.
  • Introduce tst_xsutils.cpp with tests for getPad across a range of inputs, readInteger/writeInteger for uint32 (including failure on too-short buffers and round-trip correctness) and for uint16 to hit the sizeof==2 branch.
  • Add tests for readString/readSkip success/failure cases, including zero-length operations that leave buffers untouched while returning empty strings, writeString/writeSkip behavior, and hasXsValue over all XsValue variant alternatives including the default-constructed case.
tests/tst_xsutils.cpp
Add unit tests for SunriseSunset covering typical mid-latitude scenarios, polar day/night sentinel handling including the new sunrise normalisation, and diverse geographic/timezone inputs that exercise internal hour-wrap branches.
  • Create tst_sunrisesunset.cpp with tests that assert getSunriseSunset returns true with valid QDateTime outputs whose dates match the request for Beijing summer/winter solstice, equinox sunrise-before-sunset, and longitude shifts affecting sunrise time.
  • Add tests for polar day ensuring sunrise is normalized to the requested date’s start-of-day and sunset > sunrise (DDE-135 chore: 添加libglib2.0-dev构建依赖 #5 fix), polar night where sunrise==sunset at 100h sentinel, and several extreme longitude/offset/latitude cases that validate output without strict hour expectations but drive UT<0/UT>=24 wrap branches.
tests/tst_sunrisesunset.cpp
Add unit tests for wallpaperslideshow FormatPicture MIME mapping, including coverage for GIF mapping and non-image files.
  • Introduce tst_format.cpp with data-driven tests for PNG/BMP/JPEG/TIFF formats using real QImage fixtures saved to disk and validated through FormatPicture::getPictureType.
  • Add a targeted GIF test that writes a minimal GIF89a header by hand so QMimeDatabase yields image/gif and asserts that getPictureType now returns "gif" (DDE-135 [deepin-service-plugins] Updates for project Deepin Desktop Environment #4), plus a test that non-image text files return an empty type string.
tests/tst_format.cpp
Provide a fake WallpaperSlideshow service and D-Bus contract test that validates the org.deepin.dde.WallpaperSlideshow interface and its generated adaptor against a real session bus without pulling in heavy production dependencies.
  • Add fakeservice.h implementing FakeWallpaperSlideshowService with a WallpaperSlideShow property and SetWallpaperSlideShow/GetWallpaperSlideShow slots storing per-monitor slideshow strings.
  • Introduce tst_wallpaperslideshow_dbus.cpp that generates a WallpaperSlideshowAdaptor from the project XML, registers the fake service on a unique per-process name and the standard object path, and tests introspection XML, monitor-specific Set/Get round-trips, independence of different monitors, and read/write access to the WallpaperSlideShow property via org.freedesktop.DBus.Properties.
tests/fakeservice.h
tests/tst_wallpaperslideshow_dbus.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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants