Repository navigation
test: add unit tests for 3rdparty archive interface and plugins - #553
Open
dengzhongyuan365-dev wants to merge 2 commits into
Open
dengzhongyuan365-dev wants to merge 2 commits into
dengzhongyuan365-dev wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengzhongyuan365-dev 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 |
Reviewer's GuideConfigurationRegister the contract test executable and its plugin build dependencies so it participates in the test target.
TestsDefine shared fixtures and parameterized read-only and read-write contracts, then exercise archive operations, content, signals, cancellation, passwords, and interface defaults across plugin backends.
Update existing CLI plugin expectations and state/setup coverage to match current switch types, extraction prerequisites, and completion behavior.
Align the RAR plugin tests with its list-valued test switch and current wrong-password line handling.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 8 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/UnitTest/3rdparty/contract/contract_test.h" line_range="143" />
<code_context>
+
+ QProcess proc;
+ proc.setWorkingDirectory(m_tempDir->filePath("zip_src"));
+ proc.start("zip", QStringList() << "-r" << fullPath << "file1.txt" << "dir");
+ proc.waitForFinished();
+ return fullPath;
</code_context>
<issue_to_address>
**Missing tools invalidate archive fixtures**
When an external archive command is unavailable or fails, the archive fixture helpers return paths without confirming that `QProcess` started successfully, exited successfully, or produced valid archives. Tests then fail on missing fixtures, and wrong-password tests can pass without exercising password handling.
Generate the fixtures without relying on undeclared external commands, and verify each archive before tests use it.
Also at `tests/UnitTest/3rdparty/contract/contract_test.h:159-161`, `tests/UnitTest/3rdparty/contract/contract_test.h:174-176`, `tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp:351`.
</issue_to_address>
### Comment 2
<location path="tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp" line_range="332" />
<code_context>
+ ASSERT_NE(pwIface, nullptr);
+
+ // Set a wrong password
+ pwIface->setPassword("wrongpassword");
+
+ QString extractDir = globalFixture().tempPath() + "/extract_wrongpw";
</code_context>
<issue_to_address>
**Wrong-password test blocks**
When the Libzip wrong-password contract case runs, `LibzipPlugin::extractFiles()` clears the password set by the test and requests a password; because the test only handles overwrite queries, `waitForResponse()` blocks and the contract test never reaches its assertion.
Handle the password-needed query in the test and respond with the wrong password so extraction can return an error.
Also at `tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp:349`.
</issue_to_address>
### Comment 3
<location path="tests/UnitTest/3rdparty/interface/ut_common.cpp" line_range="136" />
<code_context>
+TEST_F(UT_Common, test_isSubpathOfLnfs)
+{
+ bool result = m_tester->isSubpathOfLnfs("/tmp");
+ EXPECT_FALSE(result); // /tmp is not on a LNF filesystem
+}
+
</code_context>
<issue_to_address>
**LNF mount makes assertion fail**
When `/tmp` is mounted on `fuse.dlnfs` or `ulnfs`, `Common::isSubpathOfLnfs("/tmp")` returns true, so the hard-coded `EXPECT_FALSE` fails and makes the unit-test result depend on the machine’s mount configuration.
Avoid asserting that `/tmp` is never on an LNF filesystem; test the result against a controlled path or mount configuration.
</issue_to_address>
### Comment 4
<location path="tests/UnitTest/3rdparty/interface/ut_common.cpp" line_range="141-142" />
<code_context>
+
+TEST_F(UT_Common, test_isSupportSeek)
+{
+ bool result = m_tester->isSupportSeek("/tmp/testfile");
+ EXPECT_TRUE(result);
}
</code_context>
<issue_to_address>
**Seek test assumes writable temporary storage**
When the test environment makes `/tmp` unwritable or places it on a filesystem that does not support seeking, `isSupportSeek()` returns false when it cannot create and read back its temporary file, so the new assertion fails and the unit-test suite exits unsuccessfully.
Use a writable temporary directory created by the test and assert behavior appropriate to that environment.
</issue_to_address>
### Comment 5
<location path="tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp" line_range="97" />
<code_context>
+ ReadOnlyBackendCase{
+ "LibzipPlugin", makeLibzipPlugin,
+ globalFixture().makeZipArchive("readonly/zip/test.zip"),
+ true, true, true, false, false, true
+ },
+ ReadOnlyBackendCase{
</code_context>
<issue_to_address>
**Negative archive checks always skip**
When the parameterized read-only contract tests run, both `TestArchiveCorruptReturnsError` and `TestArchivePasswordProtected` skip when their capability flags are false, and every read-only backend sets both flags false. The suite therefore never exercises either negative `testArchive()` case, leaving those added checks with no coverage.
Enable these cases for a backend that supports the checks, or remove the skipped tests if no backend supports them.
Also at `tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp:102`, `tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp:107`, `tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp:112`, `tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp:117`, `tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp:122`.
</issue_to_address>
### Comment 6
<location path="tests/UnitTest/3rdparty/contract/ut_contract_readwrite.cpp" line_range="71" />
<code_context>
+ true, true, true, true, true
+ },
+ ReadWriteBackendCase{
+ "ReadWriteLibarchivePlugin", makeRwReadWriteLibarchivePlugin,
+ rw_globalFixture().makeTarArchive("readwrite/tar/test.tar"),
+ false, false, false, false, true
</code_context>
<issue_to_address>
**Read-write read operations lack contracts**
`ReadWriteLibarchivePlugin` is instantiated only in `ReadWriteContractTest`, whose tests cover its archive name, mutations, and inherited defaults but never call `list()`, `testArchive()`, or `extractFiles()`. Those implemented read operations receive no contract coverage for this backend.
Include `ReadWriteLibarchivePlugin` in the read-only contract suite, or add equivalent list, test, and extraction contract cases to the read-write suite.
</issue_to_address>
### Comment 7
<location path="tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp" line_range="317" />
<code_context>
+
+ DataManager::get_instance().resetArchiveData();
+ PluginFinishType result = m_iface->list();
+ EXPECT_EQ(result, PFT_Nomral);
+}
+
</code_context>
<issue_to_address>
**Partial extraction can silently do nothing**
When a backend returns `PFT_Nomral` without writing the selected entry, `ExtractPartialFiles` only checks that `extractFiles` returns `PFT_Nomral`; a backend that writes none of the selected entry still passes, so a broken partial-extraction path goes unnoticed.
Assert that the selected entry exists in the output directory and, for a regular file, that its content matches the archive.
</issue_to_address>
### Comment 8
<location path="tests/UnitTest/3rdparty/contract/ut_contract_readwrite.cpp" line_range="115-116" />
<code_context>
+
+ DataManager::get_instance().resetArchiveData();
+ PluginFinishType result = m_iface->list();
+ EXPECT_EQ(result, PFT_Nomral);
+}
+
</code_context>
<issue_to_address>
**Adding files can create an empty archive**
When `addFiles` creates an archive but omits the supplied file, `AddFilesCreatesArchive` checks only for `PFT_Nomral` and archive-path existence; an implementation that ignores the supplied file but creates an empty archive still passes, leaving file addition untested.
Reopen or list the created archive and assert that `test.txt` is present with the expected content.
</issue_to_address>|
|
||
| QProcess proc; | ||
| proc.setWorkingDirectory(m_tempDir->filePath("zip_src")); | ||
| proc.start("zip", QStringList() << "-r" << fullPath << "file1.txt" << "dir"); |
There was a problem hiding this comment.
🟡 Medium · Missing tools invalidate archive fixtures
When an external archive command is unavailable or fails, the archive fixture helpers return paths without confirming that QProcess started successfully, exited successfully, or produced valid archives. Tests then fail on missing fixtures, and wrong-password tests can pass without exercising password handling.
Generate the fixtures without relying on undeclared external commands, and verify each archive before tests use it.
Also at tests/UnitTest/3rdparty/contract/contract_test.h:159-161, tests/UnitTest/3rdparty/contract/contract_test.h:174-176, tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp:351.
Prompt for AI agents
In `tests/UnitTest/3rdparty/contract/contract_test.h` at line 143:
**Missing tools invalidate archive fixtures**
When an external archive command is unavailable or fails, the archive fixture helpers return paths without confirming that `QProcess` started successfully, exited successfully, or produced valid archives. Tests then fail on missing fixtures, and wrong-password tests can pass without exercising password handling.
Generate the fixtures without relying on undeclared external commands, and verify each archive before tests use it.
Also at `tests/UnitTest/3rdparty/contract/contract_test.h:159-161`, `tests/UnitTest/3rdparty/contract/contract_test.h:174-176`, `tests/UnitTest/3rdparty/contract/ut_contract_readonly.cpp:351`.
dengzhongyuan365-dev
force-pushed
the
agent/agent/d156040a0720
branch
2 times, most recently
from
November 13, 2026 08:01
24a1b78 to
dc24c01
Compare
dengzhongyuan365-dev
added a commit
to dengzhongyuan365-dev/deepin-compressor
that referenced
this pull request
Nov 13, 2026
1. Add contract tests for read-only and read-write archive plugin operations 2. Fix and enhance existing tests in cli7zplugin, clirarplugin, and libzipplugin 3. Add new tests for common utilities, data structures, and single-file plugins Log: No user-facing changes Influence: 1. Run ctest to verify all unit tests pass 2. Verify contract tests cover read-only and read-write plugin scenarios 3. Check that existing test fixes don't introduce regressions test: 补充三方归档接口及插件单元测试 1. 新增只读和读写归档插件操作的契约测试 2. 修正并增强 cli7zplugin、clirarplugin、libzipplugin 现有测试 3. 新增通用工具、数据结构和单文件插件测试 Log: 无用户可见变化 Influence: 1. 执行 ctest 验证所有单元测试通过 2. 验证契约测试覆盖只读和读写插件场景 3. 检查现有测试修正未引入回归 Issue: linuxdeepin#553
dengzhongyuan365-dev
added a commit
to dengzhongyuan365-dev/deepin-compressor
that referenced
this pull request
Nov 13, 2026
1. Capture asynchronous corrupt-archive failures through the contract helper. 2. Validate fixture file operations and fail fast when setup commands fail. 3. Cover ReadWriteLibarchive read paths and partial extraction content. Log: No user-facing changes. Influence: 1. Build and run contract_test. 2. Verify corrupt and password archive contracts. 3. Verify partial extraction and read-write backend regressions. Issue: linuxdeepin#553
dengzhongyuan365-dev
force-pushed
the
agent/agent/d156040a0720
branch
3 times, most recently
from
November 13, 2026 08:06
88c087b to
6b98d2d
Compare
1. Add contract tests for read-only and read-write archive plugin operations 2. Fix and enhance existing tests in cli7zplugin, clirarplugin, and libzipplugin 3. Add new tests for common utilities, data structures, and single-file plugins Influence: 1. Run ctest to verify all unit tests pass 2. Verify contract tests cover read-only and read-write plugin scenarios 3. Check that existing test fixes don't introduce regressions test: 补充三方归档接口及插件单元测试 1. 新增只读和读写归档插件操作的契约测试 2. 修正并增强 cli7zplugin、clirarplugin、libzipplugin 现有测试 3. 新增通用工具、数据结构和单文件插件测试 Influence: 1. 执行 ctest 验证所有单元测试通过 2. 验证契约测试覆盖只读和读写插件场景 3. 检查现有测试修正未引入回归 Log: No user-facing changes. PMS: TASK-398535
1. Capture asynchronous corrupt-archive failures through the contract helper. 2. Validate fixture file operations and fail fast when setup commands fail. 3. Cover ReadWriteLibarchive read paths and partial extraction content. Influence: 1. Build and run contract_test. 2. Verify corrupt and password archive contracts. 3. Verify partial extraction and read-write backend regressions. Log: No user-facing changes. PMS: TASK-398535
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ❌评价： 存在逻辑缺陷 ❌ 不通过 潜在问题： 建议： ['将 waitForFinishedSignal() 重构为在调用操作前连接信号，或合并到 runInterfaceOp() 中统一处理', 'ExtractPartialFiles 中将回退逻辑改为 GTEST_SKIP() 跳过测试，而非使用可能失败的回退', 'AddCommentOnNonSupportingBackendReturnsError 使用 runOp() 包装 addComment 调用以保持异步一致性'] 2. 代码质量 ❌评价： 存在重复代码，结构需要优化 ❌ 不通过 潜在问题： 建议： ['提取重复的 stub 设置代码为辅助函数如 setupOverwriteStubs(Stub &stub)', '注册 LibPigzPlugin 或移除其 GTEST_SKIP() 守卫', 'runInterfaceOp 超时时添加 ADD_FAILURE() 日志输出', '将魔法数字14替换为 nestedContent.toUtf8().size()'] 3. 代码性能 ✅评价： 性能良好，资源使用合理 ✅ 通过 潜在问题： 建议： ['将超时值提取为 constexpr int kAsyncTimeoutMs = 15000 常量'] 4. 代码安全 🔒评价： 存在0个安全漏洞 ✅ 通过
安全漏洞详情： 建议： ['无安全改进建议'] 💡 改进建议代码示例// 修复1: waitForFinishedSignal 竞态条件 - 合并到 runInterfaceOp 统一处理
// 修复2: ExtractPartialFiles 回退逻辑
// 修复3: AddCommentOnNonSupportingBackend 使用 runOp
// 修复2: ut_contract_readonly.cpp ExtractPartialFiles
TEST_P(ReadOnlyContractTest, ExtractPartialFiles)
{
if (!GetParam().supportsExtractFiles) GTEST_SKIP();
if (GetParam().name == "LibPigzPlugin") GTEST_SKIP();
runOp([this]() { return m_iface->list(); });
ArchiveData &data = DataManager::get_instance().archiveData();
ASSERT_FALSE(data.listRootEntry.isEmpty());
FileEntry target;
bool found = false;
for (const FileEntry &e : data.listRootEntry) {
if (!e.isDirectory) {
target = e;
found = true;
break;
}
}
if (!found) {
GTEST_SKIP() << "No non-directory entry found for partial extraction test";
}
ASSERT_TRUE(found);
ASSERT_FALSE(target.isDirectory);
// ... rest of test
}
// 修复3: ut_contract_readwrite.cpp AddCommentOnNonSupportingBackendReturnsError
TEST_P(ReadWriteContractTest, AddCommentOnNonSupportingBackendReturnsError)
{
if (GetParam().supportsAddComment) GTEST_SKIP();
PluginFinishType result = runOp([this]() {
return m_iface->addComment("should fail");
});
if (GetParam().unsupportedAddCommentReturnsError) {
EXPECT_EQ(result, PFT_Error);
} else {
EXPECT_EQ(result, PFT_Nomral);
}
}本报告由 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: No user-facing changes
Influence:
Summary by Sourcery
Strengthen third-party archive plugin validation with shared read-only and read-write contract tests and updated existing unit coverage.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: