test(e2e): collab Messaging metrics — messages_sent, channel_posts (#1527) - #1578
Closed
mozhaev-dev wants to merge 1 commit into
Closed
test(e2e): collab Messaging metrics — messages_sent, channel_posts (#1527)#1578mozhaev-dev wants to merge 1 commit into
mozhaev-dev wants to merge 1 commit into
Conversation
…1527) PR C of the #1527 split — end-to-end YAML specs over the gold view (PR A) and peer-counter query + catalog seed (PR B). One metric_key per file (current rig convention, à la collab_emails_{sent,received,read}); both query metric_id …0053. - collab_messaging_messages_sent.test.yaml: department of 5 with per-person totals {10,20,30,40,50} sourced across Teams + Zulip to prove the cross-vendor UNION — carol = Teams 15 + Zulip 15 = 30. Cases: cross-vendor sum · single- vendor still non-NULL (bob, Zulip-only) · empty window → NULL, not 0. - collab_messaging_channel_posts.test.yaml: department of 5 with per-person Teams post+reply counts summing to {2,4,6,8,10}, proving the posts+replies fold (carol = post 4 + reply 2 = 6). Cases: fold · empty window → NULL, not 0. Reuses existing people / m365_teams / zulip templates + schemas; both specs pass static validation (ref resolution + schema padding + additionalProperties:false). Scope notes: - Slack is a declared source but has no e2e fixtures yet (needs schemas/bronze_slack.users_details.yaml + a template from the real bronze). The multi-vendor honest-NULL contract is proven with Teams + Zulip; adding Slack is a follow-up. - Depends on PR A (gold view) + PR B (query_ref + seed) being present for the full ClickHouse run; not runnable green until those land. Honest-NULL "No data"/tooltip rendering is #1517. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
📝 WalkthroughWalkthroughAdds two new end-to-end YAML test specifications for Collaboration Messaging metrics: ChangesCollaboration Messaging metrics e2e tests
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 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 |
mozhaev-dev
marked this pull request as ready for review
July 3, 2026 07:34
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ingestion/tests/e2e/metrics/collab_messaging_messages_sent.test.yaml`:
- Around line 81-97: The bob Zulip-only test currently checks only
collab_person_counter_daily.messages_sent.value, leaving the rest of the stat
payload unverified. Update the expectation in this test case to assert the full
row shape for the messages_sent metric, including median, range_min, range_max,
p25, and p75 alongside value. Use the collab_person_counter_daily.messages_sent
metric_key in the existing collaboration query to locate the assertion block.
🪄 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: 9671b70f-ddb3-4ade-8870-7d42e9b4d19c
📒 Files selected for processing (2)
src/ingestion/tests/e2e/metrics/collab_messaging_channel_posts.test.yamlsrc/ingestion/tests/e2e/metrics/collab_messaging_messages_sent.test.yaml
Comment on lines
+81
to
+97
| # bob has ONLY Zulip — the metric is still non-NULL from the remaining source | ||
| - name: "messages_sent — single-vendor still non-NULL (bob, Zulip-only = 20)" | ||
| request: | ||
| url: /v1/metrics/queries | ||
| method: POST | ||
| body: | ||
| queries: | ||
| - id: collaboration | ||
| metric_id: 00000000-0000-0000-0001-000000000053 | ||
| $filter: "person_id eq 'bob@example.com' and metric_date ge '2026-12-01' and metric_date le '2026-12-31'" | ||
| expect: | ||
| - assert: "status == 200" | ||
| - in: collaboration | ||
| assert: "result.status == 'ok'" | ||
| - in: collaboration | ||
| find: { metric_key: collab_person_counter_daily.messages_sent } | ||
| equal: { value: 20 } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target file context ==\n'
cat -n src/ingestion/tests/e2e/metrics/collab_messaging_messages_sent.test.yaml | sed -n '1,220p'
printf '\n== sibling fixture names ==\n'
git ls-files 'src/ingestion/tests/e2e/metrics/*.test.yaml' | sed 's#^`#-` #'
printf '\n== search for no-unasserted-stat / equal semantics ==\n'
rg -n "no-unasserted-stat|unasserted-stat|range_min|range_max|p25|p75|median|metric_key: collab_person_counter_daily.messages_sent" src . -g '!**/node_modules/**'Repository: constructorfabric/insight
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== collab_zulip_chat fixture ==\n'
cat -n src/ingestion/tests/e2e/metrics/collab_zulip_chat.test.yaml | sed -n '1,220p'
printf '\n== collab_emails_sent fixture ==\n'
cat -n src/ingestion/tests/e2e/metrics/collab_emails_sent.test.yaml | sed -n '1,220p'
printf '\n== collab_meeting_hours fixture ==\n'
cat -n src/ingestion/tests/e2e/metrics/collab_meeting_hours.test.yaml | sed -n '1,260p'
printf '\n== exact compare / equal semantics ==\n'
rg -n "no-unasserted-stat|unasserted|find: \{ metric_key|equal: \{ value:" src docs -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'Repository: constructorfabric/insight
Length of output: 30262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== expect_engine.py relevant sections ==\n'
cat -n src/ingestion/tests/e2e/lib/expect_engine.py | sed -n '1,280p'
printf '\n== collab messaging migration ==\n'
cat -n src/backend/services/analytics-api/src/migration/m20260624_000001_collab_zulip_chat.rs | sed -n '1,260p'Repository: constructorfabric/insight
Length of output: 26588
Complete the bob case’s stat assertions. The collab_person_counter_daily.messages_sent row also carries median, range_min, range_max, p25, and p75, so asserting only value leaves the row under-checked.
Suggested fix
- in: collaboration
find: { metric_key: collab_person_counter_daily.messages_sent }
- equal: { value: 20 }
+ equal: { value: 20, median: 30, range_min: 10, range_max: 50, p25: 20, p75: 40 }📝 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
| # bob has ONLY Zulip — the metric is still non-NULL from the remaining source | |
| - name: "messages_sent — single-vendor still non-NULL (bob, Zulip-only = 20)" | |
| request: | |
| url: /v1/metrics/queries | |
| method: POST | |
| body: | |
| queries: | |
| - id: collaboration | |
| metric_id: 00000000-0000-0000-0001-000000000053 | |
| $filter: "person_id eq 'bob@example.com' and metric_date ge '2026-12-01' and metric_date le '2026-12-31'" | |
| expect: | |
| - assert: "status == 200" | |
| - in: collaboration | |
| assert: "result.status == 'ok'" | |
| - in: collaboration | |
| find: { metric_key: collab_person_counter_daily.messages_sent } | |
| equal: { value: 20 } | |
| # bob has ONLY Zulip — the metric is still non-NULL from the remaining source | |
| - name: "messages_sent — single-vendor still non-NULL (bob, Zulip-only = 20)" | |
| request: | |
| url: /v1/metrics/queries | |
| method: POST | |
| body: | |
| queries: | |
| - id: collaboration | |
| metric_id: 00000000-0000-0000-0001-000000000053 | |
| $filter: "person_id eq 'bob@example.com' and metric_date ge '2026-12-01' and metric_date le '2026-12-31'" | |
| expect: | |
| - assert: "status == 200" | |
| - in: collaboration | |
| assert: "result.status == 'ok'" | |
| - in: collaboration | |
| find: { metric_key: collab_person_counter_daily.messages_sent } | |
| equal: { value: 20, median: 30, range_min: 10, range_max: 50, p25: 20, p75: 40 } |
🤖 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_messaging_messages_sent.test.yaml`
around lines 81 - 97, The bob Zulip-only test currently checks only
collab_person_counter_daily.messages_sent.value, leaving the rest of the stat
payload unverified. Update the expectation in this test case to assert the full
row shape for the messages_sent metric, including median, range_min, range_max,
p25, and p75 alongside value. Use the collab_person_counter_daily.messages_sent
metric_key in the existing collaboration query to locate the assertion block.
Source: Learnings
Contributor
Author
mozhaev-dev
added a commit
that referenced
this pull request
Jul 6, 2026
The single-vendor non-NULL case asserted only `value`; the row also carries the department bands, which are deterministic (same org_unit distribution as the carol case). Assert median/p25/p75/range too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
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
1 participant
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.
PR C of the #1527 split — end-to-end YAML specs over the gold view (PR A, #1567) and peer-counter query + catalog seed (PR B, #1568).
Split plan: A gold view (#1567) → B backend (#1568) → C (this) → D FE (insight-front).
Changes
One
metric_keyper file (current rig convention, à lacollab_emails_{sent,received,read}); both query metric_id…0053.collab_messaging_messages_sent.test.yaml— department of 5 with per-person totals{10,20,30,40,50}sourced across Teams + Zulip to prove the cross-vendor UNION: carol = Teams 15 + Zulip 15 = 30. Cases:collab_messaging_channel_posts.test.yaml— department of 5 with Teams post+reply counts summing to{2,4,6,8,10}, proving the posts+replies fold (carol = post 4 + reply 2 = 6). Cases: fold · empty window → NULL, not 0.Reuses existing
people/m365_teams/zuliptemplates + schemas.Validation
additionalProperties:false(ranlib/fixture_loader.load).find/equal/CEL mirror the proven teams/zulip specs.main). No local cluster here.Scope notes
schemas/bronze_slack.users_details.yaml+ a template generated from the real bronze). The multi-vendor honest-NULL contract ("drop a source → others non-NULL; drop all → NULL, not 0") is proven here with Teams + Zulip; adding Slack is a follow-up.Refs #1527 · #1516
🤖 Generated with Claude Code
Summary by CodeRabbit