Guard the rapidfuzz-dependent assertion in test_validate - #1705
Open
sbryngelson wants to merge 1 commit into
Open
Guard the rapidfuzz-dependent assertion in test_validate#1705sbryngelson wants to merge 1 commit into
sbryngelson wants to merge 1 commit into
Conversation
…s_targeted_error test_family_attr_typo_gives_targeted_error asserted that 'geometry' appears in the valid-attribute list for a patch_ib(1)%geometri typo. That only holds when rapidfuzz is installed: _family_attr_error orders candidates by similarity before truncating the list at 8 entries, and without rapidfuzz suggest_similar returns an empty list, so the ordering falls back to alphabetical and patch_ib's 26 attributes push 'geometry' past the cut. The test therefore failed rather than skipped in environments without the optional dependency, unlike the two neighbouring tests in the same class that already carry @unittest.skipUnless(RAPIDFUZZ_AVAILABLE, ...). Split the similarity-ordering assertion into its own guarded test and leave the rapidfuzz-independent assertions (targeted error, no 'Did you mean') running unconditionally, so coverage of the base path is not lost.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a unit-test robustness issue in the Python parameter-validation test suite by ensuring an assertion that depends on the optional rapidfuzz dependency is only evaluated when rapidfuzz is available, while keeping the rapidfuzz-independent coverage in place.
Changes:
- Removes the unguarded assertion that
"geometry"appears in the truncated “Valid attributes …” list for a family-attribute typo. - Adds a new
@unittest.skipUnless(RAPIDFUZZ_AVAILABLE, ...)test that asserts"geometry"is present when similarity ordering (viarapidfuzz) is available.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+91
to
+93
| def test_family_attr_typo_lists_intended_attr_first(self): | ||
| """The intended attribute must survive truncation of the valid-attribute list. | ||
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1705 +/- ##
=======================================
Coverage 60.77% 60.77%
=======================================
Files 83 83
Lines 20872 20872
Branches 3101 3101
=======================================
Hits 12685 12685
Misses 6121 6121
Partials 2066 2066 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
test_family_attr_typo_gives_targeted_errorfails rather than skips whenrapidfuzzis not importable.Cause
The test feeds
patch_ib(1)%geometritocheck_unknown_paramsand asserts thatgeometryappears in the resulting "Valid attributes: ..." list._family_attr_error(toolchain/mfc/params/validate.py:78-90) orders candidates by similarity and then truncates at 8 entries:suggest.pydegrades gracefully when the import fails —RAPIDFUZZ_AVAILABLE = Falseandsuggest_similarreturns[]— so the ordering falls back to plain alphabetical.patch_ibnow has 26 attributes andgeometrysits past the 8-entry cut behindairfoil_id, angles(1..3), angular_vel(1..3), burn_rate_exp, so the assertion fails.rapidfuzzis a declared dependency intoolchain/pyproject.toml, so a properly provisioned environment always has it and CI is unaffected. Butsuggest.pysupports running without it, and the two neighbouring tests in the same class that depend on fuzzy matching already carry@unittest.skipUnless(RAPIDFUZZ_AVAILABLE, "rapidfuzz not installed"). This one was missed, so a partial environment gets a failure instead of a skip.Change
Split the similarity-ordering assertion into its own guarded test. The rapidfuzz-independent assertions — one error, "Valid attributes" present, no "Did you mean" — stay unguarded so the targeted-error path keeps its coverage either way.
Verification