DEV-1786: PR #286 review G4 — test hardening (crossing discrimination, vacuous passes, brittle asserts) - #303
Merged
ZmeiGorynych merged 3 commits intoAug 12, 2026
Conversation
Pure test-side changes from the CodeRabbit triage of umbrella PR #286: make weak/vacuous tests discriminate the behaviour they claim to pin, plus mechanical cleanups (hoist imports, rename stale classes/methods, use shared helpers, close a connection on setup failure). Notable deviations from the literal review items: - Item 9: InKey.values is Tuple[LiteralKey, ...], so a crossing ColumnKey cannot go there; the real vacuity is fixed via the InKey column arm instead. - Item 15: the multiple `) AS _src` closes are siblings (multiple windowed measures), never nested; applied as last-match hardening. - Item 24c already used ScopeFrame.__new__; no change. - Item 27: narrowed the coverage claim — the defensive _forward_only fallbacks are internal invariants unreachable from a well-formed query. Vacuous passes for items 1/4/6/7/9/12 were demonstrated (sabotage → current test still passes) before closing. Codex review folded in: tightened the golden-harness path redaction to spare URLs/compact division (+unit test), and made the except-form guard permit re-raising handlers. Full non-integration suite green (11621 passed); ruff clean.
Contributor
📝 WalkthroughWalkthroughThe pull request updates test infrastructure and regression coverage. It improves SQLite cleanup, SQL parsing, exception-path redaction, structural SQL assertions, order-only behavior checks, isolation coverage, and rerooting validation. ChangesTest coverage and harness updates
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
- Sonar S9073 (test_dev1745_golden_sql.py): split the composite path-redaction assertion into separate asserts. - Codex (test_dev1747_reroot_filter_routing.py): the re-raise guard now requires a TOP-LEVEL raise in the handler body — a raise buried in a conditional branch no longer counts as non-swallowing. - Codex (_golden_harness.py): broaden the path-redaction regex to also collapse single-segment mounts (e.g. /workspace), still sparing URLs and compact SQL division; extend the unit test to cover it.
Member
Author
|
@coderabbitai review |
Contributor
|
…ion-dev-1450' of https://github.com/MotleyAI/slayer into egor/dev-1786-pr-286-review-g4-test-hardening-crossing-discrimination # Conflicts: # tests/test_dev1745_reachability.py
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_dev1745_reachability.py (1)
529-532: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove these imports to the module import block.
The coding guideline requires imports at the top of Python files. Import
filter_reachability_forandrecompute_filter_reachabilitywith the other module imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_dev1745_reachability.py` around lines 529 - 532, Move the filter_reachability_for and recompute_filter_reachability imports from the local scope into the module-level import block, keeping their existing usage unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_dev1745_fragment_joins.py`:
- Around line 159-171: Strengthen both tests in the fragment-join test class by
asserting the complete entered-fragment result equals ["regions.weight"], while
retaining the existing assertions that the marker values are absent. Update the
assertions in the window/90d and fmt/%Y-%m test cases; do not alter
_entered_fragments behavior.
---
Nitpick comments:
In `@tests/test_dev1745_reachability.py`:
- Around line 529-532: Move the filter_reachability_for and
recompute_filter_reachability imports from the local scope into the module-level
import block, keeping their existing usage unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef6da37f-6764-4ec5-b074-768939d836a6
📒 Files selected for processing (22)
tests/_dev1746_fixtures.pytests/_engine_helpers.pytests/_golden_harness.pytests/test_agg_render_spec.pytests/test_cross_model_rename_dev1448.pytests/test_dev1712_order_only_hidden_slots.pytests/test_dev1733_order_only_transform_composite.pytests/test_dev1744_naming_allocator.pytests/test_dev1744_value_expr.pytests/test_dev1745_fragment_joins.pytests/test_dev1745_golden_sql.pytests/test_dev1745_mode_a_door.pytests/test_dev1745_plan_time_routing.pytests/test_dev1745_reachability.pytests/test_dev1746_isolation_classifier.pytests/test_dev1747_golden_sql.pytests/test_dev1747_local_with_chain.pytests/test_dev1747_prebound_planner.pytests/test_dev1747_reroot_filter_routing.pytests/test_dev1747_reroot_visitor.pytests/test_filtered_local_isolation.pytests/test_sql_generator.py
Comment on lines
+159
to
+171
| entered = self._entered_fragments( | ||
| kwargs=(("window", "90d"),), agg="wscaled_sum", | ||
| ) | ||
| assert "90d" not in entered, entered | ||
|
|
||
| def test_marker_that_is_not_parseable_sql_is_still_skipped(self) -> None: | ||
| """The failure this guards: a marker whose text sqlglot rejects. It | ||
| must never reach the door, which raises.""" | ||
| assert self._entered_fragments(kwargs=(("fmt", "%Y-%m"),)) == [] | ||
| """The failure this guards: a marker whose text sqlglot rejects. Under a | ||
| TEMPLATED aggregation the substitution filter is what skips it, so it | ||
| never reaches the door (which would raise).""" | ||
| entered = self._entered_fragments( | ||
| kwargs=(("fmt", "%Y-%m"),), agg="wscaled_sum", | ||
| ) | ||
| assert "%Y-%m" not in entered, entered |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the default fragment is still scanned.
Both tests only assert that the marker value is absent. A regression that skips all fragment scanning when any marker exists will pass both tests. Assert entered == ["regions.weight"] in each case.
Proposed fix
entered = self._entered_fragments(
kwargs=(("window", "90d"),), agg="wscaled_sum",
)
- assert "90d" not in entered, entered
+ assert entered == ["regions.weight"], entered
...
entered = self._entered_fragments(
kwargs=(("fmt", "%Y-%m"),), agg="wscaled_sum",
)
- assert "%Y-%m" not in entered, entered
+ assert entered == ["regions.weight"], entered📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| entered = self._entered_fragments( | |
| kwargs=(("window", "90d"),), agg="wscaled_sum", | |
| ) | |
| assert "90d" not in entered, entered | |
| def test_marker_that_is_not_parseable_sql_is_still_skipped(self) -> None: | |
| """The failure this guards: a marker whose text sqlglot rejects. It | |
| must never reach the door, which raises.""" | |
| assert self._entered_fragments(kwargs=(("fmt", "%Y-%m"),)) == [] | |
| """The failure this guards: a marker whose text sqlglot rejects. Under a | |
| TEMPLATED aggregation the substitution filter is what skips it, so it | |
| never reaches the door (which would raise).""" | |
| entered = self._entered_fragments( | |
| kwargs=(("fmt", "%Y-%m"),), agg="wscaled_sum", | |
| ) | |
| assert "%Y-%m" not in entered, entered | |
| entered = self._entered_fragments( | |
| kwargs=(("window", "90d"),), agg="wscaled_sum", | |
| ) | |
| assert entered == ["regions.weight"], entered | |
| def test_marker_that_is_not_parseable_sql_is_still_skipped(self) -> None: | |
| """The failure this guards: a marker whose text sqlglot rejects. Under a | |
| TEMPLATED aggregation the substitution filter is what skips it, so it | |
| never reaches the door (which would raise).""" | |
| entered = self._entered_fragments( | |
| kwargs=(("fmt", "%Y-%m"),), agg="wscaled_sum", | |
| ) | |
| assert entered == ["regions.weight"], entered |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_dev1745_fragment_joins.py` around lines 159 - 171, Strengthen both
tests in the fragment-join test class by asserting the complete entered-fragment
result equals ["regions.weight"], while retaining the existing assertions that
the marker values are absent. Update the assertions in the window/90d and
fmt/%Y-%m test cases; do not alter _entered_fragments behavior.
ZmeiGorynych
merged commit Aug 12, 2026
ff2654c
into
egor/dev-1742-one-doctrine-consolidation-of-sql-generation-dev-1450
4 checks passed
ZmeiGorynych
added a commit
that referenced
this pull request
Aug 12, 2026
Siblings G1 (#300), G3 (#301), G4 (#303) landed on the umbrella. One conflict in slayer/engine/isolation.py: kept G1's DEV-1783 union body + precise return type for _crossing_input_paths, applied this branch's typed `bundle` (ResolvedSourceBundle) over G1's `Any`, and reconciled the typing import (TYPE_CHECKING + List, dropping the now-unused Any). Full non-integration suite green (11645 passed).
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
1 participant
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.
Group 4 of 4 from the CodeRabbit review triage of umbrella PR #286. Pure test-side changes — no production code touched. PRs back into the umbrella branch
egor/dev-1742-….What
All 29 review items addressed — strengthen weak/vacuous tests so they discriminate the behaviour they claim to pin, plus mechanical cleanups (hoist imports, rename stale classes/methods, use shared helpers, close a connection on setup failure).
Per the issue rules, each claimed-vacuous test (items 1, 4, 6, 7, 9, 12) was first demonstrated to pass vacuously (sabotage the asserted condition, watch it still pass), then closed so it now discriminates.
Notable deviations from the literal review items
InKey.valuesisTuple[LiteralKey, ...], so a crossingColumnKeycannot be placed there (the reviewer's suggestion is type-infeasible). Fixed the real vacuity via the InKey column arm instead.) AS _srccloses are siblings (multiple windowed measures), never nested; CodeRabbit's "nested" rationale is inaccurate. Applied as harmless last-match hardening with a corrected comment.ScopeFrame.__new__; no change needed.asyncio_mode=autoruns it); still did the sync-def+ stale-docstring cleanup.Design decisions
TestCrossingFirstLastStaysRanked. The reviewer's literal fix (assert the crossed hop injoin_chain) is impossible: host-rooted ranked plans always carryjoin_chain=[]/target_path=(). Only the structural-time-arg case exposes the crossing at plan level (ranking_time_key.path); the other two are pinned at render level by existing DEV-1748 tests (cross-referenced)._forward_onlyabandon branches) — narrowed the claim: the common forward-abandon IS exercised; the late fallbacks are defensive internal invariants unreachable from a well-formed query.Codex review
A Codex pass on the diff surfaced two findings, both folded in:
Validation
ruff check slayer/ tests/: clean.Summary by CodeRabbit
Bug Fixes
Tests