test(validation): one bad cell must trip each rule, and nothing may mutate - #476
Merged
Merged
Conversation
…utate Three properties the suite did not assert directly. One-value violations. A rule tested only against a wholly-broken frame can pass while detecting something else, and a rule never tested against a clean frame can pass by always failing. Each of unique, allowed_values, range, nullable, max_missing_ratio, regex, compound_unique and min_rows is now asserted both ways: exactly one bad cell is detected, and the same rule stays silent on clean data. max_rows, a missing required column and strict_columns extras are covered too. All 16 already behave correctly. Non-mutation by digest. The existing checks use assert_frame_equal, which compares values and dtypes but ignores .attrs and the index name, so an in-place change to either would go unnoticed. These hash content, labels, dtypes, attrs and index name, and all seven of run_suite, validate_fields, profile, suggest_plan, infer_roles, explain_clean and clean leave the input byte-identical. Cross-field. The inventory recorded "no explicit min <= max test"; the capability exists through caller-supplied cross_rules callables, so this was a coverage gap rather than a missing feature. On a frame carrying one date inversion and one price inversion on the same row, both rules fire, only that row is reported, the action is manual_review rather than an automatic repair, and a row with a missing half of either pair is correctly not reported -- absence is not inversion. Remediation integrity. accepted + quarantined + rejected + needs_review equals the input row count, so no row is silently dropped; the original 'apple' stays recoverable from the quarantine frame; and the audit entry carries original, action, applied, classification and the reason "expected numeric in 'amount' but got text value 'apple'; not silently converted". The report's claim and the frames agree, which is the property worth guarding: row 1 is absent from accepted and present in quarantined. Every property already holds, so these are regression guards rather than bug reports. No library code changed. Full suite 6674 passed / 0 failed, coverage 93.91%; ruff clean repo-wide.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
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.
Why
Three properties the suite did not assert directly.
1. One-value violations
A rule tested only against a wholly-broken frame can pass while detecting something else entirely; a rule never tested against a clean frame can pass by always failing. Each rule is now asserted both ways.
uniqueallowed_valuesmin_value/max_valuenullable=Falsemax_missing_ratioregexcompound_uniquemin_rows/max_rowsstrict_columnsextrasAll 16 already behave correctly.
2. Non-mutation, by digest
The existing checks use
assert_frame_equal, which compares values and dtypes but ignores.attrsand the index name — an in-place change to either would go unnoticed. These hash content + labels + dtypes +attrs+ index name.run_suite,validate_fields,profile,suggest_plan,infer_roles,explain_cleanandcleanall leave the input byte-identical.3. Cross-field, including
min <= maxThe inventory recorded "no explicit min<=max test". The capability does exist, through caller-supplied
cross_rulescallables — so this was a coverage gap, not a missing feature.On a frame with one date inversion and one price inversion on the same row: both rules fire, only that row is reported, the action is
manual_reviewrather than an automatic repair, and a row with a missing half of either pair is correctly not reported — absence is not inversion.4. Remediation integrity
accepted + quarantined + rejected + needs_review == len(df)— no row silently dropped."apple"stays recoverable from the quarantine frame.original,action,applied,classificationand the reason "expected numeric in 'amount' but got text value 'apple'; not silently converted".accepted, present inquarantined.Verification
main, so these are regression guards rather than bug reports.ruff check .clean repo-wide.