Consolidate the join tests - #328
Open
amc-corey-cox wants to merge 5 commits into
Open
Conversation
The seven tests drove the private _perform_unit_conversion with MagicMock slot derivations and a mocked induced_slot, against the repo's own "never write mock tests" rule. They were not redundant, though: the compliance suite covers unit pairs exhaustively but always with a float source value, so six of the seven guarded behaviour nothing else touched — structured target slots, source-unit mismatch, numeric strings (8966fe7), non-numeric raising (328dc30), and the none_if_non_numeric opt-out. All are now driven through map_object with real schemas and specs. The rewrite also corrects what the mocks asserted: callers see TransformationError with row context, not the bare ValueError raised inside. Refs #298
Five of the seven tests in test_lookup_index_context_manager.py exercise engine.py's owns_index bookkeeping, not LookupIndex, so they belong under test_transformer/. The two that genuinely cover the context manager stay. Deliberately keeping test_lookup_index_edge_cases.py, which #298 proposed deleting: its coverage rows really are byte-identical, but it is the only place asserting close lifecycle (close clears tables, operations after close raise, double close is safe) and lookup_row's LIMIT 1 behaviour on duplicate keys. Those lines are executed by other tests with other data, which is why coverage can't see the loss. Refs #298
The successful conversions are now registered setups, so they run both in isolation and cumulatively in test_integration alongside every other scaffold feature. No inline schemas remain — the source/target patches and the spec fragment are all the tests carry. The two error cases stay unregistered, since a setup that raises would break the cumulative run for every feature. Fixes the ordering hazard that made this awkward. Parametrizing over the shared TEST_SETUP_FUNCTIONS only sees setups registered before the parametrizing module is imported, while integration_scaffold reads the list at fixture-execution time, so a setup defined in a later-collected module silently got integration coverage with no unit test. Verified: a probe setup registered from a later module left test_unit at 8 cases while breaking test_integration. make_setup_registry gives each module a local list that is complete by the time its own parametrize runs, so files unit-test exactly their own setups and integration still accumulates every file's. run_transformer moves to conftest rather than being duplicated. Refs #298
… claims Six fail-loud tests were a 3x2 matrix — enum, permissible-value and top-level slot derivations, each spelled as an expr reference and as a structural dotted populated_from — with identical bodies and assertions. Now one parametrized test, with the rationale from the two documented cases kept in its docstring. test_implicit_join_ambiguous_columns_resolved_when_one_non_id was a strict subset of test_implicit_join_resolves_via_engine: same transformer, same data, fewer assertions. It never checked the disambiguation it describes, so it now asserts the synthesized join key directly instead of re-checking resolved values. Keeps test_expr_implicit_join.py, which #298 proposed deleting as having zero unique coverage. Coverage really is identical without it, but it is the only test of expr-triggered synthesis on a flat top-level slot — the #275 case where the table appears solely inside the expression string. The neighbouring tests either nest the reference in a class_derivation for that table, or trigger synthesis through populated_from (#279). Documented in the module docstring. Refs #298
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the #298 “join consolidation” work by reducing duplication in join-synthesis tests, tightening one ambiguous-join test to assert the inferred join key directly, and documenting why a seemingly redundant expr-based join test remains valuable.
Changes:
- Consolidates six “fail loud” cases into one parametrized test covering both
exprand dottedpopulated_fromspellings. - Refactors the “strict subset” ambiguous-join test to assert
joins["Reading"].join_on == "subject_id"rather than rechecking already-covered runtime resolution. - Expands the module docstring in
test_expr_implicit_join.pyto justify keeping the scenario-based regression guard despite coverage parity.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_transformer/test_join_synthesis_completeness.py | Replaces multiple near-identical fail-loud tests with a single parametrized matrix covering equivalent spellings. |
| tests/test_transformer/test_implicit_cross_table_join.py | Updates the ambiguous-columns test to directly assert inferred join-key selection instead of duplicating engine-resolution assertions. |
| tests/test_transformer/test_expr_implicit_join.py | Adds explanatory documentation clarifying the unique scenario coverage of expr-only join synthesis. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+229
to
233
| joins = tr.derived_specification.class_derivations[0].joins | ||
|
|
||
| assert len(results) == 2 | ||
| # id columns are ambiguous (both tables have them), but subject_id is the join key | ||
| # The nested row's 'score' should be resolved | ||
| assert results[0]["observation"]["value"] == 95.5 | ||
| assert results[1]["observation"]["value"] == 88.0 | ||
| assert "Reading" in joins | ||
| assert joins["Reading"].join_on == "subject_id", "'id' is common to both tables but must be excluded" | ||
|
|
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.
Final #298 chunk. Stacked on #323 — base is
cut-mock-tests, so review only the last commit; it retargets to main automatically once #323 merges.The issue estimated ~610 lines cut here. The real answer is 134 deleted / 75 added, because two of its four claims didn't survive checking.
Parametrized: the six fail-loud tests ✅
A clean 3×2 matrix — enum derivations, permissible-value derivations and top-level slot derivations, each spelled as an
exprreference and as a structural dottedpopulated_from— with byte-identical bodies and the samepytest.raises(ValueError, match="cannot be joined"). Now one parametrized test with six ids. The rationale from the two that carried docstrings (parity between spellings; the structural counterpart to the #279 flat-slot fix) is preserved in the new docstring.Fixed rather than deleted: the "strict subset" test ✅→
test_implicit_join_ambiguous_columns_resolved_when_one_non_idreally was a strict subset oftest_implicit_join_resolves_via_engine: same transformer, same fixture, same spec, fewer assertions.But look at what it claims: that
idis excluded as an ambiguous identifier andsubject_idchosen as the join key. It never asserted that — only that values resolved, which is why it collapsed into a subset. It now asserts the synthesized key directly (joins["Reading"].join_on == "subject_id"), which is both non-redundant and what the docstring always promised. Deleting it would have lost the only check of join-key inference.Kept:
test_expr_implicit_join.py❌#298 called this 114 lines of "zero unique coverage". Coverage-wise that's true — I measured it, identical with and without (
4762 statements, 441 missed, 2096 branches, 235 partial).It is nonetheless the only test of its scenario. The neighbours differ in the trigger, not the assertion:
test_implicit_join_expr_dot_notationexprinside a nested class_derivation for Readingtest_flat_dot_notation_normalization_synthesizes_joinpopulated_from: Reading.score(#279)test_expr_reference_synthesizes_joinexpron a flat top-level slotThat last one is #275 exactly: "invisible to synthesis and silently resolved to None", and per its docstring the dominant production shape (bdc-harmonized-variables: 440 specs, zero
joins:blocks). Documented in the module docstring so the next audit doesn't re-propose it.Kept: the miss/sparse tests ❌
#298 wanted the seven "join miss → None" tests parametrized. They span four files with different fixtures and encode different semantics from #217 — scalar miss →
None, multivalued miss →[], nested object suppressed, plus the guard against silently resolving an ambiguous column to the parent. Parametrizing would conflate deliberately distinct behaviours. Two of them also live intest_join_engine.pyandtest_cross_table_lookup.py, which #298 itself says stay.Verification
1102 passed, 4 skipped, 1 xfailed — unchanged. ruff and format clean.