Repository navigation
test(iconutils): relax theme icon fallback assert - #847
Merged
Merged
Conversation
1. Drop the pixmap-null assertion in getThemeIconNonexistentFallback, since the fallback icon is resolved through the system icon theme. 2. Under QT_QPA_PLATFORM=offscreen no theme name is set and themeSearchPaths holds only ":/icons", so the fallback pixmap is always null. 3. Keep the assertion that a missing icon returns false, which is the control flow this case actually covers. Log: Stop asserting a non-null fallback pixmap in the theme icon unit test. Influence: Unit test passes in minimal chroots without deepin-icon-theme. test(iconutils): 放宽主题图标回退断言 1. 移除 getThemeIconNonexistentFallback 中对 pixmap 非空的断言，回退图标来自系统图标主题。 2. 在 QT_QPA_PLATFORM=offscreen 下未设置主题名且 themeSearchPaths 仅含 ":/icons"， 回退图标必然为空。 3. 保留缺失图标返回 false 的断言，这才是该用例真正覆盖的控制流。 Log: 移除主题图标单元测试中回退图标非空的断言。 PMS: TASK-398579 Influence: 最小化 chroot 环境下单元测试可通过。
Reviewer's GuideTestsRelax the nonexistent-theme-icon test to assert only that lookup fails, since the system-theme fallback may be null in offscreen or minimal environments.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
wjyrich
approved these changes
Nov 12, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MyLeeJiEun, wjyrich 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/iconutilstest.cpp" line_range="291" />
<code_context>
QPixmap pixmap;
bool result = IconUtils::getThemeIcon(pixmap, QStringLiteral("nonexistent-icon-name-xyz"), 32);
QVERIFY(!result);
- QVERIFY(!pixmap.isNull());
+ // Don't assert !pixmap.isNull(): the fallback is looked up through
+ // DIconTheme::findQIcon("application-x-desktop"), i.e. it comes from the
</code_context>
<issue_to_address>
**Fallback rendering is no longer tested**
When a future change breaks fallback pixmap loading while `getThemeIcon` still returns false for the missing icon, the remaining `QVERIFY(!result)` passes even when the default icon is available but `getThemeIcon` fails to populate the fallback pixmap, so this test no longer catches regressions in fallback rendering.
Use a controlled test icon theme or resource to verify fallback rendering without depending on host-installed themes.
</issue_to_address>| QPixmap pixmap; | ||
| bool result = IconUtils::getThemeIcon(pixmap, QStringLiteral("nonexistent-icon-name-xyz"), 32); | ||
| QVERIFY(!result); | ||
| QVERIFY(!pixmap.isNull()); |
There was a problem hiding this comment.
🟡 Medium · Fallback rendering is no longer tested
When a future change breaks fallback pixmap loading while getThemeIcon still returns false for the missing icon, the remaining QVERIFY(!result) passes even when the default icon is available but getThemeIcon fails to populate the fallback pixmap, so this test no longer catches regressions in fallback rendering.
Use a controlled test icon theme or resource to verify fallback rendering without depending on host-installed themes.
Prompt for AI agents
In `tests/iconutilstest.cpp` at line 291:
**Fallback rendering is no longer tested**
When a future change breaks fallback pixmap loading while `getThemeIcon` still returns false for the missing icon, the remaining `QVERIFY(!result)` passes even when the default icon is available but `getThemeIcon` fails to populate the fallback pixmap, so this test no longer catches regressions in fallback rendering.
Use a controlled test icon theme or resource to verify fallback rendering without depending on host-installed themes.
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 语法正确，逻辑清晰 ✅ 通过 潜在问题： 建议： [] 2. 代码质量 ✅评价： 代码结构清晰，注释完整 ✅ 通过 潜在问题： 建议： [] 3. 代码性能 ✅评价： 性能良好，资源使用合理 ✅ 通过 潜在问题： 建议： [] 4. 代码安全 🔒评价： 存在0个安全漏洞 ✅ 通过
安全漏洞详情： 建议： [] 💡 改进建议代码示例本次变更无需修复，代码质量良好。注释详尽且准确，测试逻辑合理。本报告由 AI 代码审查工具自动生成 |
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
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.
Log: Stop asserting a non-null fallback pixmap in the theme icon unit test.
Influence: Unit test passes in minimal chroots without deepin-icon-theme.
test(iconutils): 放宽主题图标回退断言
Log: 移除主题图标单元测试中回退图标非空的断言。
PMS: TASK-398579
Influence: 最小化 chroot 环境下单元测试可通过。
Summary by Sourcery
Allow the theme icon fallback test to pass when no system icon theme is available.
Enhancements:
Tests: