test(e2e): PoC — run a data_quality check against seeded silver (#1348)#1358
Closed
SharedQA wants to merge 6 commits into
Closed
test(e2e): PoC — run a data_quality check against seeded silver (#1348)#1358SharedQA wants to merge 6 commits into
SharedQA wants to merge 6 commits into
Conversation
…tructorfabric#1348) Concrete answer to "how do we test the data-quality checks" (constructorfabric#1348): reuse the existing bronze→API e2e rig (ClickHouse + dbt already come up) and add the two missing pieces. - DbtRunner.run_test(selector): runs `dbt test --select <check>` and returns (status, failures) from run_results.json. The catalog checks are severity='warn' so dbt exits 0 regardless; we read the failures count exactly as the deployed emitter does. `--defer --state` resolves the model ref to the seeded silver relation without rebuilding it. - dataquality/test_collab_document_counts_non_negative.py: the good/bad fixture pair for the constructorfabric#1321 non-negative check (PR constructorfabric#1350) — seed clean rows → 0 violations, status pass; seed a negative row → exactly 1 flagged, status warn (proving warn-severity surfaces a finding without failing the build). Two placeholder/model drifts this surfaced (both noted in dataquality/README): - the silver class_collab_document_activity placeholder was missing unique_key / synced_count / visited_page_count / product — aligned here so the check's projection resolves; - the bronze sharepoint_activity placeholder lacks the document-activity source columns, so the full bronze→silver build isn't rig-testable yet. This PoC seeds silver directly to sidestep that; extending the bronze placeholder is a follow-up. Draft: validated against rig conventions but not run locally (no warehouse) — the e2e CI job is the first real execution. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughThe PR extends the ChangesNon-negative collab count data quality check and e2e validation
Sequence Diagram(s)sequenceDiagram
participant Test as pytest test
participant Seed as _seed()
participant CH as ClickHouse silver table
participant Runner as DbtRunner.run_test()
participant dbt as dbt test process
participant Results as run_results.json
Test->>Seed: _seed(cfg, rows)
Seed->>CH: TRUNCATE silver.class_collab_document_activity
Seed->>CH: INSERT rows
Test->>Runner: run_test("assert_collab_document_counts_non_negative", worker_ctx)
Runner->>Results: delete stale run_results.json
Runner->>dbt: dbt test --select assert_collab_document_counts_non_negative --vars worker_id
dbt->>CH: query silver table
dbt->>Results: write run_results.json
Runner->>Results: read and parse JSON
Runner-->>Test: return (status, failures)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
…ts (constructorfabric#1348) Review of the data-quality PoC: - Relative dates: `_row()` now uses `today() - days_ago` instead of a hardcoded 2026 date. The non-negative check has no date filter, but a sibling check (no_spike) filters `date >= today() - {120,3}`; a hardcoded date would be filtered out and a seeded violation would falsely pass. A date factory must survive time-bound dbt logic. - Crash-safe results: `DbtRunner.run_test` now unlinks run_results.json BEFORE running and treats an absent file afterward as a dbt failure (compile/conn error), surfacing stdout/stderr — instead of silently reading a stale file from the previous run. - Documented two review points in the README: the bronze→silver blindspot is the priority follow-up (and why it's a real task — the build pulls m365__bronze_ promoted + both staging models, so the m365 bronze placeholders must be completed first), and the serial/TRUNCATE limitation is the rig-wide model today (worker_id already plumbed for the eventual per-worker-schema fix), not a flaw unique to this test. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/ingestion/tests/e2e/e2e_lib/dbt_runner.py`:
- Around line 175-184: The selector resolution in the run_test method uses
substring matching to find nodes, which can match multiple unrelated nodes if
they share common text patterns. When multiple matches exist, the code takes the
first match without verification, potentially returning the wrong test's status
and failures. Replace the substring matching logic (using the in operator with
selector) with exact matching on the unique_id to ensure you only match the
intended node, or add validation to ensure the selector uniquely identifies
exactly one node before accessing matches[0].
🪄 Autofix (Beta)
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
Run ID: 1a086ad6-d1e3-40f4-b656-980c852e89f9
📒 Files selected for processing (6)
src/ingestion/dbt/tests/collaboration/assert_collab_document_counts_non_negative.sqlsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/tests/e2e/dataquality/README.mdsrc/ingestion/tests/e2e/dataquality/__init__.pysrc/ingestion/tests/e2e/dataquality/test_collab_document_counts_non_negative.pysrc/ingestion/tests/e2e/e2e_lib/dbt_runner.py
Address CodeRabbit review on constructorfabric#1358: run_test matched the selector as a substring of unique_id and took matches[0] unverified, so a selector could collide on a shared prefix (e.g. `assert_collab_document_counts_non_negative` vs `..._no_spike`) and report the wrong node's status/failures. Match on exact unique_id or a full dot-segment instead, and require exactly one match (raise otherwise). Resolves both singular tests (name is the last segment) and generic tests (name precedes the hash). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kenan Salim <kenan.salim@gmail.com>
The dbt profile hardcoded host=127.0.0.1, which is unreachable from the dockerized e2e runner (CH is the compose service `clickhouse`). This made the data_quality PoC test's `run_test` -> `dbt test` fail locally. Use cfg.ch_host (127.0.0.1 in host mode, `clickhouse` in docker mode) and update the profiles meta-test assertion to match. Validated locally: dataquality + meta/test_dbt_runner suites pass (6 passed), exercising run_test against real ClickHouse. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kenan Salim <kenan.salim@gmail.com>
Contributor
Author
|
Closing as redundant — fully subsumed: the e2e harness ( |
SharedQA
added a commit
to SharedQA/insight
that referenced
this pull request
Jun 23, 2026
…ld + run_test) Unify the dbt_runner used by the collab-document e2e PRs onto one version: add the run_test() helper (data-quality checks vs seeded silver) alongside the existing build() used by the transform test. Byte-identical to the copy in constructorfabric#1363, so the two PRs merge in any order and collapse to one file instead of three divergent copies (the third, constructorfabric#1358, is closed as redundant). Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com> constructorfabric#1362 owns this harness (dbt_runner + test_dbt_runner + pytest.ini).
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
2 participants
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.
Proof-of-concept for the question on #1348 — how do we actually test the
data_qualitycatalog checks, which need a warehouse and data? Concrete answer: reuse the existing bronze→API e2e rig and add the two missing pieces.What's here
DbtRunner.run_test(<check>)— runsdbt test --select <check>and returns(status, failures)fromrun_results.json. The catalog checks areseverity='warn', so dbt exits 0 even on a violation; reading thefailurescount is exactly how the deployed emitter detects findings.--defer --stateresolves the modelref()to the seeded silver relation without rebuilding (and wiping) it.dataquality/test_collab_document_counts_non_negative.py— the good/bad fixture pair for the EPIC: data-correctness contract coverage (bronze · silver · gold) → 100% #1321 non-negative check (PR test(data): silver business-rule data tests — non-negative counts (#1321) #1350):passwarn(proving warn-severity surfaces a finding without failing the build)Two drifts this PoC surfaced (the genuinely useful part for the discussion):
class_collab_document_activityplaceholder had drifted from the model — missingunique_key,synced_count,visited_page_count,product. Aligned here so the check's projection resolves.sharepoint_activityplaceholder lacks the document-activity source columns, so the full bronze→silver build of this model isn't rig-testable yet. This PoC seeds silver directly to sidestep that; extending the bronze placeholder is the natural follow-up that would let the whole path run.Status: draft. Validated against the rig's conventions and APIs but not run locally (no warehouse here) — the e2e CI job on this PR is the first real execution. Stacked on #1350 (the check under test lives there), so the diff includes that file until #1350 merges.
Opening this as a worked example to anchor the #1348 plan, not as a merge candidate yet.
Summary by CodeRabbit
New Features
Tests
Chores