test: fix relative paths in test cases #325
Merged
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
Signed-off-by: ComixHe <heyuming@deepin.org>
Reviewer's GuideRefactors test data path handling in several tests to use directory traversal from the build directory and QDir’s absolute paths, ensuring correct relative paths when tests are run from different working directories. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这段代码主要修改了测试用例中获取资源文件路径的方式，从手动拼接字符串改为使用 1. 总体评价这次修改的主要目的是提高代码的可移植性和健壮性。原代码通过字符串拼接（ 2. 代码审查与改进意见A. 语法逻辑
B. 代码质量
C. 代码性能
D. 代码安全
3. 具体文件审查细节
|
![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 3 issues, and left some high level feedback:
- The repeated
cdUp()/cd(<subdir>)sequences to locatetests/dataandtoolsdirectories could be centralized into a small helper (e.g.testRootDir()ortestDataDir()), which would reduce duplication and make future path changes easier to manage. - These path calculations still assume the test working directory is two levels under the repository root; if the tests are ever run from a different CWD this will break, so consider basing paths on a more stable reference (e.g. a CMake-configured test data root or the executable location) instead of
QDir::current().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated `cdUp()`/`cd(<subdir>)` sequences to locate `tests/data` and `tools` directories could be centralized into a small helper (e.g. `testRootDir()` or `testDataDir()`), which would reduce duplication and make future path changes easier to manage.
- These path calculations still assume the test working directory is two levels under the repository root; if the tests are ever run from a different CWD this will break, so consider basing paths on a more stable reference (e.g. a CMake-configured test data root or the executable location) instead of `QDir::current()`.
## Individual Comments
### Comment 1
<location> `tests/ut_desktopentry.cpp:19-23` </location>
<code_context>
- auto curDir = QDir::current();
- QByteArray fakeXDG = (curDir.absolutePath() + QDir::separator() + "data").toLocal8Bit();
- ASSERT_TRUE(qputenv("XDG_DATA_DIRS", fakeXDG));
+ auto fakeXDG = QDir::current();
+ ASSERT_TRUE(fakeXDG.cdUp());
+ ASSERT_TRUE(fakeXDG.cdUp());
+ ASSERT_TRUE(fakeXDG.cd("tests/data"));
+ ASSERT_TRUE(qputenv("XDG_DATA_DIRS", fakeXDG.absolutePath().toLocal8Bit()));
ParserError err;
auto file = DesktopFile::searchDesktopFileById("deepin-editor", err);
</code_context>
<issue_to_address>
**suggestion (testing):** Consider using a shared helper or QFINDTESTDATA-style lookup for the test data root instead of hardcoding `cdUp()` hops.
These `cdUp()` calls still rely on a fixed directory layout (two levels up, then `tests/data`), which can break under different IDE/CTest/build setups. Centralizing the lookup in a helper (e.g. `QDir testDataRoot()`) or using `QFINDTESTDATA` would make the tests more portable and avoid duplicating this logic later in the file.
Suggested implementation:
```cpp
static QDir testDataRoot()
{
const QString marker = QLatin1String("tests/data");
#ifdef QFINDTESTDATA
// Try to locate the tests/data directory using QFINDTESTDATA in a
// build-system independent way. We use "." as a neutral marker and
// then walk up to find a matching tests/data directory.
const QString candidate = QFINDTESTDATA(".");
if (!candidate.isEmpty()) {
QDir dir(candidate);
const int idx = candidate.indexOf(marker);
if (idx != -1) {
// If the resolved path already contains "tests/data", cut it there.
return QDir(candidate.left(idx + marker.size()));
}
// Otherwise walk upwards until we find a tests/data directory.
while (!dir.isRoot()) {
QDir probe(dir);
if (probe.cd(marker)) {
return probe;
}
dir.cdUp();
}
}
#endif
// Fallback: walk up from the current directory until we find tests/data.
QDir dir = QDir::current();
while (!dir.isRoot()) {
QDir probe(dir);
if (probe.cd(marker)) {
return probe;
}
dir.cdUp();
}
// As a last resort, return the current directory (tests may fail visibly).
return QDir::current();
}
static void SetUpTestCase()
```
```cpp
env = qgetenv("XDG_DATA_DIRS");
const auto fakeXDG = testDataRoot();
ASSERT_TRUE(qputenv("XDG_DATA_DIRS", fakeXDG.absolutePath().toLocal8Bit()));
```
```cpp
auto desktopFileDir = testDataRoot();
```
1. Ensure that this file includes the necessary Qt headers if they are not already present:
- `<QDir>`
- `<QString>`
- `<QTest>` (or a header that provides the `QFINDTESTDATA` macro).
2. If your project’s QtTest setup doesn’t define `QFINDTESTDATA` in this compilation unit, either:
- Add the proper QtTest include, or
- Remove/adjust the `#ifdef QFINDTESTDATA` block and rely solely on the fallback directory walk as appropriate for your environment.
3. If your actual test data root is not literally `tests/data` or uses a different relative structure, update the `marker` constant in `testDataRoot()` accordingly so it resolves the correct directory.
</issue_to_address>
### Comment 2
<location> `tests/ut_desktopentry.cpp:47-51` </location>
<code_context>
- auto curDir = QDir::current();
- QString path{curDir.absolutePath() + QDir::separator() + "data" + QDir::separator() + "applications" + QDir::separator() +
- "deepin-editor.desktop"};
+ auto desktopFileDir = QDir::current();
+ ASSERT_TRUE(desktopFileDir.cdUp());
+ ASSERT_TRUE(desktopFileDir.cdUp());
+ ASSERT_TRUE(desktopFileDir.cd("tests/data/applications"));
+ const auto& path = desktopFileDir.absoluteFilePath("deepin-editor.desktop");
EXPECT_EQ(exampleFile->sourcePath(), path);
EXPECT_EQ(exampleFile->desktopId().toStdString(), "deepin-editor");
</code_context>
<issue_to_address>
**suggestion:** Reuse the same data-root resolution as in `SetUpTestCase` to keep the desktop file path logic consistent.
This test now resolves the path differently from `SetUpTestCase` (manual `cdUp()` twice into `tests/data/applications` vs. using the shared data-root logic). If the directory layout changes, both places must be updated in lockstep. Consider reusing the same helper or deriving `applications` from the directory used in `SetUpTestCase` to avoid divergence between setup and the expected `sourcePath()` value.
Suggested implementation:
```cpp
const auto &fileptr = file();
ASSERT_FALSE(fileptr.isNull());
const auto &exampleFile = file();
// Resolve the desktop file path based on the same data-root used to set XDG_DATA_DIRS.
QDir applicationsDir{fakeXDG};
ASSERT_TRUE(applicationsDir.cd("applications"));
const auto &path = applicationsDir.absoluteFilePath("deepin-editor.desktop");
EXPECT_EQ(exampleFile->sourcePath(), path);
EXPECT_EQ(exampleFile->desktopId().toStdString(), "deepin-editor");
}
```
If `fakeXDG` is not the same directory used in `SetUpTestCase` as the data-root, replace `fakeXDG` with the actual data-root `QDir` (e.g., a static member initialized in `SetUpTestCase`) so that both the environment (`XDG_DATA_DIRS`) and the expected `sourcePath()` derive from the same root.
</issue_to_address>
### Comment 3
<location> `tests/ut_hook.cpp:12-17` </location>
<code_context>
{
- auto file =
- QDir::currentPath() + QDir::separator() + "data" + QDir::separator() + "hooks.d" + QDir::separator() + "1-test.json";
+ auto hooksDir = QDir::current();
+ ASSERT_TRUE(hooksDir.cdUp());
+ ASSERT_TRUE(hooksDir.cdUp());
+ ASSERT_TRUE(hooksDir.cd("tests/data/hooks.d"));
+
+ auto file = hooksDir.absoluteFilePath("1-test.json");
auto hook = ApplicationHook::loadFromFile(file);
EXPECT_TRUE(hook);
</code_context>
<issue_to_address>
**suggestion (testing):** Make hook test data lookup resilient to different working directories (e.g. via a shared helper or QFINDTESTDATA).
This test currently assumes the process starts two levels below the project root and that `tests/data/hooks.d` exists there, which can be brittle under different runners. Consider resolving `1-test.json` via a shared `testDataRoot()` helper (as used in other tests) or a `QFINDTESTDATA`-style approach so it no longer depends on the initial working directory.
Suggested implementation:
```cpp
#include "applicationHooks.h"
#include <gtest/gtest.h>
#include <QDir>
#include <QStringList>
#include <QtTest/QTest>
```
```cpp
TEST(ApplicationHookTest, load)
{
const auto file = QFINDTESTDATA("data/hooks.d/1-test.json");
ASSERT_FALSE(file.isEmpty());
auto hook = ApplicationHook::loadFromFile(QString::fromUtf8(file));
```
To fully support this change, ensure that `data/hooks.d/1-test.json` is correctly registered as test data in your build system so that `QFINDTESTDATA("data/hooks.d/1-test.json")` can locate it. For example, with qmake you would add it to `TESTDATA += data/hooks.d/1-test.json`, and with CMake/Qt6 you would typically use `qt_add_test()` / `qt_add_resources()` or `set_tests_properties()` equivalents to install or expose the test data at runtime.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+19
to
+23
| auto fakeXDG = QDir::current(); | ||
| ASSERT_TRUE(fakeXDG.cdUp()); | ||
| ASSERT_TRUE(fakeXDG.cdUp()); | ||
| ASSERT_TRUE(fakeXDG.cd("tests/data")); | ||
| ASSERT_TRUE(qputenv("XDG_DATA_DIRS", fakeXDG.absolutePath().toLocal8Bit())); |
There was a problem hiding this comment.
suggestion (testing): Consider using a shared helper or QFINDTESTDATA-style lookup for the test data root instead of hardcoding cdUp() hops.
These cdUp() calls still rely on a fixed directory layout (two levels up, then tests/data), which can break under different IDE/CTest/build setups. Centralizing the lookup in a helper (e.g. QDir testDataRoot()) or using QFINDTESTDATA would make the tests more portable and avoid duplicating this logic later in the file.
Suggested implementation:
static QDir testDataRoot()
{
const QString marker = QLatin1String("tests/data");
#ifdef QFINDTESTDATA
// Try to locate the tests/data directory using QFINDTESTDATA in a
// build-system independent way. We use "." as a neutral marker and
// then walk up to find a matching tests/data directory.
const QString candidate = QFINDTESTDATA(".");
if (!candidate.isEmpty()) {
QDir dir(candidate);
const int idx = candidate.indexOf(marker);
if (idx != -1) {
// If the resolved path already contains "tests/data", cut it there.
return QDir(candidate.left(idx + marker.size()));
}
// Otherwise walk upwards until we find a tests/data directory.
while (!dir.isRoot()) {
QDir probe(dir);
if (probe.cd(marker)) {
return probe;
}
dir.cdUp();
}
}
#endif
// Fallback: walk up from the current directory until we find tests/data.
QDir dir = QDir::current();
while (!dir.isRoot()) {
QDir probe(dir);
if (probe.cd(marker)) {
return probe;
}
dir.cdUp();
}
// As a last resort, return the current directory (tests may fail visibly).
return QDir::current();
}
static void SetUpTestCase()
env = qgetenv("XDG_DATA_DIRS");
const auto fakeXDG = testDataRoot();
ASSERT_TRUE(qputenv("XDG_DATA_DIRS", fakeXDG.absolutePath().toLocal8Bit()));
auto desktopFileDir = testDataRoot();
- Ensure that this file includes the necessary Qt headers if they are not already present:
<QDir><QString><QTest>(or a header that provides theQFINDTESTDATAmacro).
- If your project’s QtTest setup doesn’t define
QFINDTESTDATAin this compilation unit, either:- Add the proper QtTest include, or
- Remove/adjust the
#ifdef QFINDTESTDATAblock and rely solely on the fallback directory walk as appropriate for your environment.
- If your actual test data root is not literally
tests/dataor uses a different relative structure, update themarkerconstant intestDataRoot()accordingly so it resolves the correct directory.
Comment on lines
+47
to
+51
| auto desktopFileDir = QDir::current(); | ||
| ASSERT_TRUE(desktopFileDir.cdUp()); | ||
| ASSERT_TRUE(desktopFileDir.cdUp()); | ||
| ASSERT_TRUE(desktopFileDir.cd("tests/data/applications")); | ||
| const auto& path = desktopFileDir.absoluteFilePath("deepin-editor.desktop"); |
There was a problem hiding this comment.
suggestion: Reuse the same data-root resolution as in SetUpTestCase to keep the desktop file path logic consistent.
This test now resolves the path differently from SetUpTestCase (manual cdUp() twice into tests/data/applications vs. using the shared data-root logic). If the directory layout changes, both places must be updated in lockstep. Consider reusing the same helper or deriving applications from the directory used in SetUpTestCase to avoid divergence between setup and the expected sourcePath() value.
Suggested implementation:
const auto &fileptr = file();
ASSERT_FALSE(fileptr.isNull());
const auto &exampleFile = file();
// Resolve the desktop file path based on the same data-root used to set XDG_DATA_DIRS.
QDir applicationsDir{fakeXDG};
ASSERT_TRUE(applicationsDir.cd("applications"));
const auto &path = applicationsDir.absoluteFilePath("deepin-editor.desktop");
EXPECT_EQ(exampleFile->sourcePath(), path);
EXPECT_EQ(exampleFile->desktopId().toStdString(), "deepin-editor");
}
If fakeXDG is not the same directory used in SetUpTestCase as the data-root, replace fakeXDG with the actual data-root QDir (e.g., a static member initialized in SetUpTestCase) so that both the environment (XDG_DATA_DIRS) and the expected sourcePath() derive from the same root.
Comment on lines
+12
to
+17
| auto hooksDir = QDir::current(); | ||
| ASSERT_TRUE(hooksDir.cdUp()); | ||
| ASSERT_TRUE(hooksDir.cdUp()); | ||
| ASSERT_TRUE(hooksDir.cd("tests/data/hooks.d")); | ||
|
|
||
| auto file = hooksDir.absoluteFilePath("1-test.json"); |
There was a problem hiding this comment.
suggestion (testing): Make hook test data lookup resilient to different working directories (e.g. via a shared helper or QFINDTESTDATA).
This test currently assumes the process starts two levels below the project root and that tests/data/hooks.d exists there, which can be brittle under different runners. Consider resolving 1-test.json via a shared testDataRoot() helper (as used in other tests) or a QFINDTESTDATA-style approach so it no longer depends on the initial working directory.
Suggested implementation:
#include "applicationHooks.h"
#include <gtest/gtest.h>
#include <QDir>
#include <QStringList>
#include <QtTest/QTest>
TEST(ApplicationHookTest, load)
{
const auto file = QFINDTESTDATA("data/hooks.d/1-test.json");
ASSERT_FALSE(file.isEmpty());
auto hook = ApplicationHook::loadFromFile(QString::fromUtf8(file));
To fully support this change, ensure that data/hooks.d/1-test.json is correctly registered as test data in your build system so that QFINDTESTDATA("data/hooks.d/1-test.json") can locate it. For example, with qmake you would add it to TESTDATA += data/hooks.d/1-test.json, and with CMake/Qt6 you would typically use qt_add_test() / qt_add_resources() or set_tests_properties() equivalents to install or expose the test data at runtime.
BLumia
approved these changes
Jan 28, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, ComixHe 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 |
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.
Summary by Sourcery
Adjust test utilities to resolve data and tools directories via QDir navigation to stable absolute paths.
Tests: