test(metrics): migrate e2e coverage to unified metrics#1820
Open
aleksdotbar wants to merge 3 commits into
Open
test(metrics): migrate e2e coverage to unified metrics#1820aleksdotbar wants to merge 3 commits into
aleksdotbar wants to merge 3 commits into
Conversation
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
📝 WalkthroughWalkthroughThe E2E framework now seeds schema-backed bronze tables, runs dbt gold models, collects builtin metric definitions, and validates ChangesUnified metric-results E2E migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
cyberantonz
approved these changes
Jul 17, 2026
ktursunov
approved these changes
Jul 17, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
src/ingestion/tests/e2e/metrics/collab_files_shared_external.test.yaml (1)
38-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore empty-window coverage across the department-shape fixtures. These migrations retain only populated requests, leaving unified zero-value behavior untested; two descriptions still explicitly claim empty-window coverage.
src/ingestion/tests/e2e/metrics/collab_files_shared_external.test.yaml#L38-L67: add a period-only empty-window case assertingvalue: 0.src/ingestion/tests/e2e/metrics/collab_files_shared_internal.test.yaml#L38-L87: restore the removed period-only empty-window case.src/ingestion/tests/e2e/metrics/collab_meeting_free_days.test.yaml#L55-L81: implement the empty-window case promised by the description.src/ingestion/tests/e2e/metrics/collab_meetings.test.yaml#L34-L171: implement the empty-window case promised by the description.Based on learnings, these fixtures should contain one custom-window case plus one empty-window case.
🤖 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/collab_files_shared_external.test.yaml` around lines 38 - 67, Restore empty-window coverage in all four fixtures: in src/ingestion/tests/e2e/metrics/collab_files_shared_external.test.yaml (38-67) add a period-only empty-window request asserting value 0; in src/ingestion/tests/e2e/metrics/collab_files_shared_internal.test.yaml (38-87) restore the removed equivalent case; and in src/ingestion/tests/e2e/metrics/collab_meeting_free_days.test.yaml (55-81) and src/ingestion/tests/e2e/metrics/collab_meetings.test.yaml (34-171) add the period-only empty-window cases promised by their descriptions. Keep each fixture to one custom-window case plus one empty-window case.Source: Learnings
🤖 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/conftest.py`:
- Around line 214-220: Update the coverage-artifact collection flow around
collect in the test session fixture to stop suppressing collection errors:
remove the warning-only exception handling or re-raise the caught exception
after any required logging. Ensure collect failures propagate and fail the
session, while preserving the enclosing finally cleanup that stops analytics.
In `@src/ingestion/tests/e2e/lib/ch_seeder.py`:
- Around line 136-147: The _ensure_table method must validate an existing
table’s columns and types against the generated schema instead of returning
whenever _fetch_column_types returns a non-empty result. Recreate the
fixture-owned bronze table when the schemas differ, or raise an actionable
schema-mismatch error; preserve the current creation path when the table is
absent and use _fetch_column_types and _clickhouse_type for the comparison.
- Around line 185-190: Update the nested-array handling in _clickhouse_type to
unwrap only a complete outer Nullable(...) wrapper from the recursive item type,
without removing a closing parenthesis from plain Array(...) types. Use an exact
helper for this wrapper removal so nested arrays generate balanced ClickHouse
type expressions.
- Around line 71-77: Update CHSeeder.seed_bronze and its related initialization
to accept and retain WorkerContext, then resolve each source schema through
worker_ctx.schema(schema). Use the resolved schema consistently for
_ensure_table, seed_records, ledger.record, and any selector inputs so seeding
matches the worker-scoped schema used by DbtRunner.
In `@src/ingestion/tests/e2e/lib/expect_engine.py`:
- Around line 136-142: Update the equal-rule handling in the expectation
evaluator to distinguish omitted fields from fields explicitly set to None:
check field presence on target before comparing its value, so an expected None
only passes when the field exists and is explicitly null. Preserve the existing
_values_equal comparison and ExpectError formatting for present fields.
In `@src/ingestion/tests/e2e/metrics/collab_activity.test.yaml`:
- Around line 82-98: Update the metric assertions to validate daily bucket
placement as well as values: in
src/ingestion/tests/e2e/metrics/collab_activity.test.yaml lines 82-98, add
representative expected bucket_start dates to the relevant assertions; in
src/ingestion/tests/e2e/metrics/collab_emails_read.test.yaml lines 58-60,
collab_emails_received.test.yaml lines 58-60, collab_emails_sent.test.yaml lines
63-65, and collab_files_engaged.test.yaml lines 63-65, require bucket_start to
equal '2026-12-25' while preserving the existing value checks.
- Around line 53-98: Restore empty-window coverage in the metric-results
fixtures: add an empty-period case with null period values to
collab_activity.test.yaml#L53-L98; restore the metric-results empty-window cases
in collab_emails_read.test.yaml#L34-L63,
collab_emails_received.test.yaml#L34-L63, and
collab_emails_sent.test.yaml#L39-L68; and add the expected empty-window case in
collab_files_engaged.test.yaml#L39-L68. Keep each simplified per-metric fixture
to one custom-window case and one empty-window case, asserting null rather than
omitted or zero-valued results.
In `@src/ingestion/tests/e2e/metrics/collab_emails_read.test.yaml`:
- Around line 61-63: Update the breakdown assertions in
src/ingestion/tests/e2e/metrics/collab_emails_read.test.yaml:61-63,
src/ingestion/tests/e2e/metrics/collab_emails_received.test.yaml:61-63,
src/ingestion/tests/e2e/metrics/collab_emails_sent.test.yaml:66-68, and
src/ingestion/tests/e2e/metrics/collab_files_engaged.test.yaml:66-68 to assert
both the tool dimension key and its expected concrete value, rather than
checking only d.key == 'tool'.
In `@src/ingestion/tests/e2e/metrics/collab_files_shared_external.test.yaml`:
- Around line 62-67: Update the collab.files_shared_external assertions in
src/ingestion/tests/e2e/metrics/collab_files_shared_external.test.yaml lines
62-67 to require bucket_start == '2026-12-25' in the timeseries assertion and
tool == 'm365' in the breakdown dimensions. Apply the same bucket_start date
requirement to both timeseries assertions and require tool == 'm365' in the
internal breakdown assertion in
src/ingestion/tests/e2e/metrics/collab_files_shared_internal.test.yaml lines
68-87.
In `@src/ingestion/tests/e2e/metrics/collab_meeting_free_days.test.yaml`:
- Around line 79-81: Update the collab.meeting_free_days assertion to require
exactly five points and validate that their bucket starts correspond to December
23, 24, 25, 26, and 27, with the expected values. Ensure the check is scoped to
erin@example.com so missing or duplicated meeting-free day buckets fail the
test.
In `@src/ingestion/tests/e2e/metrics/collab_meetings.test.yaml`:
- Around line 88-91: Update each timeseries assertion for collab.meeting_hours
and the referenced metric cases to include the expected bucket_start date
alongside the value in contains.points. Preserve the existing metric values and
entity filters while ensuring every assertion matches both the date bucket and
value.
In `@src/ingestion/tests/e2e/metrics/tasks_bugs.test.yaml`:
- Around line 367-380: Update the timeseries assertions to validate both the
expected bucket date and value, rather than value alone: in
src/ingestion/tests/e2e/metrics/tasks_bugs.test.yaml lines 367-380 require June
25 for both metrics; tasks_closed.test.yaml lines 292-294 require December 25;
tasks_dev_time.test.yaml lines 82-84 require March 25;
tasks_estimation_accuracy.test.yaml lines 88-90 require December 25;
tasks_flow_efficiency.test.yaml lines 91-93 require March 25; and
wiki_pages_created.test.yaml lines 64-66 require February 4.
In `@src/ingestion/tests/e2e/metrics/tasks_closed.test.yaml`:
- Around line 269-294: Restore empty-window contract coverage while preserving
each fixture’s existing custom-window case: in
src/ingestion/tests/e2e/metrics/tasks_closed.test.yaml lines 269-294,
tasks_dev_time.test.yaml lines 58-87, tasks_estimation_accuracy.test.yaml lines
65-90, and tasks_flow_efficiency.test.yaml lines 68-93, restore the removed
empty-period case; in tasks_bugs.test.yaml lines 342-380 and
wiki_edits.test.yaml lines 29-67, add disjoint empty-period cases covering both
metrics; in tasks_due_dates.test.yaml lines 364-418, add empty-view assertions
for all three metrics; and in wiki_pages_created.test.yaml lines 40-66,
implement the second empty-period case promised by the fixture description.
In `@src/ingestion/tests/e2e/metrics/wiki_comments.test.yaml`:
- Around line 46-72: Restore empty-window coverage for the migrated
/v1/metric-results fixtures: in
src/ingestion/tests/e2e/metrics/wiki_comments.test.yaml:46-72 add the documented
empty-window case; in
src/ingestion/tests/e2e/metrics/tasks_pickup_time.test.yaml:67-96,
tasks_reopen_rate.test.yaml:117-142, tasks_resolution_time.test.yaml:61-90, and
tasks_worklog_accuracy.test.yaml:72-97 restore one no-data case each; in
src/ingestion/tests/e2e/metrics/tasks_stale_in_progress.test.yaml:68-93 retain
the prior out-of-window assertion using the new response shape; and in
src/ingestion/tests/e2e/metrics/git_metrics.test.yaml:30-265 add one
unified-metrics request for an empty period. Keep each per-metric fixture to one
custom-window case plus one empty-window case.
In `@src/ingestion/tests/e2e/README.md`:
- Around line 97-108: Remove the stale legacy coverage instructions near the
metrics gate documentation, including the ad-hoc command that omits the required
--universe-file option and the “44/96 skip-listed” statement. Preserve the
current registry-driven gate instructions and their explicit no-skip-list
behavior.
---
Nitpick comments:
In `@src/ingestion/tests/e2e/metrics/collab_files_shared_external.test.yaml`:
- Around line 38-67: Restore empty-window coverage in all four fixtures: in
src/ingestion/tests/e2e/metrics/collab_files_shared_external.test.yaml (38-67)
add a period-only empty-window request asserting value 0; in
src/ingestion/tests/e2e/metrics/collab_files_shared_internal.test.yaml (38-87)
restore the removed equivalent case; and in
src/ingestion/tests/e2e/metrics/collab_meeting_free_days.test.yaml (55-81) and
src/ingestion/tests/e2e/metrics/collab_meetings.test.yaml (34-171) add the
period-only empty-window cases promised by their descriptions. Keep each fixture
to one custom-window case plus one empty-window case.
🪄 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: fc89ca71-b512-405d-9a5d-16dd66aeedfe
📒 Files selected for processing (113)
.github/workflows/e2e-bronze-to-api.ymlsrc/ingestion/tests/e2e/README.mdsrc/ingestion/tests/e2e/api/test_metric_results.pysrc/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/e2e.shsrc/ingestion/tests/e2e/lib/analytics.pysrc/ingestion/tests/e2e/lib/ch_seeder.pysrc/ingestion/tests/e2e/lib/collect_metric_definitions.pysrc/ingestion/tests/e2e/lib/collect_metrics.pysrc/ingestion/tests/e2e/lib/dbt_runner.pysrc/ingestion/tests/e2e/lib/expect_engine.pysrc/ingestion/tests/e2e/lib/fixture_loader.pysrc/ingestion/tests/e2e/lib/metric_coverage.pysrc/ingestion/tests/e2e/meta/test_expect_engine.pysrc/ingestion/tests/e2e/meta/test_metric_coverage.pysrc/ingestion/tests/e2e/metrics/ai_active_ai_members.test.yamlsrc/ingestion/tests/e2e/metrics/ai_active_days.test.yamlsrc/ingestion/tests/e2e/metrics/ai_ai_loc_share2.test.yamlsrc/ingestion/tests/e2e/metrics/ai_assistant_activity.test.yamlsrc/ingestion/tests/e2e/metrics/ai_assistant_messages.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cc_active.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cc_lines.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cc_overage.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cc_sessions.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cc_tool_accept.test.yamlsrc/ingestion/tests/e2e/metrics/ai_chatgpt_active.test.yamlsrc/ingestion/tests/e2e/metrics/ai_claude_web.test.yamlsrc/ingestion/tests/e2e/metrics/ai_codex_active.test.yamlsrc/ingestion/tests/e2e/metrics/ai_codex_lines.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cost.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cursor_acceptance.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cursor_agents.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cursor_completions.test.yamlsrc/ingestion/tests/e2e/metrics/ai_cursor_lines.test.yamlsrc/ingestion/tests/e2e/metrics/ai_dev_conversations.test.yamlsrc/ingestion/tests/e2e/metrics/ai_edit_acceptance.test.yamlsrc/ingestion/tests/e2e/metrics/ai_lines.test.yamlsrc/ingestion/tests/e2e/metrics/ai_prs_total.test.yamlsrc/ingestion/tests/e2e/metrics/ai_prs_with_cc.test.yamlsrc/ingestion/tests/e2e/metrics/ai_team_ai_loc.test.yamlsrc/ingestion/tests/e2e/metrics/collab_activity.test.yamlsrc/ingestion/tests/e2e/metrics/collab_emails_read.test.yamlsrc/ingestion/tests/e2e/metrics/collab_emails_received.test.yamlsrc/ingestion/tests/e2e/metrics/collab_emails_sent.test.yamlsrc/ingestion/tests/e2e/metrics/collab_files_engaged.test.yamlsrc/ingestion/tests/e2e/metrics/collab_files_shared_external.test.yamlsrc/ingestion/tests/e2e/metrics/collab_files_shared_internal.test.yamlsrc/ingestion/tests/e2e/metrics/collab_meeting_free.test.yamlsrc/ingestion/tests/e2e/metrics/collab_meeting_free_days.test.yamlsrc/ingestion/tests/e2e/metrics/collab_meeting_free_zoom.test.yamlsrc/ingestion/tests/e2e/metrics/collab_meeting_hours.test.yamlsrc/ingestion/tests/e2e/metrics/collab_meeting_hours_zoom_cross.test.yamlsrc/ingestion/tests/e2e/metrics/collab_meetings.test.yamlsrc/ingestion/tests/e2e/metrics/collab_meetings_count.test.yamlsrc/ingestion/tests/e2e/metrics/collab_meetings_count_zoom_cross.test.yamlsrc/ingestion/tests/e2e/metrics/collab_messages.test.yamlsrc/ingestion/tests/e2e/metrics/collab_teams_chats.test.yamlsrc/ingestion/tests/e2e/metrics/collab_teams_meeting_hours.test.yamlsrc/ingestion/tests/e2e/metrics/collab_teams_meetings.test.yamlsrc/ingestion/tests/e2e/metrics/collab_zoom_meeting_hours.test.yamlsrc/ingestion/tests/e2e/metrics/collab_zoom_meetings.test.yamlsrc/ingestion/tests/e2e/metrics/collab_zulip_chat.test.yamlsrc/ingestion/tests/e2e/metrics/git_metrics.test.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_claude_team.claude_team_overage_spend.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_confluence.wiki_pages.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_github.commits.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_github.file_changes.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_github.pull_requests.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_jira.jira_user.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_m365.sharepoint_activity.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_outline.wiki_page_versions.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_zoom.meetings.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_zoom.participants.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_zulip_proxy.messages.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_zulip_proxy.users.yamlsrc/ingestion/tests/e2e/metrics/task_delivery_bugs_to_task_ratio_ratiosum_jira.test.yamlsrc/ingestion/tests/e2e/metrics/task_delivery_status_category_jira.test.yamlsrc/ingestion/tests/e2e/metrics/task_delivery_task_reopen_rate_gate_jira.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_bugs.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_closed.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_dev_time.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_due_dates.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_estimation_accuracy.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_flow_efficiency.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_pickup_time.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_reopen_rate.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_resolution_time.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_stale_in_progress.test.yamlsrc/ingestion/tests/e2e/metrics/tasks_worklog_accuracy.test.yamlsrc/ingestion/tests/e2e/metrics/team_bullet_collab_emails_sent.test.yamlsrc/ingestion/tests/e2e/metrics/team_bullet_task_delivery_tasks_completed.test.yamlsrc/ingestion/tests/e2e/metrics/templates/claude_team_overage.yamlsrc/ingestion/tests/e2e/metrics/templates/confluence_wiki_footer_comments.yamlsrc/ingestion/tests/e2e/metrics/templates/confluence_wiki_pages.yamlsrc/ingestion/tests/e2e/metrics/templates/git_activity.yamlsrc/ingestion/tests/e2e/metrics/templates/jira_task.yamlsrc/ingestion/tests/e2e/metrics/templates/m365_sharepoint.yamlsrc/ingestion/tests/e2e/metrics/templates/people.yamlsrc/ingestion/tests/e2e/metrics/templates/wiki_page_versions.yamlsrc/ingestion/tests/e2e/metrics/templates/zoom.yamlsrc/ingestion/tests/e2e/metrics/templates/zulip.yamlsrc/ingestion/tests/e2e/metrics/test_fixtures.pysrc/ingestion/tests/e2e/metrics/wiki_comments.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_confluence_active_authors.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_confluence_comments.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_confluence_edits.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_confluence_pages_created.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_edits.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_outline_active_authors.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_outline_comments.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_outline_edits.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_outline_pages_created.test.yamlsrc/ingestion/tests/e2e/metrics/wiki_pages_created.test.yaml
💤 Files with no reviewable changes (57)
- src/ingestion/tests/e2e/metrics/ai_cursor_acceptance.test.yaml
- src/ingestion/tests/e2e/metrics/team_bullet_collab_emails_sent.test.yaml
- src/ingestion/tests/e2e/metrics/templates/confluence_wiki_pages.yaml
- src/ingestion/tests/e2e/metrics/ai_codex_active.test.yaml
- src/ingestion/tests/e2e/metrics/ai_cc_sessions.test.yaml
- src/ingestion/tests/e2e/metrics/collab_zoom_meeting_hours.test.yaml
- src/ingestion/tests/e2e/metrics/ai_team_ai_loc.test.yaml
- src/ingestion/tests/e2e/metrics/ai_active_ai_members.test.yaml
- src/ingestion/tests/e2e/metrics/ai_cc_lines.test.yaml
- src/ingestion/tests/e2e/metrics/ai_cursor_lines.test.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_m365.sharepoint_activity.yaml
- src/ingestion/tests/e2e/metrics/ai_cursor_agents.test.yaml
- src/ingestion/tests/e2e/metrics/collab_zoom_meetings.test.yaml
- src/ingestion/tests/e2e/metrics/templates/confluence_wiki_footer_comments.yaml
- src/ingestion/tests/e2e/metrics/templates/m365_sharepoint.yaml
- src/ingestion/tests/e2e/metrics/ai_chatgpt_active.test.yaml
- src/ingestion/tests/e2e/metrics/wiki_outline_edits.test.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_zoom.meetings.yaml
- src/ingestion/tests/e2e/metrics/templates/claude_team_overage.yaml
- src/ingestion/tests/e2e/metrics/ai_prs_with_cc.test.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_confluence.wiki_pages.yaml
- src/ingestion/tests/e2e/metrics/ai_cc_tool_accept.test.yaml
- src/ingestion/tests/e2e/metrics/wiki_confluence_active_authors.test.yaml
- src/ingestion/tests/e2e/metrics/collab_teams_meeting_hours.test.yaml
- src/ingestion/tests/e2e/metrics/collab_meeting_hours_zoom_cross.test.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_zulip_proxy.users.yaml
- src/ingestion/tests/e2e/metrics/team_bullet_task_delivery_tasks_completed.test.yaml
- src/ingestion/tests/e2e/metrics/wiki_outline_comments.test.yaml
- src/ingestion/tests/e2e/metrics/collab_meetings_count.test.yaml
- src/ingestion/tests/e2e/metrics/ai_cc_overage.test.yaml
- src/ingestion/tests/e2e/metrics/templates/zoom.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_claude_team.claude_team_overage_spend.yaml
- src/ingestion/tests/e2e/metrics/wiki_outline_active_authors.test.yaml
- src/ingestion/tests/e2e/metrics/collab_meetings_count_zoom_cross.test.yaml
- src/ingestion/tests/e2e/metrics/task_delivery_status_category_jira.test.yaml
- src/ingestion/tests/e2e/metrics/task_delivery_task_reopen_rate_gate_jira.test.yaml
- src/ingestion/tests/e2e/metrics/ai_cursor_completions.test.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_zoom.participants.yaml
- src/ingestion/tests/e2e/metrics/collab_meeting_hours.test.yaml
- src/ingestion/tests/e2e/metrics/wiki_confluence_comments.test.yaml
- src/ingestion/tests/e2e/lib/collect_metrics.py
- src/ingestion/tests/e2e/metrics/ai_prs_total.test.yaml
- src/ingestion/tests/e2e/metrics/templates/zulip.yaml
- src/ingestion/tests/e2e/metrics/ai_claude_web.test.yaml
- src/ingestion/tests/e2e/metrics/schemas/bronze_zulip_proxy.messages.yaml
- src/ingestion/tests/e2e/metrics/wiki_confluence_edits.test.yaml
- src/ingestion/tests/e2e/metrics/collab_zulip_chat.test.yaml
- src/ingestion/tests/e2e/metrics/collab_meeting_free.test.yaml
- src/ingestion/tests/e2e/metrics/ai_cc_active.test.yaml
- src/ingestion/tests/e2e/metrics/collab_teams_meetings.test.yaml
- src/ingestion/tests/e2e/metrics/collab_teams_chats.test.yaml
- src/ingestion/tests/e2e/metrics/wiki_confluence_pages_created.test.yaml
- src/ingestion/tests/e2e/metrics/task_delivery_bugs_to_task_ratio_ratiosum_jira.test.yaml
- src/ingestion/tests/e2e/metrics/ai_ai_loc_share2.test.yaml
- src/ingestion/tests/e2e/metrics/collab_meeting_free_zoom.test.yaml
- src/ingestion/tests/e2e/metrics/wiki_outline_pages_created.test.yaml
- src/ingestion/tests/e2e/metrics/ai_codex_lines.test.yaml
Comment on lines
214
to
+220
| out_dir = Path(__file__).parent / ".artifacts" | ||
| result = subprocess.run( | ||
| [ | ||
| sys.executable, | ||
| str(script), | ||
| "--url", | ||
| proc.base_url, | ||
| "--out-dir", | ||
| str(out_dir), | ||
| "--tenant", | ||
| str(TEST_TENANT_ID), | ||
| ], | ||
| check=False, | ||
| ) | ||
| if result.returncode != 0: | ||
| try: | ||
| collect(cfg, out_dir) | ||
| except Exception as error: | ||
| LOG.warning( | ||
| "coverage-artifact collection failed (rc=%d); gate jobs may lack inputs", | ||
| result.returncode, | ||
| "coverage-artifact collection failed: %s; gate jobs may lack inputs", | ||
| error, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail when coverage artifacts cannot be refreshed.
Suppressing this error can leave an old metric_definitions.json in .artifacts, allowing the registry coverage gate to validate stale definitions. Let collection failure fail the session; the enclosing finally still stops analytics.
Proposed fix
out_dir = Path(__file__).parent / ".artifacts"
- try:
- collect(cfg, out_dir)
- except Exception as error:
- LOG.warning(
- "coverage-artifact collection failed: %s; gate jobs may lack inputs",
- error,
- )
+ collect(cfg, out_dir)📝 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
| out_dir = Path(__file__).parent / ".artifacts" | |
| result = subprocess.run( | |
| [ | |
| sys.executable, | |
| str(script), | |
| "--url", | |
| proc.base_url, | |
| "--out-dir", | |
| str(out_dir), | |
| "--tenant", | |
| str(TEST_TENANT_ID), | |
| ], | |
| check=False, | |
| ) | |
| if result.returncode != 0: | |
| try: | |
| collect(cfg, out_dir) | |
| except Exception as error: | |
| LOG.warning( | |
| "coverage-artifact collection failed (rc=%d); gate jobs may lack inputs", | |
| result.returncode, | |
| "coverage-artifact collection failed: %s; gate jobs may lack inputs", | |
| error, | |
| out_dir = Path(__file__).parent / ".artifacts" | |
| collect(cfg, out_dir) |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 217-217: Do not catch blind exception: Exception
(BLE001)
🤖 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/conftest.py` around lines 214 - 220, Update the
coverage-artifact collection flow around collect in the test session fixture to
stop suppressing collection errors: remove the warning-only exception handling
or re-raise the caught exception after any required logging. Ensure collect
failures propagate and fail the session, while preserving the enclosing finally
cleanup that stops analytics.
Source: Linters/SAST tools
Comment on lines
+71
to
77
| def seed_bronze(self, bronze: dict[str, list[dict]], schemas: dict[str, dict]) -> None: | ||
| """Seed every `<db>.<table>: [records]` entry of a TestYaml.""" | ||
| for table_fqn, rows in bronze.items(): | ||
| schema, _, table = table_fqn.partition(".") | ||
| self._ensure_table(schema, table, schemas[table_fqn]) | ||
| self.seed_records(schema, table, rows) | ||
| self.ledger.record(schema, table) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Seed into the worker-scoped bronze schema.
DbtRunner passes the worker ID to dbt, while this method creates and writes the unsuffixed fixture FQN. Under xdist, workers can collide in the same bronze tables or dbt can read a suffixed schema that was never seeded. Inject WorkerContext into CHSeeder and consistently use worker_ctx.schema(schema) for creation, insertion, ledger entries, and selector inputs.
🤖 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/lib/ch_seeder.py` around lines 71 - 77, Update
CHSeeder.seed_bronze and its related initialization to accept and retain
WorkerContext, then resolve each source schema through
worker_ctx.schema(schema). Use the resolved schema consistently for
_ensure_table, seed_records, ledger.record, and any selector inputs so seeding
matches the worker-scoped schema used by DbtRunner.
Comment on lines
+136
to
+147
| def _ensure_table(self, database: str, table: str, schema: dict[str, Any]) -> None: | ||
| if self._fetch_column_types(database, table): | ||
| return | ||
| columns = ", ".join( | ||
| f"`{name}` {_clickhouse_type(definition)}" | ||
| for name, definition in schema.get("properties", {}).items() | ||
| ) | ||
| ch.execute(self.cfg, f"CREATE DATABASE IF NOT EXISTS `{database}`") | ||
| ch.execute( | ||
| self.cfg, | ||
| f"CREATE TABLE `{database}`.`{table}` ({columns}) ENGINE = MergeTree ORDER BY tuple()", | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not accept an existing table with a stale schema.
Any non-empty table definition returns immediately. After a fixture schema changes, warm runs therefore retain missing or incompatible columns and can either fail downstream dbt models or seed data using the wrong types. Compare the actual columns with the generated schema and recreate the fixture-owned bronze table, or fail with an actionable schema-mismatch error.
🤖 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/lib/ch_seeder.py` around lines 136 - 147, The
_ensure_table method must validate an existing table’s columns and types against
the generated schema instead of returning whenever _fetch_column_types returns a
non-empty result. Recreate the fixture-owned bronze table when the schemas
differ, or raise an actionable schema-mismatch error; preserve the current
creation path when the table is absent and use _fetch_column_types and
_clickhouse_type for the comparison.
Comment on lines
+185
to
+190
| def _clickhouse_type(definition: dict[str, Any]) -> str: | ||
| declared = definition.get("type", "string") | ||
| types = declared if isinstance(declared, list) else [declared] | ||
| base = next((item for item in types if item != "null"), "string") | ||
| if base == "array": | ||
| return f"Array({_clickhouse_type(definition.get('items', {})).removeprefix('Nullable(').removesuffix(')')})" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix nested-array type generation.
For nested arrays, _clickhouse_type(items) returns Array(...); removesuffix(")") then removes its closing parenthesis even though no Nullable( prefix was removed, producing malformed SQL such as Array(Array(Nullable(String)).
Use an exact helper that unwraps only a complete outer Nullable(...) wrapper.
🤖 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/lib/ch_seeder.py` around lines 185 - 190, Update the
nested-array handling in _clickhouse_type to unwrap only a complete outer
Nullable(...) wrapper from the recursive item type, without removing a closing
parenthesis from plain Array(...) types. Use an exact helper for this wrapper
removal so nested arrays generate balanced ClickHouse type expressions.
Comment on lines
136
to
+142
| if "equal" in rule: | ||
| if it is None: | ||
| raise ExpectError(f"{where}: `equal` requires a `find` that selects one row") | ||
| entry["asserted"] |= {k for k in rule["equal"] if k in _STAT_FIELDS} | ||
| for field, exp in rule["equal"].items(): | ||
| got = it.get(field) | ||
| if not _values_equal(got, exp): | ||
| raise ExpectError(f"{where}: {field}: expected {exp!r}, got {got!r}") | ||
| if target is None: | ||
| raise ExpectError(f"{where}: `equal` requires `metric`, `view`, or `find`") | ||
| for field, expected in rule["equal"].items(): | ||
| got = target.get(field) | ||
| if not _values_equal(got, expected): | ||
| raise ExpectError(f"{where}: {field}: expected {expected!r}, got {got!r}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Distinguish missing fields from explicit null.
target.get(field) lets an omitted field satisfy an expected None, so nullable-field assertions can miss response contract regressions.
Proposed fix
for field, expected in rule["equal"].items():
- got = target.get(field)
+ if field not in target:
+ raise ExpectError(f"{where}: {field}: field is missing")
+ got = target[field]
if not _values_equal(got, expected):📝 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
| if "equal" in rule: | |
| if it is None: | |
| raise ExpectError(f"{where}: `equal` requires a `find` that selects one row") | |
| entry["asserted"] |= {k for k in rule["equal"] if k in _STAT_FIELDS} | |
| for field, exp in rule["equal"].items(): | |
| got = it.get(field) | |
| if not _values_equal(got, exp): | |
| raise ExpectError(f"{where}: {field}: expected {exp!r}, got {got!r}") | |
| if target is None: | |
| raise ExpectError(f"{where}: `equal` requires `metric`, `view`, or `find`") | |
| for field, expected in rule["equal"].items(): | |
| got = target.get(field) | |
| if not _values_equal(got, expected): | |
| raise ExpectError(f"{where}: {field}: expected {expected!r}, got {got!r}") | |
| if "equal" in rule: | |
| if target is None: | |
| raise ExpectError(f"{where}: `equal` requires `metric`, `view`, or `find`") | |
| for field, expected in rule["equal"].items(): | |
| if field not in target: | |
| raise ExpectError(f"{where}: {field}: field is missing") | |
| got = target[field] | |
| if not _values_equal(got, expected): | |
| raise ExpectError(f"{where}: {field}: expected {expected!r}, got {got!r}") |
🤖 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/lib/expect_engine.py` around lines 136 - 142, Update
the equal-rule handling in the expectation evaluator to distinguish omitted
fields from fields explicitly set to None: check field presence on target before
comparing its value, so an expected None only passes when the field exists and
is explicitly null. Preserve the existing _values_equal comparison and
ExpectError formatting for present fields.
Comment on lines
+88
to
+91
| - metric: collab.meeting_hours | ||
| view: timeseries | ||
| find: { entity_id: erin@example.com } | ||
| contains: { points: { value: 5 } } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the expected date in every timeseries assertion.
contains: { points: { value: ... } } can pass when the value is assigned to the wrong bucket. Match both bucket_start: "2026-12-25" and the expected value for each metric.
Also applies to: 104-107, 120-123, 136-139, 152-155, 168-171
🤖 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/collab_meetings.test.yaml` around lines 88 -
91, Update each timeseries assertion for collab.meeting_hours and the referenced
metric cases to include the expected bucket_start date alongside the value in
contains.points. Preserve the existing metric values and entity filters while
ensuring every assertion matches both the date bucket and value.
Comment on lines
+367
to
+380
| - metric: tasks.bugs_fixed | ||
| view: timeseries | ||
| assert: "items.exists(s, s.entity_id == 'erin@example.com' && s.points.exists(p, double(p.value) == 4.0))" | ||
| - metric: tasks.bugs_ratio | ||
| view: period | ||
| find: { entity_id: erin@example.com } | ||
| equal: { value: 100 } | ||
| - metric: tasks.bugs_ratio | ||
| view: peer | ||
| find: { entity_id: erin@example.com } | ||
| equal: { target_value: 100, p25: null, median: null, p75: null, min: null, max: null, n: 4 } | ||
| - metric: tasks.bugs_ratio | ||
| view: timeseries | ||
| assert: "items.exists(s, s.entity_id == 'erin@example.com' && s.points.exists(p, double(p.value) == 100.0))" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the expected day in each timeseries assertion. Value-only predicates allow incorrect bucket dates to pass.
src/ingestion/tests/e2e/metrics/tasks_bugs.test.yaml#L367-L380: require June 25 for both metrics.src/ingestion/tests/e2e/metrics/tasks_closed.test.yaml#L292-L294: require December 25.src/ingestion/tests/e2e/metrics/tasks_dev_time.test.yaml#L82-L84: require March 25.src/ingestion/tests/e2e/metrics/tasks_estimation_accuracy.test.yaml#L88-L90: require December 25.src/ingestion/tests/e2e/metrics/tasks_flow_efficiency.test.yaml#L91-L93: require March 25.src/ingestion/tests/e2e/metrics/wiki_pages_created.test.yaml#L64-L66: require February 4.
📍 Affects 6 files
src/ingestion/tests/e2e/metrics/tasks_bugs.test.yaml#L367-L380(this comment)src/ingestion/tests/e2e/metrics/tasks_closed.test.yaml#L292-L294src/ingestion/tests/e2e/metrics/tasks_dev_time.test.yaml#L82-L84src/ingestion/tests/e2e/metrics/tasks_estimation_accuracy.test.yaml#L88-L90src/ingestion/tests/e2e/metrics/tasks_flow_efficiency.test.yaml#L91-L93src/ingestion/tests/e2e/metrics/wiki_pages_created.test.yaml#L64-L66
🤖 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/tasks_bugs.test.yaml` around lines 367 - 380,
Update the timeseries assertions to validate both the expected bucket date and
value, rather than value alone: in
src/ingestion/tests/e2e/metrics/tasks_bugs.test.yaml lines 367-380 require June
25 for both metrics; tasks_closed.test.yaml lines 292-294 require December 25;
tasks_dev_time.test.yaml lines 82-84 require March 25;
tasks_estimation_accuracy.test.yaml lines 88-90 require December 25;
tasks_flow_efficiency.test.yaml lines 91-93 require March 25; and
wiki_pages_created.test.yaml lines 64-66 require February 4.
Comment on lines
+269
to
+294
| - name: tasks.closed | ||
| request: | ||
| url: /v1/metrics/queries | ||
| url: /v1/metric-results | ||
| method: POST | ||
| body: | ||
| queries: | ||
| - id: task_delivery | ||
| metric_id: 00000000-0000-0000-0001-000000000011 | ||
| $top: 50 | ||
| $filter: "person_id eq 'erin@example.com' and metric_date ge '2026-12-20' and metric_date le '2026-12-31'" | ||
| $orderby: metric_key | ||
| entity: { type: person, ids: [erin@example.com] } | ||
| period: { from: "2026-12-20", to: "2026-12-31" } | ||
| metrics: | ||
| - metric_key: tasks.closed | ||
| views: | ||
| - { view: period } | ||
| - { view: peer } | ||
| - { view: timeseries, bucket: day } | ||
| expect: | ||
| - assert: "status == 200" | ||
| - in: task_delivery | ||
| assert: "result.status == 'ok'" | ||
| - in: task_delivery | ||
| find: { metric_key: tasks_completed } | ||
| equal: { value: 5, median: 3, range_min: 1, range_max: 5, p25: 2, p75: 4 } | ||
|
|
||
| - name: "tasks_completed — empty window (no data → no items)" | ||
| request: | ||
| url: /v1/metrics/queries | ||
| method: POST | ||
| body: | ||
| queries: | ||
| - id: task_delivery | ||
| metric_id: 00000000-0000-0000-0001-000000000011 | ||
| $top: 50 | ||
| $filter: "person_id eq 'erin@example.com' and metric_date ge '2026-06-01' and metric_date le '2026-06-30'" | ||
| $orderby: metric_key | ||
| expect: | ||
| - assert: "status == 200" | ||
| - in: task_delivery | ||
| assert: "result.status == 'ok'" | ||
| - in: task_delivery | ||
| assert: "size(items) == 0" | ||
| - metric: tasks.closed | ||
| view: period | ||
| find: { entity_id: erin@example.com } | ||
| equal: { value: 5 } | ||
| - metric: tasks.closed | ||
| view: peer | ||
| find: { entity_id: erin@example.com } | ||
| equal: { target_value: 5, p25: 2, median: 3, p75: 4, min: 1, max: 5, n: 5 } | ||
| - metric: tasks.closed | ||
| view: timeseries | ||
| assert: "items.exists(s, s.entity_id == 'erin@example.com' && s.points.exists(p, double(p.value) == 5.0))" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore empty-window contract coverage across the migrated fixtures. Each fixture now exercises only populated responses, leaving the no-data behavior untested.
src/ingestion/tests/e2e/metrics/tasks_closed.test.yaml#L269-L294: restore the removed empty-period case.src/ingestion/tests/e2e/metrics/tasks_bugs.test.yaml#L342-L380: add a disjoint empty-period case for both bug metrics.src/ingestion/tests/e2e/metrics/tasks_dev_time.test.yaml#L58-L87: restore the removed empty-period case.src/ingestion/tests/e2e/metrics/tasks_due_dates.test.yaml#L364-L418: add empty-view assertions for all three metrics.src/ingestion/tests/e2e/metrics/tasks_estimation_accuracy.test.yaml#L65-L90: restore the removed empty-period case.src/ingestion/tests/e2e/metrics/tasks_flow_efficiency.test.yaml#L68-L93: restore the removed empty-period case.src/ingestion/tests/e2e/metrics/wiki_edits.test.yaml#L29-L67: add an empty-period case for both wiki metrics.src/ingestion/tests/e2e/metrics/wiki_pages_created.test.yaml#L40-L66: implement the second case promised by the description.
Based on learnings, each per-metric fixture should retain one custom-window case plus an empty-window case.
📍 Affects 8 files
src/ingestion/tests/e2e/metrics/tasks_closed.test.yaml#L269-L294(this comment)src/ingestion/tests/e2e/metrics/tasks_bugs.test.yaml#L342-L380src/ingestion/tests/e2e/metrics/tasks_dev_time.test.yaml#L58-L87src/ingestion/tests/e2e/metrics/tasks_due_dates.test.yaml#L364-L418src/ingestion/tests/e2e/metrics/tasks_estimation_accuracy.test.yaml#L65-L90src/ingestion/tests/e2e/metrics/tasks_flow_efficiency.test.yaml#L68-L93src/ingestion/tests/e2e/metrics/wiki_edits.test.yaml#L29-L67src/ingestion/tests/e2e/metrics/wiki_pages_created.test.yaml#L40-L66
🤖 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/tasks_closed.test.yaml` around lines 269 -
294, Restore empty-window contract coverage while preserving each fixture’s
existing custom-window case: in
src/ingestion/tests/e2e/metrics/tasks_closed.test.yaml lines 269-294,
tasks_dev_time.test.yaml lines 58-87, tasks_estimation_accuracy.test.yaml lines
65-90, and tasks_flow_efficiency.test.yaml lines 68-93, restore the removed
empty-period case; in tasks_bugs.test.yaml lines 342-380 and
wiki_edits.test.yaml lines 29-67, add disjoint empty-period cases covering both
metrics; in tasks_due_dates.test.yaml lines 364-418, add empty-view assertions
for all three metrics; and in wiki_pages_created.test.yaml lines 40-66,
implement the second empty-period case promised by the fixture description.
Source: Learnings
Comment on lines
+46
to
+72
| cases: | ||
| - name: wiki.comments | ||
| request: | ||
| url: /v1/metric-results | ||
| method: POST | ||
| body: | ||
| entity: { type: person, ids: [erin@example.com] } | ||
| period: { from: "2026-08-02", to: "2026-08-08" } | ||
| metrics: | ||
| - metric_key: wiki.comments | ||
| views: | ||
| - { view: period } | ||
| - { view: peer } | ||
| - { view: timeseries, bucket: day } | ||
| expect: | ||
| - assert: "status == 200" | ||
| - metric: wiki.comments | ||
| view: period | ||
| find: { entity_id: erin@example.com } | ||
| equal: { value: 5 } | ||
| - metric: wiki.comments | ||
| view: peer | ||
| find: { entity_id: erin@example.com } | ||
| equal: { target_value: 5, p25: 2, median: 3, p75: 4, min: 1, max: 5, n: 5 } | ||
| - metric: wiki.comments | ||
| view: timeseries | ||
| assert: "items.exists(s, s.entity_id == 'erin@example.com' && s.points.exists(p, double(p.value) == 5.0))" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore empty-window coverage after the endpoint migration.
These fixtures now verify only populated responses, leaving /v1/metric-results no-data behavior uncovered. The wiki fixture additionally promises a second empty-window case in its description.
src/ingestion/tests/e2e/metrics/wiki_comments.test.yaml#L46-L72: add the documented empty-window case.src/ingestion/tests/e2e/metrics/tasks_pickup_time.test.yaml#L67-L96: restore the removed no-data case.src/ingestion/tests/e2e/metrics/tasks_reopen_rate.test.yaml#L117-L142: restore the removed no-data case.src/ingestion/tests/e2e/metrics/tasks_resolution_time.test.yaml#L61-L90: restore the removed no-data case.src/ingestion/tests/e2e/metrics/tasks_stale_in_progress.test.yaml#L68-L93: retain the previous out-of-window assertion using the new response shape.src/ingestion/tests/e2e/metrics/tasks_worklog_accuracy.test.yaml#L72-L97: restore the removed no-data case.src/ingestion/tests/e2e/metrics/git_metrics.test.yaml#L30-L265: add one request covering the unified metrics over an empty period.
Based on learnings, each per-metric fixture should retain one custom-window case plus one empty-window case.
📍 Affects 7 files
src/ingestion/tests/e2e/metrics/wiki_comments.test.yaml#L46-L72(this comment)src/ingestion/tests/e2e/metrics/tasks_pickup_time.test.yaml#L67-L96src/ingestion/tests/e2e/metrics/tasks_reopen_rate.test.yaml#L117-L142src/ingestion/tests/e2e/metrics/tasks_resolution_time.test.yaml#L61-L90src/ingestion/tests/e2e/metrics/tasks_stale_in_progress.test.yaml#L68-L93src/ingestion/tests/e2e/metrics/tasks_worklog_accuracy.test.yaml#L72-L97src/ingestion/tests/e2e/metrics/git_metrics.test.yaml#L30-L265
🤖 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/wiki_comments.test.yaml` around lines 46 -
72, Restore empty-window coverage for the migrated /v1/metric-results fixtures:
in src/ingestion/tests/e2e/metrics/wiki_comments.test.yaml:46-72 add the
documented empty-window case; in
src/ingestion/tests/e2e/metrics/tasks_pickup_time.test.yaml:67-96,
tasks_reopen_rate.test.yaml:117-142, tasks_resolution_time.test.yaml:61-90, and
tasks_worklog_accuracy.test.yaml:72-97 restore one no-data case each; in
src/ingestion/tests/e2e/metrics/tasks_stale_in_progress.test.yaml:68-93 retain
the prior out-of-window assertion using the new response shape; and in
src/ingestion/tests/e2e/metrics/git_metrics.test.yaml:30-265 add one
unified-metrics request for an empty period. Keep each per-metric fixture to one
custom-window case plus one empty-window case.
Source: Learnings
Comment on lines
+97
to
+108
| A job (`metric-coverage-gate`) in the **E2E — Bronze to API** workflow checks enabled builtin product definitions. The metrics lane snapshots `metric_key`, computation, dimensions, and cohort metadata to `.artifacts/metric_definitions.json`; the gate then verifies every builtin has period, peer, and timeseries assertions, plus breakdown assertions for declared dimensions and histogram assertions for median metrics. | ||
|
|
||
| Locally, after a run: | ||
|
|
||
| ```bash | ||
| ./e2e.sh test metrics/ # runs the metrics suite (emits both .artifacts files; only catalog_metrics.json feeds this gate) | ||
| ./e2e.sh test metrics/ # runs the metrics suite and writes metric_definitions.json | ||
| ./e2e.sh gates metrics # metric gate only, against .artifacts/ (in the runner image; no DB) | ||
| ``` | ||
|
|
||
| `./e2e.sh gates` with no argument runs both gates (handy after running both suites locally; see [API endpoint coverage gate](#api-endpoint-coverage-gate) below for the api/-only equivalent). `gates api` / `gates metrics` run one gate against one artifact each — that per-lane shape is what mirrors the two independent CI jobs, each of which only ever needs its own lane's artifact. | ||
|
|
||
| The verdict per **metric_key** (each individual number) is **binary**: | ||
|
|
||
| - **value-tested** — a `metrics/*.test.yaml` asserts it (`find: {metric_key: …}` paired with `equal`/`assert`) → **PASS** | ||
| - **skip-listed** (in the inline `SKIP_LIST` in [`lib/metric_coverage.py`](lib/metric_coverage.py)) → **PASS** (baseline) | ||
| - **neither** → **FAIL** — a number nobody validates must get an assertion or a `SKIP_LIST` entry. | ||
|
|
||
| Catalog keys are dotted (`collab_bullet_rows.m365_emails_sent`); a test asserts the bare response key (`m365_emails_sent`). The column suffix is unique across the catalog, so the gate maps bare→dotted by suffix (a future collision raises). `SKIP_LIST` is the accepted baseline and single source of truth (no side-car file — just `(metric_key, reason)`). Kept honest: a **stale** entry (key no longer in the catalog), a **redundant** one (now value-tested), or a test asserting a **non-catalog** key (typo / unseeded → matches 0 rows) all fail. PASS iff no FAILs. | ||
| Missing views, unknown asserted keys, unknown requested keys, and stale fixtures fail. There are no legacy suffix mappings or skip lists. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the remaining legacy coverage instructions.
The following Lines 110-115 are now stale: the ad-hoc command omits required --universe-file, and the “44/96 skip-listed” statement contradicts this section’s registry-driven, no-skip-list gate.
🧰 Tools
🪛 LanguageTool
[grammar] ~97-~97: Ensure spelling is correct
Context: ...Bronze to API** workflow checks enabled builtin product definitions. The metrics lane s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~97-~97: Ensure spelling is correct
Context: ...ons.json`; the gate then verifies every builtin has period, peer, and timeseries assert...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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/README.md` around lines 97 - 108, Remove the stale
legacy coverage instructions near the metrics gate documentation, including the
ad-hoc command that omits the required --universe-file option and the “44/96
skip-listed” statement. Preserve the current registry-driven gate instructions
and their explicit no-skip-list behavior.
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
3 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.
Summary
/v1/metric-results./v1/metrics/queriesusage from the declarative metric suite.Closes #1699
Validation
Summary by CodeRabbit
New Features
/v1/metric-resultsAPI workflow.Documentation
Bug Fixes