test(stand): cover metric drilldown for every metric - #2255
Conversation
Drilldown capability is declared per source, not per metric, so an evidence relation drifting from the observations derived from it moves every metric of that family at once. Sweep the catalogue and require each metric's evidence to add back up to its own served value. Refs #1603 Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis change adds catalogue-wide metric drilldown reconciliation, evidence-reference registry validation, shared browser page objects, and Playwright coverage for card, cell, timeseries, and export evidence flows. Testing documentation now describes generalized browser journeys and non-required CI checks. ChangesMetric Evidence Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
tests/stand/api/analytics/test_drilldown.py (5)
161-209: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBound the walk when
page_budgetisNone.The loop terminates only on
next_cursor is Noneor on a repeated cursor.page_budget=Noneremoves the page cap, andcursorsgrows once per page.test_git_commit_drilldown_pages_and_reconcilesandtest_git_commit_drilldown_exports_all_rowscall_walkwithlimit=1and no budget. If the seed gains commits, those two tests issue one request per row. A seed change then produces a slow run rather than a readable failure.Consider defaulting
page_budgetto_PAGE_BUDGETand letting callers raise it, so every walk carries a 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 `@tests/stand/api/analytics/test_drilldown.py` around lines 161 - 209, The _walk helper currently permits unbounded pagination when page_budget is None, causing limit=1 callers to request indefinitely as data grows. Default page_budget to _PAGE_BUDGET and preserve the existing override behavior so callers can explicitly raise the bound while every walk remains capped.
557-564: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe git selection is now declared in two incompatible ways.
_seeded_requestencodes the filter and display dimension at Lines 156-157. Both tests re-declare the same pair inline for_walk, andtest_git_commit_drilldown_exports_all_rowsthen exports through_seeded_request. The walk and the export therefore describe one selection in two places. If either drifts, Line 604 compares an export of one selection against a walk of another, and the count mismatch reads as a serializer defect rather than a test defect.Give
_walkthe same filter and dimension arguments from one shared constant, or let_walkaccept the request_seeded_requestbuilds.Also applies to: 588-596
🤖 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 `@tests/stand/api/analytics/test_drilldown.py` around lines 557 - 564, Centralize the git selection used by _seeded_request and both _walk calls so the filter for source=github and the repository display dimension are defined only once. Update the tests around _walk and test_git_commit_drilldown_exports_all_rows to reuse that shared selection, preserving identical parameters for walking and exporting.
540-550: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe export tests compare counts, not cell values.
The CSV check compares the header labels and the row count. The XLSX check compares only the row count. A serializer that emitted the correct number of rows with wrong, shifted, or empty cells passes both. The test name states that the export carries every row, and the docstring states that comparing against the page is what catches a broken serializer.
Compare at least the first data row's cells against the matching row in
walk.rows, projected throughwalk.column_keys.🤖 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 `@tests/stand/api/analytics/test_drilldown.py` around lines 540 - 550, The export assertions in the drilldown test only validate headers and row counts, not serialized cell values. Update the CSV and XLSX checks around `_export` and `_xlsx_rows` to compare at least the first data row with the corresponding `walk.rows` entry, projecting values through `walk.column_keys`; preserve the existing format, status, content type, and row-count assertions.
369-375: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe bounded branch drops
expectation.transform.The unbounded path at Line 377 applies
transformbefore comparing. The bounded path compares the rawevidencevalue. NoCOLLAPSE_BOUNDED_RATIOentry inMATRIXsets a transform today, so this is currently unreachable. A future bounded ratio with a clamp would be compared against an untransformed value and could pass or fail for the wrong reason.Either apply the transform here as well, or assert
expectation.transform is Nonein this branch so the omission is explicit.🤖 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 `@tests/stand/api/analytics/test_drilldown.py` around lines 369 - 375, The bounded branch in the drilldown assertion omits expectation.transform before comparing evidence. Update the bounded path to apply expectation.transform consistently with the unbounded path, or explicitly assert that expectation.transform is None before the comparison; preserve the existing bounded ratio validation behavior.
436-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
EVIDENCE_UNAVAILABLEon a structured error field, not the raw body.
response.parse(ProblemDocument)already exposes the parsed error. Use the error-class-specific field for the precondition code instead of matching the whole response text, so assertions fail when the named precondition changes.🤖 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 `@tests/stand/api/analytics/test_drilldown.py` around lines 436 - 439, Update the assertion in the drilldown test to check that “EVIDENCE_UNAVAILABLE” appears in the appropriate structured error field from the parsed ProblemDocument returned by response.parse, rather than in response.text. Preserve the existing status assertion and diagnostic context for failures.tests/stand/api/analytics/drilldown_matrix.py (1)
100-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
BY_KEYfromtests/stand/api/analytics/drilldown_matrix.py.
BY_KEYis exported but no repository code reads it. Keep it only if a consumer is added; otherwise remove the unused export.🤖 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 `@tests/stand/api/analytics/drilldown_matrix.py` around lines 100 - 101, Remove the unused BY_KEY export from the drilldown_matrix definitions, including any associated declaration or export wiring. Leave metric_key and source unchanged, and do not retain BY_KEY unless repository usage is introduced.
🤖 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 `@tests/stand/api/analytics/drilldown_matrix.py`:
- Around line 113-188: Add an import-time duplicate-key validation adjacent to
BY_KEY, before or while constructing the lookup, so repeated metric_key values
in MATRIX raise an explicit failure instead of being silently overwritten.
Preserve the existing BY_KEY mapping for unique keys and make the guard identify
the duplicated key.
In `@tests/stand/api/analytics/test_drilldown.py`:
- Around line 387-422: Both Tier match statements silently accept future
unhandled variants. In tests/stand/api/analytics/test_drilldown.py lines
387-422, add a final unhandled arm to _reconcile that raises AssertionError
including metric_key and the tier; likewise add the same failing arm to the
match in _assert_shape at lines 296-321, ensuring any new Tier variant fails
explicitly.
- Around line 503-509: Update the incomplete-page branch in the drilldown
comparison to fail the test via pytest.fail instead of only issuing
warnings.warn, ensuring exceeding _PAGE_BUDGET cannot leave the comparison
passing while unreconciled. Preserve the existing diagnostic message and early
return behavior as appropriate.
- Around line 74-77: Update _request_for() to use None—not truthiness—as the
omission sentinel for entity_id, so an explicitly passed _EMPTY_ENTITY_ID is
preserved instead of replaced with dev_lead. Adjust the rejection test to pass
the empty identifier directly, while allowing callers to pass None when they
want the default entity ID.
In `@tests/stand/README.md`:
- Line 23: Update the introductory description in the README to remove or revise
the stale “four browser journeys” count so it no longer contradicts the
generalized journey descriptions and newly added browser evidence modules. Keep
the rest of the introduction unchanged.
In `@tests/stand/ui/test_team_grid_cell_evidence.py`:
- Around line 64-70: Update the cell evidence test around the evidence drilldown
assertions to verify that the opened evidence belongs to member, using the
request payload or returned evidence owner data exposed by the test flow. Keep
the existing table visibility, date header, row-count, and metric-selector
assertions unchanged.
In `@tests/stand/ui/test_timeseries_block_evidence.py`:
- Around line 61-70: Update the metric-selection test around selector.click()
and evidence.table() to verify that selecting other triggers a new drilldown
request or displays evidence content specific to other. Do not rely solely on
table visibility, since the existing table may belong to opened_with; preserve
the selector value assertion while adding a metric-specific replacement check.
- Around line 121-128: Strengthen the assertions around
open_total_row_evidence(TASKS_CLOSED) to verify the total-row request selects no
individual time bucket, or that the returned evidence covers the entire
displayed block period. Keep the existing visibility, column, and non-empty
table assertions while adding this full-period validation.
---
Nitpick comments:
In `@tests/stand/api/analytics/drilldown_matrix.py`:
- Around line 100-101: Remove the unused BY_KEY export from the drilldown_matrix
definitions, including any associated declaration or export wiring. Leave
metric_key and source unchanged, and do not retain BY_KEY unless repository
usage is introduced.
In `@tests/stand/api/analytics/test_drilldown.py`:
- Around line 161-209: The _walk helper currently permits unbounded pagination
when page_budget is None, causing limit=1 callers to request indefinitely as
data grows. Default page_budget to _PAGE_BUDGET and preserve the existing
override behavior so callers can explicitly raise the bound while every walk
remains capped.
- Around line 557-564: Centralize the git selection used by _seeded_request and
both _walk calls so the filter for source=github and the repository display
dimension are defined only once. Update the tests around _walk and
test_git_commit_drilldown_exports_all_rows to reuse that shared selection,
preserving identical parameters for walking and exporting.
- Around line 540-550: The export assertions in the drilldown test only validate
headers and row counts, not serialized cell values. Update the CSV and XLSX
checks around `_export` and `_xlsx_rows` to compare at least the first data row
with the corresponding `walk.rows` entry, projecting values through
`walk.column_keys`; preserve the existing format, status, content type, and
row-count assertions.
- Around line 369-375: The bounded branch in the drilldown assertion omits
expectation.transform before comparing evidence. Update the bounded path to
apply expectation.transform consistently with the unbounded path, or explicitly
assert that expectation.transform is None before the comparison; preserve the
existing bounded ratio validation behavior.
- Around line 436-439: Update the assertion in the drilldown test to check that
“EVIDENCE_UNAVAILABLE” appears in the appropriate structured error field from
the parsed ProblemDocument returned by response.parse, rather than in
response.text. Preserve the existing status assertion and diagnostic context for
failures.
🪄 Autofix
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 Plus
Run ID: cee4b90a-a142-4031-851a-d6365f495d9c
📒 Files selected for processing (11)
docs/TESTING.mdsrc/backend/services/analytics/src/domain/metric_definitions/builtin.rstests/stand/README.mdtests/stand/api/analytics/drilldown_matrix.pytests/stand/api/analytics/test_drilldown.pytests/stand/ui/pages/group_dialog.pytests/stand/ui/pages/person_view.pytests/stand/ui/pages/team_view.pytests/stand/ui/test_collaboration_card_evidence.pytests/stand/ui/test_team_grid_cell_evidence.pytests/stand/ui/test_timeseries_block_evidence.py
The catalogue's capability query also requires the definition's schema to be checked, while the drilldown endpoint requires only healthy evidence, so the catalogue withholds the capability for metrics whose evidence it serves. Assert the direction that holds — an advertised metric must answer — and pin the other as a strict xfail. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
A dialog with rows in it proves the request succeeded, not that it carried the person whose cell was clicked, the metric that was chosen, or the block's period rather than one bucket. Read the selection off the wire. Also make both `Tier` matches exhaustive, so a tier added without a rule fails to type-check instead of asserting nothing, and drop an unused lookup table. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
2 participants
Summary
drilldown_matrix.py, and fail when a metric is added without oneEVIDENCE_UNAVAILABLEevidence_refparses as an evidence relationNotes
Capability is declared per source, not per metric, so an evidence relation drifting from the observations derived from it moves every metric of that family at once — a single-metric test cannot see it. Reconciliation is tiered because an evidence row means a different thing per granularity and computation: exact row count, summed values, median identity, scaled ratio with the definition's affine and clamp applied, distinct dates, and an inequality where day flags collapse across a person's accounts.
No value is typed into a test — both sides come from the service, so the seed can change underneath it.
Not covered, deliberately: clicking a chart mark. Recharts marks carry no accessible handle, and its click path is the one the table cell already exercises.
Validation
ruff format --check,ruff check,mypycargo test -p analytics --bin analytics metric_definitions::builtin— 13 passedRefs #1603
Summary by CodeRabbit
New Features
Bug Fixes
Documentation