test: add comprehensive unit tests for property dialog. #3394
Conversation
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
All tests follow the Google Test framework and verify proper initialization, method execution, and error handling for the property dialog plugin components. Log: add comprehensive unit tests for property dialog.
Reviewer's GuideAdds comprehensive Google Test-based unit tests for the dfmplugin-propertydialog plugin (dialogs, widgets, menu scene, events, and global enums) and extends trashcore plugin tests (TrashFileInfo and TrashCore) to cover previously untested branches and edge cases. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
deepin pr auto review我来对这次代码变更进行详细审查：
b) test_multifilepropertydialog.cpp：
c) test_propertydialog.cpp：
d) test_propertydialog_global.cpp：
e) test_propertyeventcall.cpp：
f) test_propertymenuscene.cpp：
b) test_trashfileinfo.cpp：
总体评价：
这些变更整体上是积极的，提高了代码的测试覆盖率，但还可以进一步完善测试用例，特别是边界条件和异常情况的处理。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Several tests (e.g., PermissionManagerWidgetCanChmodTest) use assertions like
EXPECT_TRUE(canChmod || !canChmod), which are effectively no-ops; consider replacing these with more meaningful expectations or simplyEXPECT_NO_THROWaround the call if you only want to validate it doesn’t crash. - Many tests construct URLs pointing to hard-coded paths such as
/tmp/test.txt; to avoid environment-dependent behavior and potential flakiness, consider usingQTemporaryFile/QDir::tempPath()or local test fixtures instead of fixed absolute paths. - There are multiple commented-out test bodies (e.g., FilePropertyDialogContentHeightTest, FilePropertyDialogMousePressEventTest, EditStackedWidget::renameFile path) left in the suite; consider either fixing and enabling these tests or removing the commented code to keep the test files clean and focused.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests (e.g., PermissionManagerWidgetCanChmodTest) use assertions like `EXPECT_TRUE(canChmod || !canChmod)`, which are effectively no-ops; consider replacing these with more meaningful expectations or simply `EXPECT_NO_THROW` around the call if you only want to validate it doesn’t crash.
- Many tests construct URLs pointing to hard-coded paths such as `/tmp/test.txt`; to avoid environment-dependent behavior and potential flakiness, consider using `QTemporaryFile`/`QDir::tempPath()` or local test fixtures instead of fixed absolute paths.
- There are multiple commented-out test bodies (e.g., FilePropertyDialogContentHeightTest, FilePropertyDialogMousePressEventTest, EditStackedWidget::renameFile path) left in the suite; consider either fixing and enabling these tests or removing the commented code to keep the test files clean and focused.
## Individual Comments
### Comment 1
<location> `autotests/plugins/dfmplugin-propertydialog/test_propertydialog.cpp:404-408` </location>
<code_context>
+ EXPECT_NO_THROW(delete dialog);
+}
+
+// TEST_F(TestPropertyDialog, FilePropertyDialogContentHeightTest)
+// {
+// FilePropertyDialog dialog;
+// int height = dialog.contentHeight();
+// EXPECT_GE(height, 0);
+// }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Commented‑out tests should be either fixed or removed to avoid dead code and confusion about intended coverage.
Given the commented‑out tests (e.g. `FilePropertyDialogContentHeightTest`, `FilePropertyDialogMousePressEventTest`, and the rename/mouse tests in `EditStackedWidget`), please either make them reliable (e.g. via `QSignalSpy`, proper event loop handling, or stubs) or remove them and record the missing coverage in a ticket. Keeping them commented out obscures which behaviors are actually tested.
Suggested implementation:
```cpp
TEST_F(TestPropertyDialog, FilePropertyDialogDestructorTest)
{
FilePropertyDialog *dialog = new FilePropertyDialog();
EXPECT_NO_THROW(delete dialog);
}
#include "dfmplugin_propertydialog_global.h"
```
```cpp
```
The snippet you provided only shows one commented-out test (`FilePropertyDialogContentHeightTest`). Your review comment also mentions other commented-out tests (`FilePropertyDialogMousePressEventTest`, and rename/mouse tests in `EditStackedWidget`) that are not visible in this excerpt.
To fully implement your suggestion and avoid dead code:
1. Locate each of those commented-out tests in `autotests/plugins/dfmplugin-propertydialog/test_propertydialog.cpp`.
2. For each one, either:
- Remove the commented-out test block entirely (as done above), and create an external ticket documenting the missing coverage, or
- Re-enable and fix the test (e.g. by:
* Using `QSignalSpy` to wait for expected signals instead of manual sleeps,
* Ensuring the dialog/widget is shown and an event loop is running (`QTest::qWaitForWindowExposed`, `QCoreApplication::processEvents`),
* Using `QTest::mouseClick` / `QTest::mouseMove` / `QTest::keyClick` to drive interactions).
3. Ensure no `// TEST_F(...` or similarly commented-out test macros remain in the file so that the active test suite clearly represents current coverage.
If you share the rest of the file, I can provide concrete SEARCH/REPLACE blocks for each of the other commented-out tests as well.
</issue_to_address>
### Comment 2
<location> `autotests/plugins/dfmplugin-propertydialog/test_propertydialog.cpp:514-519` </location>
<code_context>
+ EXPECT_NO_THROW(widget.toggleFileExecutable(false));
+}
+
+TEST_F(TestPropertyDialog, PermissionManagerWidgetCanChmodTest)
+{
+ PermissionManagerWidget widget;
+ FileInfoPointer info = DFMBASE_NAMESPACE::InfoFactory::create<DFMBASE_NAMESPACE::FileInfo>(QUrl::fromLocalFile("/tmp/test"));
+ bool canChmod = widget.canChmod(info);
+ EXPECT_TRUE(canChmod || !canChmod); // Just test it doesn't crash
+}
+
</code_context>
<issue_to_address>
**issue (testing):** The assertion `EXPECT_TRUE(canChmod || !canChmod)` is a tautology and doesn’t provide any behavioral checking.
This is effectively just calling `widget.canChmod(info);` and discarding the result. If you only want to assert it doesn’t crash, `EXPECT_NO_THROW(widget.canChmod(info));` is clearer. Ideally, if you can control `FileInfoPointer` via stubs, add tests that expect both `true` and `false` under specific permission/ownership conditions to capture the intended behavior.
</issue_to_address>
### Comment 3
<location> `autotests/plugins/dfmplugin-trashcore/test_trashfileinfo.cpp:454-461` </location>
<code_context>
+ });
+}
+
+// 新增测试用例：测试 fileName 方法中 dFileInfo 为空的情况
+TEST_F(TrashFileInfoTest, FileName_WithNullDFileInfo)
+{
+ QUrl url("trash:///test.txt");
+ TrashFileInfo info(url);
+
+ // Mock dFileInfo to be null (this is difficult to achieve in practice, but we can test the method)
+ EXPECT_NO_THROW(info.nameOf(DFMBASE_NAMESPACE::FileInfo::FileNameInfoType::kFileName));
+}
+
</code_context>
<issue_to_address>
**issue (testing):** The test claims to cover the `dFileInfo == nullptr` case, but the internal state is not actually forced to null.
The test currently just constructs `TrashFileInfo info(url);` and calls `nameOf(...)`, without actually forcing `dFileInfo` to `nullptr`. Depending on the constructor, `dFileInfo` may still be valid, so the null edge case isn’t really exercised. To test this accurately, you’d need a way to ensure `dFileInfo` is null (e.g., via a stubbed factory/internal initializer or a dedicated injection point). As written, the test name/comment overstate what’s covered.
</issue_to_address>
### Comment 4
<location> `autotests/plugins/dfmplugin-trashcore/test_trashfileinfo.cpp:391-400` </location>
<code_context>
+ QUrl url("trash:///test.txt");
+ TrashFileInfo info(url);
+
+ // Mock DFileInfo::attribute to return a valid target URI
+ stub.set_lamda(&DFMIO::DFileInfo::attribute, [](DFMIO::DFileInfo *self, DFMIO::DFileInfo::AttributeID id, bool *outSuccess) -> QVariant {
+ Q_UNUSED(self);
+ if (id == DFMIO::DFileInfo::AttributeID::kStandardTargetUri) {
+ if (outSuccess) *outSuccess = true;
+ return QVariant::fromValue<QUrl>(QUrl("file:///home/user/test.txt"));
+ }
+ if (outSuccess) *outSuccess = false;
+ return QVariant();
+ });
+
+ // 确保不会抛出异常
+ EXPECT_NO_THROW({
+ // 注意：我们无法直接调用私有的 initTarget 方法，但可以通过构造函数间接测试
+ TrashFileInfo info2(url);
+ });
+}
</code_context>
<issue_to_address>
**suggestion (testing):** For the `initTarget` branch tests, you stub `attribute` but never assert on the resulting target state.
These tests currently only verify that constructing `TrashFileInfo info2(url);` doesn’t throw, so they don’t confirm the new `initTarget` branches actually ran or produced the correct state. Please add assertions that validate the resolved target URL/path and any flags or state `initTarget()` is supposed to set (or some observable derived from those, like `pathOf(...)`/`nameOf(...)`), rather than relying solely on crash‑free execution.
Suggested implementation:
```cpp
// 新增测试用例：测试 initTarget 方法中的未覆盖分支
TEST_F(TrashFileInfoTest, InitTarget_WithValidTargetUri)
{
QUrl url("trash:///test.txt");
QUrl expectedTargetUrl("file:///home/user/test.txt");
// 先构造 TrashFileInfo 对象，确保 initTarget 在构造过程中被调用
TrashFileInfo info(url);
// Mock DFileInfo::attribute to return a valid target URI
stub.set_lamda(&DFMIO::DFileInfo::attribute,
[](DFMIO::DFileInfo *self,
DFMIO::DFileInfo::AttributeID id,
bool *outSuccess) -> QVariant {
Q_UNUSED(self);
if (id == DFMIO::DFileInfo::AttributeID::kStandardTargetUri) {
if (outSuccess) {
*outSuccess = true;
}
return QVariant::fromValue<QUrl>(QUrl("file:///home/user/test.txt"));
}
if (outSuccess) {
*outSuccess = false;
}
return QVariant();
});
// 确保不会抛出异常，并验证 initTarget 解析出的目标信息是否正确
EXPECT_NO_THROW({
// 注意：我们无法直接调用私有的 initTarget 方法，但可以通过构造函数间接测试
TrashFileInfo info2(url);
// 验证 initTarget 解析出的目标 URL 是否与 stub 返回的一致
EXPECT_EQ(info2.targetUrl(), expectedTargetUrl);
// 如果 TrashFileInfo 暴露了用于获取目标路径 / 文件名的接口，这里也一并验证
EXPECT_EQ(info2.targetUrl().toLocalFile(), QStringLiteral("/home/user/test.txt"));
});
}
```
The snippet above assumes that:
1. `TrashFileInfo` exposes a `targetUrl()` accessor that reflects the state set by `initTarget()`.
2. `targetUrl().toLocalFile()` is a valid way to obtain the resolved file system path.
If your actual API differs, you should adjust the assertions accordingly, for example:
- Replace `info2.targetUrl()` with whatever accessor is used to expose the resolved target (e.g. `info2.resolvedUrl()`, `info2.targetFileInfo().url()`, etc.).
- If there are dedicated helpers like `pathOf(...)` / `nameOf(...)` or specific flags that `initTarget()` is supposed to set, add `EXPECT_*` checks on those instead of (or in addition to) the `targetUrl()` assertions.
- If `TrashFileInfo` does not provide direct access to the URL but exposes only a path, adapt the `EXPECT_EQ` to compare that path with `"/home/user/test.txt"` (or the appropriate representation) instead.
</issue_to_address>
### Comment 5
<location> `autotests/plugins/dfmplugin-trashcore/test_trashcore.cpp:65-52` </location>
<code_context>
+}
+
+// 新增测试用例：测试 start 方法中的不同分支路径
+TEST_F(TrashCoreTest, Start_WithPropertyDialogPlugin)
+{
+ TrashCore plugin;
+
+ // 目前只测试基本的 start 方法
+ EXPECT_TRUE(plugin.start());
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** `Start_WithPropertyDialogPlugin` doesn’t currently differentiate from `Start_Basic` or assert the property‑dialog‑plugin specific behavior.
This test currently mirrors `Start_Basic`, so it doesn’t validate any plugin-specific behavior. If `start()` branches based on a property dialog plugin being registered, please stub/mock the relevant registry/factory so you can: (1) simulate the plugin’s presence and assert the expected registration/callback, and (2) optionally cover the absence case. If there’s no such branch yet, consider renaming the test to reflect its current scope instead of implying plugin-specific behavior.
Suggested implementation:
```cpp
// 新增测试用例：测试 start 方法：未注册属性对话框插件时的基础启动路径
TEST_F(TrashCoreTest, Start_NoPropertyDialogPlugin)
{
TrashCore plugin;
// 当前测试仅覆盖未注册属性对话框插件时的基础启动路径
EXPECT_TRUE(plugin.start());
}
// 新增测试用例：测试 followEvents 方法
TEST_F(TrashCoreTest, FollowEvents_Basic)
{
TrashCore plugin;
// 只需确保方法不抛出异常
EXPECT_NO_THROW(plugin.followEvents());
}
```
If `TrashCore::start()` later gains branching logic based on a property-dialog plugin registry/factory, you can extend `Start_NoPropertyDialogPlugin` and add a `Start_WithPropertyDialogPlugin` test by:
1. Stubbing/mocking the registry or factory in `TrashCoreTest` to simulate a registered property-dialog plugin.
2. Asserting that the expected registration/callback specific to that plugin is invoked (e.g., via mock expectations or state changes).
3. Optionally adding a test to cover the “plugin absent” case explicitly if behavior diverges from the default path.
</issue_to_address>
### Comment 6
<location> `autotests/plugins/dfmplugin-propertydialog/test_propertymenuscene.cpp:74-80` </location>
<code_context>
+ EXPECT_EQ(name, "PropertyMenu");
+}
+
+TEST_F(TestPropertyMenuScene, PropertyMenuSceneInitialize)
+{
+ PropertyMenuScene scene;
+ QVariantHash params;
+ // Test with empty params
+ bool result = scene.initialize(params);
+ EXPECT_FALSE(result); // Should fail with empty params
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** PropertyMenuScene is only tested for failure paths; a successful initialize/create/trigger flow isn’t covered.
Right now we only assert that `initialize` fails with empty params and that `create`/`triggered` don’t succeed in invalid scenarios. To exercise the intended behavior, please add at least one positive test that:
- builds a realistic `QVariantHash` for `initialize` and asserts it returns `true`,
- calls `create(&menu)` and confirms the expected property actions are added,
- invokes `triggered` on a known action and checks it returns `true` or produces the expected side effect.
This will ensure the success path is covered, not just error handling.
Suggested implementation:
```cpp
TEST_F(TestPropertyMenuScene, PropertyMenuSceneName)
{
PropertyMenuScene scene;
QString name = scene.name();
EXPECT_EQ(name, "PropertyMenu");
}
TEST_F(TestPropertyMenuScene, PropertyMenuSceneInitializeCreateTriggerSuccess)
{
PropertyMenuScene scene;
// Build a realistic parameter set for a successful initialization.
QVariantHash params;
params.insert(QStringLiteral("url"), QUrl::fromLocalFile(QStringLiteral("/tmp")));
bool initResult = scene.initialize(params);
EXPECT_TRUE(initResult);
// After successful initialization, create should populate the menu.
QMenu menu;
bool createResult = scene.create(&menu);
EXPECT_TRUE(createResult);
ASSERT_FALSE(menu.actions().isEmpty());
// Trigger a known action from the created menu and verify the success path.
QAction *action = menu.actions().first();
ASSERT_NE(action, nullptr);
bool triggerResult = scene.triggered(action);
EXPECT_TRUE(triggerResult);
}
```
The positive-flow test assumes:
1. `initialize` succeeds when given a `QVariantHash` containing a `"url"` key with a `QUrl` value. If `PropertyMenuScene` expects different or additional keys (e.g. `"windowId"`, `"selectUrls"`, etc.), update the `params.insert(...)` calls accordingly so that `initialize` returns `true`.
2. `create(&menu)` returns `true` and populates `menu.actions()` when initialization was successful. If the contract is slightly different (e.g. it always returns `true` but may leave the menu empty for some parameter sets), tighten the expectations to match the actual behavior (for example, assert a specific action count or specific action text).
3. `triggered(action)` returns `true` for at least one of the actions added by `create`. If the function instead reports success via side effects (signals, state changes) rather than the return value, replace the `EXPECT_TRUE(triggerResult);` check with assertions on those side effects (e.g. checking a mock, a spy, or scene-internal state exposed for testing).
You may also want to choose a more specific action than `menu.actions().first()` (e.g. by matching on `action->text()` or `objectName()`) once you know which property action should be present for the given parameters.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Johnson-zs
approved these changes
Dec 4, 2025
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Johnson-zs, re2zero The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
|
/forcemerge |
Contributor
|
This pr force merged! (status: blocked) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
All tests follow the Google Test framework and verify proper initialization, method execution, and error handling for the property dialog plugin components.
Log: add comprehensive unit tests for property dialog.
Summary by Sourcery
Expand automated test coverage for the property dialog plugin and trash core components, including dialogs, widgets, menu scenes, global enums, and event calls, to better validate construction, basic behavior, and error-handling paths.
Tests: