Skip to content

fix(test): compare canonical snapshot paths - #3339

Merged
imbajin merged 1 commit into
apache:masterfrom
hugegraph:fix/test-snapshot-canonical-path
Oct 14, 2026
Merged

imbajin merged 1 commit into
apache:masterfrom
hugegraph:fix/test-snapshot-canonical-path

Conversation

@imbajin

@imbajin imbajin commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Purpose of the PR

Snapshot directory-sync tests fail on macOS when the same temporary directory is reported as both /var/... and /private/var/....

Main Changes

Compare canonical directory identities instead of lexical path spellings. The marker and directory-sync assertions remain in place; production snapshot behavior is unchanged.

Verifying these changes

  • Already covered by existing tests: RocksDBSnapshotRestoreTest reproduces the alias failure before the change and passes afterward.
  • Java 17 reactor compilation, editorconfig formatting and diff checks pass. Existing Linux bind-mount cases remain skipped on macOS.
  • Three independent reviewers found no blocking issue.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects
  • Nope

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need: test fixture only.

Documentation files in this PR or paired hugegraph-doc PR:

Conceptual overview

Conceptual test-only fix: resolve both directory paths before comparison; existing assertions and production behavior remain unchanged.

Conceptual illustration; not test or release-acceptance evidence.

Canonical snapshot directory identities across macOS path aliases

Compare real directory identities in the sync fixture.
Preserve snapshot marker and directory sync assertions.
Handle macOS temporary directory aliases.
@codecov

codecov Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.84%. Comparing base (0b7c0de) to head (ef80026).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3339      +/-   ##
============================================
- Coverage     54.29%   52.84%   -1.45%     
+ Complexity     2616        3    -2613     
============================================
  Files           770      503     -267     
  Lines         67952    48402   -19550     
  Branches       9213     6963    -2250     
============================================
- Hits          36892    25580   -11312     
+ Misses        27482    20196    -7286     
+ Partials       3578     2626     -952     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: The one-line change makes checkMarkerDirectorySync compare the real paths of the two directories instead of their spellings, which is the right fix for the macOS /var versus /private/var alias, and it does not weaken the check on Linux. I found no issues. Score 9/10. Evidence: read the full exact-head diff at ef80026 (1 file, +1/-1) and the surrounding test and production code. RocksDBSnapshotRestore builds its marker from new File(data).getCanonicalFile() (RocksDBSnapshotRestore.java:106, :311-312) and passes this.marker.getParent() to forceDirectory at :438, :526 and :571, so production always hands over the canonical parent, while the test derives marker from the TemporaryFolder path, which is not canonical on macOS. The compared directory is the TemporaryFolder root created at the top of the helper and it exists at all three calls, so toRealPath() cannot throw NoSuchFileException there. The other path equalities in the class (:1419, :1441) already use toRealPath(), and I found no other lexical comparison against a canonicalized production path. The class is part of UnitTestSuite (UnitTestSuite.java:257), and the server-rocksdb / server_rocksdb (Java 17) job passed at this head. I did not run the test on macOS myself. codecov/project reports a failure, which comes from the scoped CI run uploading fewer modules than the base (503 files against 770) and not from this change.

@imbajin
imbajin merged commit 4facf98 into apache:master Oct 14, 2026
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants