fix: allow _with_context tests to collect sample rows - #981
Conversation
Elementary-namespaced tests were early-returning from the test materialization, bypassing all sampling logic. _with_context tests behave like regular dbt tests (they return failing rows) so they should go through the standard handle_dbt_test sampling path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
👋 @joostboon |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ChangesTest materialization behavior
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…alization Instead of checking test name suffix, use the existing get_elementary_test_type macro to determine if an elementary-namespaced test handles its own result row collection. Only anomaly detection and schema change tests early-return; all other elementary tests (e.g. _with_context) go through the standard sampling path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This pull request is stale because it has been open for too long with no activity. |
|
This pull request was closed because it has been inactive for too long while being marked as stale. |
There were no integration tests for the _with_context family, which is why the materialization skipping them went unnoticed. Asserts that an elementary-namespaced with_context test writes test_sample_row_count rows to test_result_rows, and that the requested context column travels with each failing row, which is the point of the family. Fails against the unfixed materialization, where the early return on namespace left these tests with no result rows at all. Modelled on test_sampling.py and uses the same harness calls.
|
Reopening — this was closed by stale-bot on 2026-07-10 after no human review, not on its merits. Added the integration test the contribution bot asked for, and independently re-verified the bug against a live Snowflake warehouse. The bug is in every release that has the featureThe namespace early-return landed in
Verified end to endelementary
Note A useful corroborating detail: the package already classifies these as regular dbt tests. What I added
I have not run the integration suite myself — no warehouse credentials for the harness — so CI is the first real execution of that test. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@integration_tests/tests/test_with_context_sampling.py`:
- Around line 44-46: Update the assertions in the sample-validation loop to
compare each sample’s CONTEXT_COLUMN value against the exact expected context
value derived from null_count, rather than only checking the "context-" prefix;
keep the existing null assertion for COLUMN_NAME.
- Line 36: Update the test assertion near the existing status check to validate
that test_result["failed_row_count"] equals null_count. Keep the existing
failure-status assertion and use the input’s null_count value as the expected
failed-row count.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b82eec2-717e-4ec8-981e-412932f557b1
📒 Files selected for processing (1)
integration_tests/tests/test_with_context_sampling.py
|
CI is red, but none of it is this PR. Breakdown:
Two independent pre-existing issues: stale CI credentials for Snowflake and Athena (expected on a branch idle since April), and The new test passed on every adapter that could connect: plus the other green adapters (fabric, dremio, vertica, clickhouse, databricks_catalog, latest_pre/postgres). Snowflake CI couldn't run, but that is the adapter I verified the fix on by hand — elementary 0.25.1 / dbt-snowflake 1.11.1, Someone with access will need to refresh the Snowflake and Athena CI secrets to get a fully green run. 🤖 Generated with Claude Code |
Addresses CodeRabbit review on #981. Assert failed_row_count equals the number of failing rows. calculate_failed_count sits after the same early return the sampling did, so it was skipped too and left failed_row_count null; this covers that second path rather than only the samples. Compare each sampled context value against the exact expected set instead of a "context-" prefix check, so the assertion proves the materialization preserved the source value. Membership rather than equality because sampling returns an arbitrary test_sample_row_count of the failing rows.
|
Both CodeRabbit suggestions applied in 4b176a8 — both were right, and the first is more load-bearing than it looks.
So this assertion covers a second broken path, not just the samples. Consistent with Exact context values — applied. Used set membership rather than equality, since sampling returns an arbitrary Also merged current 🤖 Generated with Claude Code |
| {% set elementary_test_type = elementary.get_elementary_test_type( | ||
| {"short_name": short_name, "test_namespace": "elementary"} | ||
| ) %} | ||
| {% if elementary_test_type %} |
There was a problem hiding this comment.
Can we perhaps instead be more explicit, and return a type in get_elementary_test_type for the relevant tests, and then check here that it's not that type?
There was a problem hiding this comment.
Done in c33465f — agreed, explicit is better. get_elementary_test_type now returns a "with_context" type for the six tests in the family, and the materialization checks elementary_test_type != "with_context" instead of leaning on the macro returning none. It also means a future elementary test family defaults to the existing skip behaviour rather than silently landing in this bug again.
One thing worth knowing, since it caught me while implementing it: the new type must not reach test_type. All three alerts models partition on that value exactly —
alerts_dbt_tests.sql:41→test_type = 'dbt_test'alerts_anomaly_detection.sql:41→test_type = 'anomaly_detection'alerts_schema_changes.sql:47→test_type = 'schema_change'
so a fourth value would match none of them and with_context test failures would raise no alerts at all. get_test_type therefore maps with_context → dbt_test, keeping the reported type and the alerts behaviour exactly as before; the new type stays internal to the materialization gate. I've added assert test_result["test_type"] == "dbt_test" to the integration test so that can't regress silently.
Happy to change the type name, or to move the exemption list somewhere more visible than get_elementary_test_type, if you'd prefer it shaped differently.
🤖 Generated with Claude Code
… none Per review: get_elementary_test_type now returns a distinct "with_context" type for the family, and the materialization checks against that type rather than relying on the macro returning none. Also keeps get_test_type reporting "dbt_test" for these tests. That matters and is not cosmetic: the three alerts models partition on test_type exactly -- alerts_dbt_tests filters test_type = 'dbt_test', alerts_anomaly_detection 'anomaly_detection', alerts_schema_changes 'schema_change'. Surfacing a fourth value would match none of them, so with_context test failures would raise no alerts at all. The new type is therefore internal to the materialization gate and deliberately not exposed as a test_type. Adds an assertion on test_type == "dbt_test" to the integration test to guard that.
Summary
_with_contexttests are Elementary-namespaced but behave like regular dbt tests (they return failing rows rather than handling their own result row collection)elementary-namespaced tests, bypassing the sampling logic entirely_with_contexttests never wrote rows totest_result_rows, even when tagged withshow_sample_rowsFix
Check if the test name ends with
_with_contextbefore early-returning, allowing these tests to go through the standardhandle_dbt_testsampling path.Test plan
_with_contexttest (e.g.not_null_with_context) and verify rows appear intest_result_rows_with_contextElementary tests (anomaly, schema change) still early-return and handle their own result rows🤖 Generated with Claude Code
Summary by CodeRabbit
not_null_with_contexttests so configured sample counts are respected and context values are preserved in sampled results.