Tree diff tests#218
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors the dump-loading pipeline in hhds_tree_diff.cpp, consolidates data structures, extends print_tree to handle node attributes, and introduces a comprehensive test suite validating tree edit distance computation with fixture-based dump files and custom cost functions. ChangesTree Edit Distance Testing and Dump Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hhds/tests/hhds_tree_diff_test.cpp`:
- Around line 313-316: The test TEST(TreeDiff, MissingFile) currently uses a
hardcoded path which can be flaky; change it to generate a unique non-existent
path at runtime (e.g., build a temp filename with a random/UUID suffix or use
mkstemp to create-and-unlink a file name) and pass that to load_test_dump so the
path is guaranteed absent; update the test to obtain the path before calling
load_test_dump and assert EXPECT_EQ(d.tree, nullptr) as before, referencing the
TEST(TreeDiff, MissingFile) block, the load_test_dump call and the d.tree check.
- Around line 13-18: The helper write_temp_dump currently creates a
deterministic /tmp filename and only EXPECTs that the ofstream opened, which can
collide in concurrent tests and return a path even on failure; change
write_temp_dump to create a unique temp file (use mkstemp or std::filesystem
temp_directory_path + a unique suffix/UUID) so concurrent runs don't collide,
open the file via the file descriptor/unique name, check success and fail fast
on error (use ASSERT_TRUE or throw std::runtime_error) instead of EXPECT_TRUE,
write the content, flush/close the stream, and then return the unique path;
refer to write_temp_dump to find where to replace the filename generation,
open/check logic, and return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eccebae5-410a-496a-80e7-6202d1b3bb09
📒 Files selected for processing (3)
hhds/BUILD.bazelhhds/hhds_tree_diff.cpphhds/tests/hhds_tree_diff_test.cpp
1 participant
Summary
Adds a googletest-based test suite for
hhds_tree_diff, verifying dump file loading, type table construction, and tree edit distance computation on LNAST-style dump files.Files Added
hhds/tests/hhds_tree_diff_test.cpp— test suite with 8 test caseshhds/BUILD.bazel— addedcc_testtarget forhhds_tree_diff_testTest Cases
IdenticalTreesSwappedDeclarationsconst x/const yorder → distance 6SingleInsertionSymmetricDistanceSelfDistanceMissingFileTypeTableCorrectnessNodeCountadd_trivialdump produces exactly 33 nodesTest Output
Summary by CodeRabbit
Tests
Refactor