test(data): mapping-coverage drift/floor check + e2e fixture (#736)#1363
Closed
SharedQA wants to merge 7 commits into
Closed
test(data): mapping-coverage drift/floor check + e2e fixture (#736)#1363SharedQA wants to merge 7 commits into
SharedQA wants to merge 7 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>
…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>
…ctorfabric#736) Referential mapping-coverage guard: activity.person_key (= lower(email)) must map to a BambooHR class_people email. Flags the latest complete day per tenant when mapped-coverage drops >5pt vs its 7-day trailing average OR falls under a 75% floor. data_quality catalog (severity=warn, store_failures), reads FINAL. This is the corrected form of a proposed design whose draft SQL would not run here: it joined a non-existent class_people.person_key, used Postgres idioms (::float, date_trunc, current_timestamp, nullif), and was a generic test wired via schema.yml rather than this repo's singular data_quality convention. The join is email-equality (class_people has email, not person_key); ClickHouse functions (toFloat via /, today(), nullIf) and LEFT-JOIN default-not-null semantics are handled. Decisions baked in: BambooHR is the denominator (Decision 1 — Entra-only = expected external); relative drift AND absolute floor (Decision 3). Placeholder alignment (the rig couldn't test this otherwise): class_people placeholder was missing tenant_id + source; class_collab_document_activity placeholder used insight_tenant_id but the real model emits tenant_id — added additively. e2e fixture (dataquality/test_collab_document_mapping_coverage.py): healthy multi-day series passes; 7 healthy days + an all-orphan latest day trips floor and drift (exactly one tenant-day, warn). Not closed here (honest follow-ups): person_key is email, so a renamed user orphans until the identity service lands person_id in silver (Gap 1 — upstream); Slack user_id resolution (Gap 2) and a by-domain orphan rollup for steward triage (Gap 3) are separate. Stacks on constructorfabric#1358 (shares DbtRunner.run_test + the dataquality/ harness). Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
|
Warning Review limit reached
More reviews will be available in 57 minutes and 49 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ 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 |
SharedQA
marked this pull request as ready for review
June 22, 2026 17:26
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).
SharedQA
added a commit
to SharedQA/insight
that referenced
this pull request
Jun 23, 2026
…rfabric#1363 create-bronze-placeholders.sh is extended by both this PR (SharePoint/OneDrive bronze columns for the transform test) and constructorfabric#1363 (silver placeholder columns for the mapping-coverage check). Make the file byte-identical across both via a clean 3-way union so they merge in any order with no conflict. Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com>
…orfabric#1362 Match create-bronze-placeholders.sh to the unified version in constructorfabric#1362 (this PR's silver placeholder columns + constructorfabric#1362's bronze columns) so the shared file is byte-identical and the two PRs merge in any order without conflict. dbt_runner.py here is already the shared superset (constructorfabric#1362 was synced to it). Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com>
Contributor
|
Already wired |
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.
A referential mapping-coverage guard for #736 (Entra ≫ BambooHR; orphan unlicensed M365 users). It flags the latest complete day per tenant when the share of active users that map to a BambooHR employee (
person_key = lower(email)) either drops >5 points below its 7-day trailing average or falls under a 75% floor.data_qualitycatalog:severity=warn+store_failures, readsFINAL.This is the corrected form of a proposed design. The original draft SQL wouldn't run here: it joined a non-existent
class_people.person_key(the roster hasemail), used Postgres idioms (::float,date_trunc,current_timestamp,nullif), and was a generic test wired viaschema.ymlrather than this repo's singulardata_qualityconvention. Fixed: email-equality join, ClickHouse functions + LEFT-JOIN default-not-null semantics, singular tagged test.Decisions baked in: BambooHR is the denominator (Decision 1 — Entra-only = expected external); relative drift and absolute floor (Decision 3).
Placeholder alignment (the rig couldn't test this otherwise — both were drifted from the real models):
class_peopleplaceholder was missingtenant_id+source; the activity placeholder usedinsight_tenant_idwhile the real model emitstenant_id. Added additively.e2e fixture (
dataquality/test_collab_document_mapping_coverage.py): a healthy multi-day series passes; 7 healthy days + an all-orphan latest day trips both floor and drift (exactly one tenant-day, warn).Honest follow-ups, not closed here:
person_keyis email, so a renamed user (marriage/legal change) orphans until the identity service landsperson_idin silver — that's the deeper fix (upstream), not this check.user_idresolution and a by-domain orphan rollup for steward triage are separate.Stacks on #1358 (shares
DbtRunner.run_test+ thedataquality/harness). Draft — the e2e CI job is the validator.