test(e2e): add CRM sales-rep metric e2e tests#1508
Draft
mitasovr wants to merge 4 commits into
Draft
Conversation
Add declarative YAML e2e tests for all five CRM (sales-rep dashboard)
metrics consumed by the frontend:
- …0020 CRM KPIs (crm_kpis)
- …0021 CRM Chart Flow (crm_chart_flow)
- …0022 CRM Bullet V&Q (crm_bullet_quality)
- …0023 CRM Bullet Activity (crm_bullet_activity)
- …0028 CRM Pipeline Now (crm_pipeline_now, date-less stock metric)
The CRM path is pure dbt (bronze HubSpot -> staging -> silver -> gold
views), so the tests seed bronze and exercise the full path — unlike the
Task Delivery tests, which seed silver directly because of the Rust
jira-enrich step. Team median/range is sourced from the bamboohr
department via silver.class_people.
New:
- specs/crm_{kpis,chart_flow,pipeline_now,bullet_quality,bullet_activity}.test.yaml
- specs/schemas/bronze_hubspot.{deals,owners,engagements_*}.yaml
- specs/templates/hubspot.yaml
create-bronze-placeholders.sh: add bronze_hubspot database + six bronze
table placeholders (additive only) so the rig can seed HubSpot bronze.
Verified: ./e2e.sh test = 53 passed, 0 failed (5 new + 48 existing).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
📝 WalkthroughWalkthroughProvisions a new ChangesHubSpot Bronze Fixtures and CRM Metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/ingestion/tests/e2e/specs/crm_pipeline_now.test.yaml (1)
35-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winModel the re-sync as a newer version, not an identical copy.
The second
deal-p1row is byte-for-byte identical to the first one, so this only checks duplicate suppression. It does not verify the "same unique_key + updatedAt" behavior described in the comment or catch a regression where the older version wins. Consider bumping the duplicate's versioning field and changing a rolled-up value so the assertion proves newest-row-wins dedup.🤖 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 `@src/ingestion/tests/e2e/specs/crm_pipeline_now.test.yaml` around lines 35 - 43, The duplicate `deal-p1` entry in `crm_pipeline_now.test.yaml` is identical to the original, so it only tests exact duplicate suppression instead of the intended newer-version re-sync behavior. Update the second `hubspot_deal` fixture to use the same `unique_key` but a newer versioning field such as `updatedAt`, and change at least one rolled-up property in the `deal-p1` record so the test proves the newest row wins. Keep the assertion aligned with the ingest flow that dedups by `unique_key` and versioning metadata in the `templates/hubspot.yaml#/templates/hubspot_deal` case.src/ingestion/tests/e2e/specs/templates/hubspot.yaml (1)
10-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTreat
_airbyte_extracted_atas scenario data for duplicate fixtures.These base records hard-code the same
_airbyte_extracted_at, butsrc/ingestion/scripts/create-bronze-placeholders.shuses that column as theReplacingMergeTreeversion for everybronze_hubspot.*table. Sincesrc/ingestion/tests/e2e/e2e_lib/ch_seeder.pyinserts duplicates physically, duplicate-row cases that inherit the default timestamp on both rows can make the dedup winner undefined and weaken the duplicate-deal coverage this PR is adding. I'd keep_airbyte_*in the shared scaffolding for simple fixtures, but require duplicate/versioned cases to override_airbyte_extracted_atexplicitly instead of treating it as invariant.Also applies to: 24-27, 72-75, 108-111, 135-138, 161-164, 189-192
🤖 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 `@src/ingestion/tests/e2e/specs/templates/hubspot.yaml` around lines 10 - 14, Treat _airbyte_extracted_at as scenario-specific for duplicate fixtures instead of shared invariant scaffolding. Update the HubSpot template so the duplicate/versioned cases override the timestamp explicitly in the *.test.yaml scenario files, while keeping the shared template only for true invariant envelope/owner fields. Make sure the duplicated records used by ch_seeder.py no longer inherit the same default _airbyte_extracted_at value when the test depends on ReplacingMergeTree versioning.
🤖 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/specs/crm_bullet_activity.test.yaml`:
- Around line 213-228: The crm_bullet_activity e2e spec currently checks only
that the expected metric_key entries exist, so extra or duplicate rows could
still pass. Update the expectations in crm_bullet_activity.test.yaml to also
assert the total response size is exactly four, using the existing items
collection in the crm result, so the test verifies the full response shape as
well as the individual metrics.
In `@src/ingestion/tests/e2e/specs/crm_bullet_quality.test.yaml`:
- Around line 133-151: The crm_bullet_quality.test.yaml expectations validate
the metric_key values but do not enforce the total number of bullet rows
returned. Update the assertions in the expect block to include a size check on
the crm items collection, alongside the existing find checks, so the test fails
if an extra or duplicate row appears and the contract is enforced by the spec.
In `@src/ingestion/tests/e2e/specs/crm_chart_flow.test.yaml`:
- Around line 75-108: The CRM chart flow spec only verifies individual weekly
rows, so an extra or duplicated weekly bucket could still pass. Update the
assertions in crm_chart_flow.test.yaml to check the total result size as well,
using the existing query response collection (the CRM `expect` block and `find`
checks) so the January weekly bucketing contract is enforced with a size(items)
== 3 assertion.
---
Nitpick comments:
In `@src/ingestion/tests/e2e/specs/crm_pipeline_now.test.yaml`:
- Around line 35-43: The duplicate `deal-p1` entry in
`crm_pipeline_now.test.yaml` is identical to the original, so it only tests
exact duplicate suppression instead of the intended newer-version re-sync
behavior. Update the second `hubspot_deal` fixture to use the same `unique_key`
but a newer versioning field such as `updatedAt`, and change at least one
rolled-up property in the `deal-p1` record so the test proves the newest row
wins. Keep the assertion aligned with the ingest flow that dedups by
`unique_key` and versioning metadata in the
`templates/hubspot.yaml#/templates/hubspot_deal` case.
In `@src/ingestion/tests/e2e/specs/templates/hubspot.yaml`:
- Around line 10-14: Treat _airbyte_extracted_at as scenario-specific for
duplicate fixtures instead of shared invariant scaffolding. Update the HubSpot
template so the duplicate/versioned cases override the timestamp explicitly in
the *.test.yaml scenario files, while keeping the shared template only for true
invariant envelope/owner fields. Make sure the duplicated records used by
ch_seeder.py no longer inherit the same default _airbyte_extracted_at value when
the test depends on ReplacingMergeTree versioning.
🪄 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: 7ef2e885-ec6a-4b4f-92b5-0cd8f4e95a08
📒 Files selected for processing (13)
src/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/tests/e2e/specs/crm_bullet_activity.test.yamlsrc/ingestion/tests/e2e/specs/crm_bullet_quality.test.yamlsrc/ingestion/tests/e2e/specs/crm_chart_flow.test.yamlsrc/ingestion/tests/e2e/specs/crm_kpis.test.yamlsrc/ingestion/tests/e2e/specs/crm_pipeline_now.test.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_hubspot.deals.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_hubspot.engagements_calls.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_hubspot.engagements_emails.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_hubspot.engagements_meetings.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_hubspot.engagements_tasks.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_hubspot.owners.yamlsrc/ingestion/tests/e2e/specs/templates/hubspot.yaml
Comment on lines
+213
to
+228
| expect: | ||
| - assert: "status == 200" | ||
| - in: crm | ||
| assert: "result.status == 'ok'" | ||
| - in: crm | ||
| find: { metric_key: calls } | ||
| equal: { value: 2, median: 2, range_min: 1, range_max: 3 } | ||
| - in: crm | ||
| find: { metric_key: emails } | ||
| equal: { value: 2, median: 2, range_min: 1, range_max: 3 } | ||
| - in: crm | ||
| find: { metric_key: meetings } | ||
| equal: { value: 2, median: 2, range_min: 1, range_max: 3 } | ||
| - in: crm | ||
| find: { metric_key: comms_per_won } | ||
| equal: { value: 4, median: 4, range_min: 3, range_max: 9 } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that exactly four bullet rows are returned.
Like the quality spec, this only verifies that the expected keys are present. An extra metric_key or duplicate row would still pass. Add size(items) == 4 so the test locks down the full response shape.
🤖 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 `@src/ingestion/tests/e2e/specs/crm_bullet_activity.test.yaml` around lines 213
- 228, The crm_bullet_activity e2e spec currently checks only that the expected
metric_key entries exist, so extra or duplicate rows could still pass. Update
the expectations in crm_bullet_activity.test.yaml to also assert the total
response size is exactly four, using the existing items collection in the crm
result, so the test verifies the full response shape as well as the individual
metrics.
Comment on lines
+133
to
+151
| expect: | ||
| - assert: "status == 200" | ||
| - in: crm | ||
| assert: "result.status == 'ok'" | ||
| - in: crm | ||
| find: { metric_key: deals_opened } | ||
| equal: { value: 5, median: 3, range_min: 2, range_max: 5 } | ||
| - in: crm | ||
| find: { metric_key: avg_deal_size } | ||
| equal: { value: 20000, median: 20000, range_min: 8000, range_max: 40000 } | ||
| - in: crm | ||
| find: { metric_key: cycle_days } | ||
| equal: { value: 9.5, median: 9.5, range_min: 4, range_max: 20 } | ||
| - in: crm | ||
| find: { metric_key: win_rate } | ||
| assert: > | ||
| double(it.value) > 66.6 && double(it.value) < 66.7 && | ||
| double(it.median) > 66.6 && double(it.median) < 66.7 && | ||
| double(it.range_min) == 50.0 && double(it.range_max) == 100.0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that exactly four bullet rows are returned.
The current checks prove the expected metric_key values exist, but an extra key or duplicate row would still pass. Add size(items) == 4 to enforce the contract described in the spec.
🤖 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 `@src/ingestion/tests/e2e/specs/crm_bullet_quality.test.yaml` around lines 133
- 151, The crm_bullet_quality.test.yaml expectations validate the metric_key
values but do not enforce the total number of bullet rows returned. Update the
assertions in the expect block to include a size check on the crm items
collection, alongside the existing find checks, so the test fails if an extra or
duplicate row appears and the contract is enforced by the spec.
Comment on lines
+75
to
+108
| expect: | ||
| - assert: "status == 200" | ||
| - in: crm | ||
| assert: "result.status == 'ok'" | ||
| # opened/closed/won are UInt64 → JSONEachRow quotes them as strings; cast with int(). | ||
| - in: crm | ||
| find: { metric_date: "2026-01-05" } | ||
| assert: "it.date_bucket == 'Jan 05' && int(it.opened) == 1 && int(it.closed) == 1 && int(it.won) == 1" | ||
| - name: "crm_chart_flow — week of Jan 12 (opened 2, closed 1, won 1)" | ||
| request: | ||
| url: /v1/metrics/queries | ||
| method: POST | ||
| body: | ||
| queries: | ||
| - id: crm | ||
| metric_id: 00000000-0000-0000-0001-000000000021 | ||
| $filter: "person_id eq 'alice@example.com' and metric_date ge '2026-01-01' and metric_date le '2026-01-31'" | ||
| expect: | ||
| - in: crm | ||
| find: { metric_date: "2026-01-12" } | ||
| assert: "it.date_bucket == 'Jan 12' && int(it.opened) == 2 && int(it.closed) == 1 && int(it.won) == 1" | ||
| - name: "crm_chart_flow — week of Jan 19 (opened 1, closed 1, won 0 — lost deal)" | ||
| request: | ||
| url: /v1/metrics/queries | ||
| method: POST | ||
| body: | ||
| queries: | ||
| - id: crm | ||
| metric_id: 00000000-0000-0000-0001-000000000021 | ||
| $filter: "person_id eq 'alice@example.com' and metric_date ge '2026-01-01' and metric_date le '2026-01-31'" | ||
| expect: | ||
| - in: crm | ||
| find: { metric_date: "2026-01-19" } | ||
| assert: "it.date_bucket == 'Jan 19' && int(it.opened) == 1 && int(it.closed) == 1 && int(it.won) == 0" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the total number of weekly buckets.
These checks only prove the three expected weeks exist. An extra bucket or duplicated metric_date row in January would still pass. Add size(items) == 3 so the spec actually guards the weekly bucketing contract.
🤖 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 `@src/ingestion/tests/e2e/specs/crm_chart_flow.test.yaml` around lines 75 -
108, The CRM chart flow spec only verifies individual weekly rows, so an extra
or duplicated weekly bucket could still pass. Update the assertions in
crm_chart_flow.test.yaml to check the total result size as well, using the
existing query response collection (the CRM `expect` block and `find` checks) so
the January weekly bucketing contract is enforced with a size(items) == 3
assertion.
ktursunov
reviewed
Jun 29, 2026
|
|
||
| Team = department "Sales" {alice, bob, carol} (odd team → unambiguous median): | ||
| calls alice 2 · bob 1 · carol 3 → median 2, range [1, 3] | ||
| emails alice 2 · bob 1 · carol 3 → median 2, range [1, 3] |
Contributor
There was a problem hiding this comment.
Let's have different numbers for each metric key, easier to understand the difference with testcases
…t truncate
Strengthen the Pipeline Now (…0028) e2e test to document the metric's actual
date semantics, and make warm CRM re-runs deterministic.
crm_pipeline_now is a date-less stock view (no metric_date column; openness is
the static is_closed flag). The cases now pin:
- the date-less call (how the FE queries it) → open-deal count + summed value;
- a closed deal is excluded and counted in the closed metric instead;
- a deal closed with a FUTURE close_date is still EXCLUDED (is_closed-based),
documenting the divergence from an "as of end_date" snapshot;
- supplying any metric_date bound makes the per-query result non-ok (the view
has no metric_date column), i.e. neither a start nor an end can be passed
today. Making it end-date-aware is a backend change tracked separately.
conftest.py: add the CRM silver (class_crm_{deals,activities,users}) and HubSpot
staging (hubspot__crm_{deals,activities,users}) tables to the session-start
truncate list, mirroring the collab/task entries, so warm re-runs are
deterministic (CI starts fresh anyway).
Verified: ./e2e.sh test = 53 passed, 0 failed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/ingestion/tests/e2e/specs/crm_pipeline_now.test.yaml (2)
119-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover start-only and end-only bounds separately.
The prose says any
metric_datebound should make the query non-ok, but this spec only exercises the combinedge+lefilter. Add one start-only case and one end-only case so the date-less contract is pinned down per bound.🤖 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 `@src/ingestion/tests/e2e/specs/crm_pipeline_now.test.yaml` around lines 119 - 137, The crm_pipeline_now e2e spec only covers a combined metric_date range, but the comment requires separate coverage for start-only and end-only bounds. In the crm_pipeline_now test case, add one query that uses only metric_date ge and another that uses only metric_date le, and keep the existing non-ok assertions so the date-less view contract is pinned down for each bound individually.
98-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the dedup assertion to prove which duplicate wins.
This expected
70000total only shows that one of the duplicate rows was dropped. It does not prove the deduper kept the newest HubSpot row, so a stale-row selection bug would still pass here. Make the duplicate carry a later timestamp and a different amount/name, then assert the surviving value.Example tightening
bronze_hubspot.deals: # P1 duplicate (re-sync) — same unique_key + updatedAt → must dedup - $ref: templates/hubspot.yaml#/templates/hubspot_deal id: deal-p1 unique_key: hubspot-deal-p1 properties_hubspot_owner_id: own-alice createdAt: "2026-01-05T09:00:00" - properties_amount_in_home_currency: "20000" + updatedAt: "2026-01-09T09:00:00" + properties_amount_in_home_currency: "25000" properties_hs_is_closed: "false" properties_hs_is_closed_won: "false"- - in: crm - find: { person_id: alice@example.com } - assert: "int(it.pipeline_count) == 2 && double(it.pipeline_value) == 70000.0" + - in: crm + find: { person_id: alice@example.com } + assert: "int(it.pipeline_count) == 2 && double(it.pipeline_value) == 75000.0"🤖 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 `@src/ingestion/tests/e2e/specs/crm_pipeline_now.test.yaml` around lines 98 - 115, The crm_pipeline_now e2e case only proves duplicates were removed, not which HubSpot row the deduper kept. Update the test fixture in crm_pipeline_now.test.yaml so the duplicate record has a later timestamp and a distinct amount/name, then assert against the surviving latest row in the crm query result. Use the existing crm metric query and its person_id filter, and tighten the expectation on pipeline_value (and any related fields) to verify the newest duplicate wins.
🤖 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.
Nitpick comments:
In `@src/ingestion/tests/e2e/specs/crm_pipeline_now.test.yaml`:
- Around line 119-137: The crm_pipeline_now e2e spec only covers a combined
metric_date range, but the comment requires separate coverage for start-only and
end-only bounds. In the crm_pipeline_now test case, add one query that uses only
metric_date ge and another that uses only metric_date le, and keep the existing
non-ok assertions so the date-less view contract is pinned down for each bound
individually.
- Around line 98-115: The crm_pipeline_now e2e case only proves duplicates were
removed, not which HubSpot row the deduper kept. Update the test fixture in
crm_pipeline_now.test.yaml so the duplicate record has a later timestamp and a
distinct amount/name, then assert against the surviving latest row in the crm
query result. Use the existing crm metric query and its person_id filter, and
tighten the expectation on pipeline_value (and any related fields) to verify the
newest duplicate wins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08f35f0b-f0f8-4cfc-950d-0dfeda1d68e4
📒 Files selected for processing (2)
src/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/specs/crm_pipeline_now.test.yaml
…eline_now (RED) Two things, after merging main (which renamed the rig specs/→metrics/, e2e_lib/→lib/ in constructorfabric#1507): 1. Relocate the CRM fixtures from the now-orphaned specs/ into the renamed metrics/ tree (tests → metrics/, schemas → metrics/schemas/, template → metrics/templates/) so the refactored rig discovers them again. 2. crm_pipeline_now is now a TARGET-BEHAVIOR spec that asserts the intended "as of end_date" semantics and FAILS on purpose against the shipped backend (RED until the metric is made end-date-aware): - open as of end_date ⇔ is_closed = 0 OR close_date > end_date; - the period END moves the snapshot boundary; a deal closing after end_date is still pipeline, moving the end past its close drops it; - the period START does not affect the result; - a deal closed on/before end_date is excluded (closed metric). Currently RED because the gold view is date-less (is_closed flag) and analytics-api errors the per-query when a metric_date bound is supplied. The file header lists the three backend files to change to make it pass. The other four CRM tests (kpis, chart_flow, bullet_quality, bullet_activity) pass on the new layout; only crm_pipeline_now is intentionally failing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ingestion/tests/e2e/metrics/crm_pipeline_now.test.yaml (1)
53-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe duplicate-deal fixture does not actually prove latest-row deduplication.
The "re-sync" D1 row is identical to the original in the fields shown here, and the comment mentions
updatedAteven though this file never overrides it. If dedup regressed from "latest row wins" to "pick any row per key", this test would still pass. Give the duplicate a later version marker and a changed projected value so the assertion proves which row survives.🤖 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 `@src/ingestion/tests/e2e/metrics/crm_pipeline_now.test.yaml` around lines 53 - 61, The duplicate-deal fixture in crm_pipeline_now.test.yaml does not verify latest-row deduplication because the re-sync D1 record is identical to the original and never overrides updatedAt. Update the D1 duplicate in the hubspot_deal fixture to include a later version marker and change at least one projected field so the test can prove the "latest row wins" behavior. Keep the fix focused on the duplicate-row setup used by the crm_pipeline_now end-to-end metric test.
🤖 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/metrics/crm_pipeline_now.test.yaml`:
- Around line 5-15: This e2e spec is documenting intentionally failing
metric_date behavior, but the assertions still expect success, which makes it
look like a normal passing test. Update crm_pipeline_now.test.yaml to use the
rig’s expected-failure or skip mechanism, or defer the spec until the backend
changes land; keep the intended semantics tied to the crm_pipeline_now metric
tests and avoid asserting ok on queries the current analytics-api and gold view
do not support.
---
Nitpick comments:
In `@src/ingestion/tests/e2e/metrics/crm_pipeline_now.test.yaml`:
- Around line 53-61: The duplicate-deal fixture in crm_pipeline_now.test.yaml
does not verify latest-row deduplication because the re-sync D1 record is
identical to the original and never overrides updatedAt. Update the D1 duplicate
in the hubspot_deal fixture to include a later version marker and change at
least one projected field so the test can prove the "latest row wins" behavior.
Keep the fix focused on the duplicate-row setup used by the crm_pipeline_now
end-to-end metric test.
🪄 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: 1dee955b-00b9-4e44-a7b3-77e11c3bee22
📒 Files selected for processing (13)
src/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/metrics/crm_bullet_activity.test.yamlsrc/ingestion/tests/e2e/metrics/crm_bullet_quality.test.yamlsrc/ingestion/tests/e2e/metrics/crm_chart_flow.test.yamlsrc/ingestion/tests/e2e/metrics/crm_kpis.test.yamlsrc/ingestion/tests/e2e/metrics/crm_pipeline_now.test.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.deals.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.engagements_calls.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.engagements_emails.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.engagements_meetings.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.engagements_tasks.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.owners.yamlsrc/ingestion/tests/e2e/metrics/templates/hubspot.yaml
💤 Files with no reviewable changes (11)
- src/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.deals.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.engagements_emails.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.engagements_calls.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.engagements_meetings.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.owners.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_hubspot.engagements_tasks.yaml
- src/ingestion/tests/e2e/metrics/crm_chart_flow.test.yaml
- src/ingestion/tests/e2e/metrics/crm_bullet_quality.test.yaml
- src/ingestion/tests/e2e/metrics/crm_kpis.test.yaml
- src/ingestion/tests/e2e/metrics/templates/hubspot.yaml
- src/ingestion/tests/e2e/metrics/crm_bullet_activity.test.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ingestion/tests/e2e/conftest.py
Comment on lines
+5
to
+15
| ⚠️ TARGET-BEHAVIOR SPEC — CURRENTLY EXPECTED TO FAIL (RED). ⚠️ | ||
| This test asserts the INTENDED "as of end_date" semantics. The shipped backend | ||
| does NOT implement them yet (the gold view `insight.crm_pipeline_now` is | ||
| date-less — openness is the static `is_closed` flag — and analytics-api errors | ||
| the per-query when a metric_date bound is supplied because the view has no such | ||
| column). So these cases fail today ON PURPOSE; they go green only once the | ||
| metric is made end-date-aware. Making it so is a backend change across: | ||
| • gold view src/ingestion/scripts/migrations/20260512000000_crm-gold-views.sql | ||
| • query_ref src/backend/services/analytics-api/src/migration/m20260507_000001_seed_crm_metrics.rs | ||
| • (likely) the date-filter injection in analytics-api api/handlers.rs | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't merge a permanently red e2e spec as a normal passing test.
This file says the current backend rejects these metric_date-bounded queries, but every case still asserts result.status == 'ok'. Once the metric specs are discovered, this either turns ./e2e.sh test red or means the file still is not being executed, so the claimed coverage is misleading. Gate this behind the rig's expected-failure/skip mechanism, or land it together with the backend change.
Also applies to: 105-148
🤖 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 `@src/ingestion/tests/e2e/metrics/crm_pipeline_now.test.yaml` around lines 5 -
15, This e2e spec is documenting intentionally failing metric_date behavior, but
the assertions still expect success, which makes it look like a normal passing
test. Update crm_pipeline_now.test.yaml to use the rig’s expected-failure or
skip mechanism, or defer the spec until the backend changes land; keep the
intended semantics tied to the crm_pipeline_now metric tests and avoid asserting
ok on queries the current analytics-api and gold view do not support.
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.
What
Adds declarative YAML e2e tests for all five CRM (sales-rep dashboard) metrics consumed by the frontend. No backend/metric changes — tests only.
insight.crm_kpisinsight.crm_chart_flowinsight.crm_bullet_rowsinsight.crm_bullet_rowsinsight.crm_pipeline_now(date-less stock metric)Approach
The CRM path is pure dbt (bronze HubSpot → dbt staging → dbt silver → gold views), so the tests seed bronze and exercise the full path — unlike the Task Delivery tests, which seed silver directly because of the Rust
jira-enrichstep. Teammedian/rangeis sourced from the bamboohr department viasilver.class_people(reusingtemplates/people.yaml).Each test seeds a predictable scenario (multiple reps, open/closed/won deals, activities of each type, one
Salesdepartment for the team distribution) and asserts exact aggregates:deals_opened/win_rate/avg_deal_size/cycle_days+ theSalesteam median/min/max (odd 3-rep team → unambiguousquantileExactmedian)calls/emails/meetings/comms_per_won+ team distributionFiles
specs/crm_{kpis,chart_flow,pipeline_now,bullet_quality,bullet_activity}.test.yamlspecs/schemas/bronze_hubspot.{deals,owners,engagements_calls,engagements_emails,engagements_meetings,engagements_tasks}.yamlspecs/templates/hubspot.yamlscripts/create-bronze-placeholders.sh— additive only:bronze_hubspotdatabase + six bronze table placeholders (column types verified against a dev sync), so the rig can seed HubSpot bronze.Verification
./e2e.sh test→ 53 passed, 0 failed (5 new + 48 existing; full-suite run = cross-test isolation check).🤖 Generated with Claude Code
Summary by CodeRabbit