test(e2e): per-metric collaboration specs for the remaining m365 metrics (#1413) - #1476
Merged
ktursunov merged 6 commits intoJun 25, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds collaboration metric E2E specs, bronze schemas and reusable templates, Zoom placeholder support, and metric-e2e guidance updates. ChangesM365 Collaboration E2E Specs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
ktursunov
force-pushed
the
m365-e2e-collab-rest
branch
from
June 24, 2026 20:03
50c7e45 to
b844ec5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/ingestion/tests/e2e/specs/collab_meeting_hours.test.yaml (2)
104-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
result.status == 'ok'checks to bob/carol cases for parity.Only the alice case validates query status explicitly; adding the same assertion to bob/carol makes failure mode clearer and consistent.
Suggested patch
- name: "meeting_hours — bob Teams-only (1.5)" @@ expect: - assert: "status == 200" + - in: collaboration + assert: "result.status == 'ok'" - in: collaboration find: { metric_key: meeting_hours } equal: { value: 1.5, median: 1.5, range_min: 0.5, range_max: 2.0 } @@ - name: "meeting_hours — carol Zoom-only (0.5)" @@ expect: - assert: "status == 200" + - in: collaboration + assert: "result.status == 'ok'" - in: collaboration find: { metric_key: meeting_hours } equal: { value: 0.5, median: 1.5, range_min: 0.5, range_max: 2.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/collab_meeting_hours.test.yaml` around lines 104 - 124, Add the same query-status assertion used in the alice case to the bob and carol scenarios in collab_meeting_hours.test.yaml so all three cases validate the result state consistently. Update the expect blocks for the bob Zoom-only and carol Zoom-only tests to check result.status == 'ok' alongside the existing status == 200 and metric assertions, using the same query structure and expectation style already present in the alice case.
58-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin Zoom modality flags explicitly in-spec.
This spec currently depends on template defaults for video/screen-share gating; setting them here avoids future template drift changing meeting-hour outcomes.
Suggested patch
bronze_zoom.participants: - $ref: templates/zoom.yaml#/templates/alice_zoom meeting_uuid: zmtg-alice-1 participant_uuid: zp-alice-1 join_time: "2026-01-05T10:00:00" leave_time: "2026-01-05T11:00:00" + camera: 0 + share_application: 0 + share_desktop: 0 + share_whiteboard: 0 - $ref: templates/zoom.yaml#/templates/alice_zoom # verbatim duplicate -> must dedup, not double meeting_uuid: zmtg-alice-1 participant_uuid: zp-alice-1 join_time: "2026-01-05T10:00:00" leave_time: "2026-01-05T11:00:00" + camera: 0 + share_application: 0 + share_desktop: 0 + share_whiteboard: 0 - $ref: templates/zoom.yaml#/templates/carol_zoom meeting_uuid: zmtg-carol-1 participant_uuid: zp-carol-1 join_time: "2026-01-05T14:00:00" leave_time: "2026-01-05T14:30:00" + camera: 0 + share_application: 0 + share_desktop: 0 + share_whiteboard: 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/collab_meeting_hours.test.yaml` around lines 58 - 73, The Zoom meeting-hour e2e spec currently relies on template defaults for modality gating, which can change outcomes if the template drifts. Update the collab_meeting_hours test entries that use alice_zoom and carol_zoom to set the Zoom modality flags explicitly in the spec so the expected hour calculations stay stable. Keep the existing duplicated alice entry behavior intact and make the changes in the YAML spec itself rather than in the shared templates.
🤖 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/collab_meeting_hours.test.yaml`:
- Around line 104-124: Add the same query-status assertion used in the alice
case to the bob and carol scenarios in collab_meeting_hours.test.yaml so all
three cases validate the result state consistently. Update the expect blocks for
the bob Zoom-only and carol Zoom-only tests to check result.status == 'ok'
alongside the existing status == 200 and metric assertions, using the same query
structure and expectation style already present in the alice case.
- Around line 58-73: The Zoom meeting-hour e2e spec currently relies on template
defaults for modality gating, which can change outcomes if the template drifts.
Update the collab_meeting_hours test entries that use alice_zoom and carol_zoom
to set the Zoom modality flags explicitly in the spec so the expected hour
calculations stay stable. Keep the existing duplicated alice entry behavior
intact and make the changes in the YAML spec itself rather than in the shared
templates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b081df6d-6981-4972-b718-01e143bdfb0f
📥 Commits
Reviewing files that changed from the base of the PR and between 50c7e4560269e394e97370b78fc72a910c42cac3 and b844ec5ddaff7f4402b33e921493205b9deb240e.
📒 Files selected for processing (25)
.claude/skills/metric-e2e-test/SKILL.mdsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/specs/collab_active_days.test.yamlsrc/ingestion/tests/e2e/specs/collab_emails_read.test.yamlsrc/ingestion/tests/e2e/specs/collab_emails_received.test.yamlsrc/ingestion/tests/e2e/specs/collab_emails_sent.test.yamlsrc/ingestion/tests/e2e/specs/collab_files_engaged.test.yamlsrc/ingestion/tests/e2e/specs/collab_files_shared_external.test.yamlsrc/ingestion/tests/e2e/specs/collab_files_shared_internal.test.yamlsrc/ingestion/tests/e2e/specs/collab_meeting_free.test.yamlsrc/ingestion/tests/e2e/specs/collab_meeting_hours.test.yamlsrc/ingestion/tests/e2e/specs/collab_meetings_count.test.yamlsrc/ingestion/tests/e2e/specs/collab_teams_chats.test.yamlsrc/ingestion/tests/e2e/specs/collab_teams_meeting_hours.test.yamlsrc/ingestion/tests/e2e/specs/collab_teams_meetings.test.yamlsrc/ingestion/tests/e2e/specs/collab_zulip_chat.test.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_m365.onedrive_activity.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_m365.sharepoint_activity.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_m365.teams_activity.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_zoom.participants.yamlsrc/ingestion/tests/e2e/specs/templates/m365_onedrive.yamlsrc/ingestion/tests/e2e/specs/templates/m365_sharepoint.yamlsrc/ingestion/tests/e2e/specs/templates/m365_teams.yamlsrc/ingestion/tests/e2e/specs/templates/zoom.yaml
✅ Files skipped from review due to trivial changes (6)
- src/ingestion/tests/e2e/specs/schemas/bronze_zoom.participants.yaml
- src/ingestion/tests/e2e/specs/collab_zulip_chat.test.yaml
- src/ingestion/tests/e2e/specs/templates/m365_sharepoint.yaml
- src/ingestion/tests/e2e/specs/templates/m365_onedrive.yaml
- src/ingestion/tests/e2e/specs/collab_emails_received.test.yaml
- src/ingestion/tests/e2e/specs/collab_emails_sent.test.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- src/ingestion/tests/e2e/specs/schemas/bronze_m365.teams_activity.yaml
- src/ingestion/tests/e2e/specs/schemas/bronze_m365.onedrive_activity.yaml
- src/ingestion/tests/e2e/specs/templates/zoom.yaml
- src/ingestion/tests/e2e/specs/collab_meetings_count.test.yaml
- src/ingestion/tests/e2e/specs/collab_teams_chats.test.yaml
- src/ingestion/tests/e2e/specs/templates/m365_teams.yaml
- src/ingestion/tests/e2e/specs/collab_meeting_free.test.yaml
- src/ingestion/tests/e2e/specs/schemas/bronze_m365.sharepoint_activity.yaml
- src/ingestion/tests/e2e/specs/collab_files_shared_external.test.yaml
- src/ingestion/tests/e2e/conftest.py
- src/ingestion/scripts/create-bronze-placeholders.sh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ingestion/tests/e2e/specs/collab_teams_meetings.test.yaml (1)
68-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this spec single-metric (
teams_meetings) to match the file contract.This case asserts
meetings_count, which breaks the “one metric_key per file” convention documented in the same PR. Consider moving this assertion tocollab_meetings_count.test.yaml(or a separate comparison spec) and keeping this file focused onteams_meetings.🤖 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/collab_teams_meetings.test.yaml` around lines 68 - 83, The new scenario in the `teams_meetings` spec is asserting `meetings_count`, which violates the single-metric contract for this file. Move the `meetings_count` assertion out of `collab_teams_meetings.test.yaml` into `collab_meetings_count.test.yaml` or another comparison spec, and keep this file limited to `teams_meetings` cases only. Use the existing test case name and `metric_key` fields to relocate the check without changing the intended coverage.
🤖 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/collab_teams_meetings.test.yaml`:
- Around line 68-83: The new scenario in the `teams_meetings` spec is asserting
`meetings_count`, which violates the single-metric contract for this file. Move
the `meetings_count` assertion out of `collab_teams_meetings.test.yaml` into
`collab_meetings_count.test.yaml` or another comparison spec, and keep this file
limited to `teams_meetings` cases only. Use the existing test case name and
`metric_key` fields to relocate the check without changing the intended
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13bdfdfc-b398-4350-87e1-aaa4395150c1
📥 Commits
Reviewing files that changed from the base of the PR and between b844ec5ddaff7f4402b33e921493205b9deb240e and 239a4481f2b60927590f286197f9b43e5b85c4bd.
📒 Files selected for processing (15)
.claude/skills/metric-e2e-test/SKILL.mdsrc/ingestion/tests/e2e/specs/collab_active_days.test.yamlsrc/ingestion/tests/e2e/specs/collab_emails_read.test.yamlsrc/ingestion/tests/e2e/specs/collab_emails_received.test.yamlsrc/ingestion/tests/e2e/specs/collab_emails_sent.test.yamlsrc/ingestion/tests/e2e/specs/collab_files_engaged.test.yamlsrc/ingestion/tests/e2e/specs/collab_files_shared_external.test.yamlsrc/ingestion/tests/e2e/specs/collab_files_shared_internal.test.yamlsrc/ingestion/tests/e2e/specs/collab_meeting_free.test.yamlsrc/ingestion/tests/e2e/specs/collab_meeting_hours.test.yamlsrc/ingestion/tests/e2e/specs/collab_meetings_count.test.yamlsrc/ingestion/tests/e2e/specs/collab_teams_chats.test.yamlsrc/ingestion/tests/e2e/specs/collab_teams_meeting_hours.test.yamlsrc/ingestion/tests/e2e/specs/collab_teams_meetings.test.yamlsrc/ingestion/tests/e2e/specs/collab_zulip_chat.test.yaml
✅ Files skipped from review due to trivial changes (4)
- src/ingestion/tests/e2e/specs/collab_emails_received.test.yaml
- src/ingestion/tests/e2e/specs/collab_zulip_chat.test.yaml
- src/ingestion/tests/e2e/specs/collab_emails_read.test.yaml
- src/ingestion/tests/e2e/specs/collab_emails_sent.test.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ingestion/tests/e2e/specs/collab_meeting_hours.test.yaml
- src/ingestion/tests/e2e/specs/collab_teams_meeting_hours.test.yaml
- src/ingestion/tests/e2e/specs/collab_meeting_free.test.yaml
mitasovr
reviewed
Jun 25, 2026
|
|
||
| # SAME fixture: meetings_count INCLUDES the Zoom meeting → alice = 4 Teams + 1 Zoom = 5 | ||
| # team {5,2,1} → median 2, range [1,5]. Proves the Zoom row landed. | ||
| - name: teams_meetings — Zoom counts in cross-source meetings_count (value 5) |
Contributor
There was a problem hiding this comment.
technically you can check both cases in one request
Contributor
Author
There was a problem hiding this comment.
Deliberate additional check
Contributor
Author
There was a problem hiding this comment.
But it might be too much for this metric
mitasovr
approved these changes
Jun 25, 2026
added 6 commits
June 25, 2026 15:38
…ics (constructorfabric#1413) Completes per-metric bronze->silver->gold->API coverage for the m365 collaboration bullet, picking up the 10 metrics deferred from the email batch (constructorfabric#1472): - Teams (bronze_m365.teams_activity): m365_teams_chats, meetings_count, teams_meetings, meeting_hours, teams_meeting_hours, meeting_free. - Files (bronze_m365.onedrive_activity + sharepoint_activity): m365_files_engaged, m365_files_shared_internal, m365_files_shared_external. - Multi-stream (email + onedrive + teams): m365_active_days. Each spec asserts ONLY its target metric_key's value/median/range on the IC bullet (...0012) over a one-team (Engineering) cohort, and seeds a verbatim- duplicate alice row so RMT/union_by_tag dedup is exercised on each new silver class (class_collab_chat/meeting/document_activity) — the value must not double. Adds the teams/onedrive/sharepoint bronze schemas + $ref templates. All collaboration specs pass (45 in the full e2e suite). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…rig infra Expand collab_meeting_hours into a data-source matrix proving meeting_hours aggregates across BOTH feeders of class_collab_meeting_activity: - alice: Teams + Zoom -> 2.0 (cross-source sum) - bob: Teams only -> 1.5 - carol: Zoom only -> 0.5 cohort median 1.5, range [0.5, 2.0]; dedup teeth on both sources. (Zulip is chat-only and does NOT feed meeting_hours.) Stand up Zoom in the e2e rig (no prior Zoom support): - create-bronze-placeholders.sh: complete the drifted bronze_zoom.participants placeholder (4 -> 13 columns the meeting models read) and add bronze_zoom.meetings (read by zoom__meeting_sessions; may stay empty — the participant->session join then falls back to meeting_uuid). The users/identity chain is not on the meeting path, so it is not needed. - schemas/bronze_zoom.participants.yaml + templates/zoom.yaml. - conftest.py: add zoom__collab_meeting_activity + zoom__meeting_sessions to the session-start truncate list. They are incremental/append, so on a warm rig they would otherwise accumulate duplicate unique_keys (failing their dbt `unique` test) and leak rows into class_collab_meeting_activity (skewing meeting_free). Full e2e suite green (45). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Replace the engineering term "cohort" with the product vocabulary across the
collaboration metric specs and the metric-e2e-test skill:
- department = the org_unit_id grouping (gold = BambooHR department string)
- team = the product's label for that group's distribution
(team_median / team_min / team_max; the …0005 "Team bullet")
Also clarify the IC-bullet median/range spans department members WITH DATA in
the window (so a narrow window shrinks it below the full team), and restructure
the active_days description into metric -> bronze->silver->gold path -> cases.
Comment/description-only: no change to seeds, cases, or assertions.
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…→gold → cases)
Apply one consistent shape to every collaboration metric spec description:
- Metric: <key> — IC Bullet Collaboration (…0012) + one-line definition
- Path (bronze → silver → gold): the per-metric lineage, e.g.
bronze_m365.email_activity → m365__collab_email_activity → class_collab_email_activity
converging on insight.collab_bullet_rows
- Team (department org_unit_id) roster + the case(s) it pins
10 single-case specs get the full shape; the 3 email date-window matrices and
meeting_hours keep their richer bodies and just gain the explicit Path section.
Lineage/definitions verified against 20260518000000_collab-bullet-rewrite.sql.
Description-only: bronze seeds and case assertions are byte-identical to HEAD,
so test behavior is unchanged.
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…or m365 collab Add cross-data-variant cases to the single-case collab specs and reshape every collab spec description to a human-readable bronze→silver→gold formula. Coverage (rig-verified): - files engaged / shared_internal / shared_external: per-product cases (OneDrive-only, SharePoint-only; external also proves a real in-team zero member) - meetings_count: seed Zoom and prove Teams+Zoom additivity (previously Teams-only, never exercising the cross-source sum the metric describes) - teams_meetings / teams_meeting_hours: seed a Zoom row and prove it is EXCLUDED from the m365-only slice, with a contrast assertion on the cross-source sibling (meetings_count / meeting_hours) confirming the row really landed - teams_chats: per-field cases (private-DM-only, team-channel-only) Descriptions: replace the dbt model/column lineage with a plain-language bronze→silver→gold formula plus a brief Team/Cases line; drop the verbose date-window "Test design" prose from the email specs. Skill: document the description convention (metric + bronze→silver→gold formula + Team/Cases) in metric-e2e-test/SKILL.md and reference it from the scaffolding steps. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…eeting_hours Per review (mitasovr): the seeded-Zoom exclusion check didn't earn its place. teams_meetings / teams_meeting_hours are defined as sumIf(... data_source = insight_m365), so their value is unchanged whether the seeded Zoom row landed or silently failed — the check only had teeth via a meetings_count / meeting_hours contrast assertion, which just re-tests metrics that already have their own specs (and broke the one-metric-per-file convention). Cross-source additivity stays covered in collab_meetings_count / collab_meeting_hours. Revert both specs to clean single-metric form (no Zoom seeding, no contrast case). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ktursunov
force-pushed
the
m365-e2e-collab-rest
branch
from
June 25, 2026 12:43
d7d4b52 to
998ca85
Compare
This was referenced Jul 9, 2026
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.
Summary
Completes the per-metric E2E coverage of the m365 Collaboration bullet. The
email metrics landed in #1472; this PR adds the remaining 10 metrics, each driving
the full
bronze → dbt silver → gold view → analytics-apipath and asserting onthe IC bullet (
…0012) over a one-team (Engineering) cohort.teams_activity)m365_teams_chats,meetings_count,teams_meetings,meeting_hours,teams_meeting_hours,meeting_freeonedrive_activity+sharepoint_activity)m365_files_engaged,m365_files_shared_internal,m365_files_shared_externalm365_active_daysEach spec:
metric_key'svalue/median/range_*(value = themember, median/range = the team cohort), per the skill convention;
union_by_tagdedup is exercised oneach new silver class (
class_collab_chat_activity,class_collab_meeting_activity,class_collab_document_activity) — the value must dedup, not double.Also adds the
teams/onedrive/sharepointbronze schemas +$reftemplates.Testing
(10 new specs + the email/zulip specs + meta framework tests.) Goldens were
confirmed twice — once before and once after adding the dedup rows (unchanged,
proving dedup).
Scope / follow-up
Covers all 13 m365 collaboration bullet metric_keys (3 email in #1472 + these 10).
Still deferred (separate concerns, can follow up): the
collab_team_cohortspec(Team bullet
…0005, multi-team) and thecollab_smokeintegration spec.Summary by CodeRabbit