test(metamorphic): assert what cleaning must and must not change - #475
Merged
Merged
Conversation
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 |
The suite had no invariance tests on the public API. Worse,
tests/test_execution/test_action_parity.py::_normalize sorts rows *and* columns
before comparing engines, so an order-sensitivity bug in fd.clean would pass
that gate silently. These properties are asserted directly, on unsorted output.
Invariances -- reordering rows (4 seeds) or columns, adding an unrelated
column, renaming a column whose meaning is pinned by semantic_context, and
cleaning twice must change no surviving cell. Row order is also checked against
imputation specifically, since impute reads the whole column and is the most
plausible place for a positional accident to leak in; mean and missforest are
both covered.
Required changes -- replacing a valid amount with "apple", replacing a country
with "N/A", and flagging an outlier must each change the result. Without these
a cleaner that returned its input untouched would satisfy every invariance
above and look perfect. The outlier case asserts both halves: the value stays
9999.0 and the report still records the detection.
Every property already holds on main, so these are guards against regression
rather than bug reports. The engine-parity case is the exception in spirit: it
covers ground test_action_parity cannot, because it compares the frames as
returned instead of sorting them first. Both polars and duckdb run it; neither
is skipped in CI.
Two things that looked like defects and were not, recorded here so the next
reader does not re-derive them:
- assert_frame_equal failed with an EMPTY message while every column, dtype,
index and shape matched. fd.clean returns CleanResult, whose _constructor is
pd.DataFrame by design (result.py:22), so .reindex() yields a plain frame and
the comparison failed on frame *type*. That design is right -- a derived or
filtered frame should not carry a .report() that no longer describes it -- so
the helper normalises both sides instead.
- A 2-vs-2 categorical variant tie ('USA' vs 'usa') is not normalised in either
input order, so there is no order-dependent tie-break to guard against.
Full suite 6656 passed / 0 failed, coverage 93.91%; ruff clean.
kevincostner17
force-pushed
the
test/metamorphic
branch
from
September 16, 2026 20:00
daccb13 to
4f18b49
Compare
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
The suite had no invariance tests on the public API. Worse,
tests/test_execution/test_action_parity.py::_normalizesorts rows and columns before comparing engines, so an order-sensitivity bug infd.cleanwould pass that gate silently.Invariances
Reordering rows (4 seeds) or columns, adding an unrelated column, renaming a column whose meaning is pinned by
semantic_context, and cleaning twice must change no surviving cell.Row order is checked against imputation specifically —
imputereads the whole column and is the most plausible place for a positional accident to leak in. Bothmeanandmissforestare covered.Required changes
Without these, a cleaner that returned its input untouched would satisfy every invariance above and look perfect:
"apple"changes the result"N/A"changes the resultoutliers="flag"flags and leaves the value at9999.0The outlier case asserts both halves deliberately: silence would be a missed detection, a changed value would be an unrequested repair.
Honest framing
Every property already holds on
main, so these are guards against regression rather than bug reports. The engine-parity case is the exception in spirit — it covers groundtest_action_paritycannot, because it compares the frames as returned instead of sorting them first. Both polars and duckdb run it; neither is skipped.Two things that looked like defects and were not
Recorded in the test docstrings so the next reader doesn't re-derive them:
assert_frame_equalfailed with an empty message while every column, dtype, index and shape matched.fd.cleanreturnsCleanResult, whose_constructorispd.DataFrameby design (result.py:22), so.reindex()yields a plain frame and the comparison failed on frame type. That design is right — a derived or filtered frame should not carry a.report()that no longer describes it — so the helper normalises both sides instead.'USA'vs'usa') is not normalised in either input order, so there is no order-dependent tie-break to guard against.Verification
ruff check .clean.