test: assert the suffix-distinct module names, not merely that they differ - #1436
test: assert the suffix-distinct module names, not merely that they differ#1436vitali87 wants to merge 6 commits into
Conversation
The suffix collision test checked only distinctness, which is the same predicate the fix establishes: a rule that mangled the stem would still produce two different names and pass. Now asserts the qualified names each file actually receives. Verified it fails against a deliberately mangled stem, which the uniqueness-only form accepted.
vitali87
added
the
claimed
|
@greptileai review |
|
Warning Review limit reachedNext included review available in 10 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe document-tier tests now cover heading whitespace normalization, empty-heading placeholders, and exact qualified names for files with different suffixes. ChangesDocument tier test validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized test-only change strengthens assertions for suffix-distinct module names without changing product behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the test weakness, the strengthened assertions, the deliberate failure check, and the test results. It includes Summary, Test Plan, and Type of Change sections. Related Issues and Checklist sections are missing, but the description is otherwise complete. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Found by mutation rather than inspection: replacing the whitespace collapse with a plain strip, and dropping the (untitled) fallback, both left all 34 tests passing. Two deliberate documented behaviours that nothing actually held in place. Killing a mutant does not require conceiving of the failure mode first, which is what makes it catch the case where a test was written from the same premise as the code.
|
@greptileai review |
Greptile SummaryThe PR strengthens document-tier tests by asserting exact qualified names and adds separate coverage for whitespace normalization, multiline setext headings, and untitled headings.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "test: exercise a heading whose text real..." | Re-trigger Greptile |
Another mutation survivor: flipping the default to True passed all 45 tests. The opt-in-ness is the whole reason the ast-grep tier's names are unchanged by #1428 — an argument I made in review and nothing enforced. Ruby still yields <project>.app, not app_rb. Changing that is #1429's job, deliberately and with its own review.
This reverts commit 4e45c74.
|
@greptileai review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The comment claimed the test protected identity for a heading reflowed across lines, but the fixture was one physical line with repeated spaces — it exercised space collapsing, not reflow. A setext heading's text genuinely spans lines, so that case now has its own test: the display name keeps the newline, the qualified name collapses it.
|
@greptileai review |
|
1 participant
Summary
test_same_stem_with_different_suffixes_stays_separateasserted only thatdocs/guide.mdanddocs/guide.markdownproduce two distinct qualified names. That is the same predicate the fix under test establishes, so the test could not fail for any reason the fix would cause.A rule that mangled the stem — prefixing it, truncating it, hashing it — would still produce two different names and pass.
Verified the test now catches that
Deliberately mangled the stem in
flat_module.py(guide_md→XXguide_md) and re-ran:Now asserts the actual qualified names each file receives, and the Section qns rooted under them.
Why this shape is worth watching for
The property I had in mind when writing the fix was uniqueness, so uniqueness was the property I tested. A test written from the same premise as the fix inherits its blind spots — it can only fail in ways the author already conceived of.
Credit to the
fix-doc-qualified-namesession, which found the identical shape in its own collision tests for #1429 and flagged the pattern.Test Plan
test_document_tier.py.Type of Change
Summary by CodeRabbit