test: close the metric drilldown coverage gaps - #2463
Conversation
|
Warning Review limit reached
Next review available in: 21 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 (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR centralizes metric drilldown concurrency and export-limit handling, expands CSV/XLSX and refusal coverage, adds hostile seeded commit messages, and introduces an opt-in Docker-backed rebuild test for stale cursors. ChangesMetric drilldown hardening
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: 2
🧹 Nitpick comments (6)
tests/stand/conftest.py (1)
123-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
pytest_addoptiondocstring now covers only one of two options.The docstring describes
--stand-manifestand the reasoning for having no matching ini key. The function now also registers--rebuild-lane. A reader of the docstring does not learn that the second option exists.Add one sentence naming
--rebuild-laneand pointing at the collection hook that consumes it. This is optional.🤖 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/conftest.py` around lines 123 - 134, Update the pytest_addoption docstring to add a sentence naming --rebuild-lane and referencing the collection hook that consumes it, while preserving the existing explanation of --stand-manifest and its lack of an ini key.src/ingestion/tools/seed/insight_seed/generators/git.py (1)
54-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the multi-line rationale comments.
The block at Lines 54-62 runs nine lines and the block at Lines 104-107 runs four. The repository guideline limits source comments to one line and directs lasting context to a design document or the issue.
The rationale here is real, so move the byte-class reasoning and the dev-lead-only reasoning into the seed design notes and keep one line at each site. This is optional and does not change behavior.
As per coding guidelines: "Add comments only when code cannot express the reason, such as intentional redundancy, cross-function invariants, or reasoned workarounds; keep them to one line."
Also applies to: 104-107
🤖 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/tools/seed/insight_seed/generators/git.py` around lines 54 - 62, Trim the multi-line rationale comments in the seed generator, including the sites around the synthetic commit messages and the later related block, to one line each. Move the byte-class and dev-lead-only rationale into the seed design notes or referenced issue, preserving all generated values and behavior.Source: Coding guidelines
tests/stand/api/analytics/test_drilldown_rebuild.py (2)
170-184: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
docker pslists only running containers.The docstring states that the helper force-removes any
seed-samplecontainer of the project.docker pswithout--allreturns running containers only, so a container that already exited is left behind.The hazard the caller describes is a container that keeps mutating the stand after the timeout, and such a container is running, so the current behavior covers the stated case. Add
--allto match the docstring and to clear exited leftovers.♻️ Proposed change
[ "docker", "ps", + "--all", "--quiet", "--filter", f"label=com.docker.compose.project={project}",🤖 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_rebuild.py` around lines 170 - 184, Update the docker command in the helper containing the listed subprocess call to include the --all option, so container discovery covers both running and exited seed-sample containers while preserving the existing project and service label filters.
156-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the stdout tail in the failure message.
dbt writes its model-level diagnostics to stdout. The failure message reports only
completed.stderr[-2000:], so a failed rebuild in this 900-second opt-in lane hides the output that names the failing model and the SQL error.
completed.stdoutis already captured. Add it to the message.♻️ Proposed change
if completed.returncode != 0: pytest.fail( f"rebuild of {EVIDENCE_MODEL} failed (exit {completed.returncode}); the model " "either rebuilt or kept its previous build, never half of each — dbt swaps " - f"atomically.\nstderr tail: {completed.stderr[-2000:]}" + f"atomically.\nstdout tail: {completed.stdout[-2000:]}" + f"\nstderr tail: {completed.stderr[-2000:]}" )🤖 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_rebuild.py` around lines 156 - 161, Update the pytest failure message in the rebuild failure branch to include the tail of completed.stdout alongside the existing stderr tail. Keep the current exit code and atomic rebuild context, using the already-captured completed.stdout value to expose dbt model diagnostics.src/backend/services/analytics/src/api/metric_drilldown.rs (1)
155-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one shared acquire helper for both semaphores.
acquire_query_permitrepeats the exact shape ofacquire_export_permitat Lines 139-153. Only the semaphore, the timeout, the log capacity field, and the busy constructor differ. A single helper taking those four values removes the duplication and keeps the two refusal paths from drifting.This is optional; the current form is correct.
♻️ Sketch of the shared helper
async fn acquire_permit( semaphore: &'static Semaphore, timeout: Duration, capacity: usize, kind: &'static str, busy: fn() -> CanonicalError, ) -> Result<tokio::sync::SemaphorePermit<'static>, CanonicalError> { tokio::time::timeout(timeout, semaphore.acquire()) .await .map_err(|_| { tracing::warn!( capacity, available = semaphore.available_permits(), kind, "metric drilldown capacity exhausted" ); busy() })? .map_err(|_| busy()) }As per coding guidelines: "Extract repetition into named helpers, and centralize error construction in one helper per failure kind".
🤖 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/backend/services/analytics/src/api/metric_drilldown.rs` around lines 155 - 167, Extract the duplicated semaphore-acquisition logic from acquire_query_permit and acquire_export_permit into one shared acquire_permit helper accepting the semaphore, timeout, capacity, refusal label, and busy-error constructor. Update both callers to use it while preserving their existing timeout values, logging context, and error behavior.Source: Coding guidelines
tests/stand/api/analytics/test_drilldown.py (1)
97-102: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTie the boundary test to
MAX_FILTER_VALUES
MAX_FILTER_VALUESis currently100, so150sent values and50distinct values exercise the intended boundary. Add an explicit guard or derive the test counts from the cap so a future cap change cannot silently remove this coverage.🤖 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 97 - 102, Tie _FILTER_DISTINCT_VALUES and _FILTER_VALUE_REPEATS to the drilldown MAX_FILTER_VALUES boundary by importing or defining the cap reference and adding an explicit assertion that the repeated sent count exceeds it while the distinct count remains below it. Ensure future changes to MAX_FILTER_VALUES cannot silently invalidate this boundary 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.
Inline comments:
In `@src/backend/services/analytics/src/api/metric_drilldown.rs`:
- Around line 355-365: Update the assertions around enforce_export_row_limit to
compare status_code() directly with
Some(axum::http::StatusCode::TOO_MANY_REQUESTS); remove the as_u16() conversion
in all three affected assertions so the expected and actual types are both
Option<StatusCode>.
In `@tests/stand/api/analytics/test_drilldown.py`:
- Around line 1267-1276: Update the _refusal assertion for the oversized source
filter values in the drilldown test to require the complete message “between 1
and 100 values are required” instead of the partial “values are required” text.
---
Nitpick comments:
In `@src/backend/services/analytics/src/api/metric_drilldown.rs`:
- Around line 155-167: Extract the duplicated semaphore-acquisition logic from
acquire_query_permit and acquire_export_permit into one shared acquire_permit
helper accepting the semaphore, timeout, capacity, refusal label, and busy-error
constructor. Update both callers to use it while preserving their existing
timeout values, logging context, and error behavior.
In `@src/ingestion/tools/seed/insight_seed/generators/git.py`:
- Around line 54-62: Trim the multi-line rationale comments in the seed
generator, including the sites around the synthetic commit messages and the
later related block, to one line each. Move the byte-class and dev-lead-only
rationale into the seed design notes or referenced issue, preserving all
generated values and behavior.
In `@tests/stand/api/analytics/test_drilldown_rebuild.py`:
- Around line 170-184: Update the docker command in the helper containing the
listed subprocess call to include the --all option, so container discovery
covers both running and exited seed-sample containers while preserving the
existing project and service label filters.
- Around line 156-161: Update the pytest failure message in the rebuild failure
branch to include the tail of completed.stdout alongside the existing stderr
tail. Keep the current exit code and atomic rebuild context, using the
already-captured completed.stdout value to expose dbt model diagnostics.
In `@tests/stand/api/analytics/test_drilldown.py`:
- Around line 97-102: Tie _FILTER_DISTINCT_VALUES and _FILTER_VALUE_REPEATS to
the drilldown MAX_FILTER_VALUES boundary by importing or defining the cap
reference and adding an explicit assertion that the repeated sent count exceeds
it while the distinct count remains below it. Ensure future changes to
MAX_FILTER_VALUES cannot silently invalidate this boundary coverage.
In `@tests/stand/conftest.py`:
- Around line 123-134: Update the pytest_addoption docstring to add a sentence
naming --rebuild-lane and referencing the collection hook that consumes it,
while preserving the existing explanation of --stand-manifest and its lack of an
ini key.
🪄 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: d6561036-00f0-414d-a106-41a6071298f7
📒 Files selected for processing (8)
src/backend/services/analytics/Cargo.tomlsrc/backend/services/analytics/src/api/metric_drilldown.rssrc/ingestion/tools/seed/PROFILE.mdsrc/ingestion/tools/seed/insight_seed/generators/git.pytests/pyproject.tomltests/stand/api/analytics/test_drilldown.pytests/stand/api/analytics/test_drilldown_rebuild.pytests/stand/conftest.py
d6f2d53 to
1635b0a
Compare
…age parity Implement #1603 scenarios 6 and 13 in tests/stand/api/analytics/test_drilldown.py. Scenario 6 (reliability): every unservable drilldown request is refused with its own distinguishable reason and no partial page — malformed, wrong-version, foreign-selection and expired-snapshot pagination cursors (each tampered from a genuinely issued one), undeclared/duplicated/over-cap filters, undeclared display dimensions, and a well-formed but unknown metric key (404, the deliberate opposite of metric-results' 400). Selection-shape refusals are asserted on the export route too. Scenario 13 (security): both export formats refuse an out-of-scope person identically to the paged read — same status, problem class, reason and context, a bare problem document body, and no content-disposition file offer. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
A handful of the dev lead's seeded commits now carry clearly synthetic messages that begin with a spreadsheet formula prefix (=, +, -, @) or embed a tab / newline, so the metric-drilldown export has hostile-shaped evidence to prove its escaping against (#1603 scenario 11). Only the first few non-merge commits of the one person are touched: row counts, metric values and every other person's evidence are unchanged. PROFILE.md is regenerated for the seed_revision bump. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
CSV and XLSX, and compare cell for cell against the paged rows, so both sides of every comparison come from the service. Asserts the backend's actual CSV neutralization contract (a leading apostrophe when a cell's first byte is =, +, -, @, tab, CR, LF or space; embedded tabs and newlines kept inside their cell by RFC 4180 quoting) and that XLSX stores every value byte-identical as a shared or inline string, with no formula cells anywhere in the sheet. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
A continuation token pins the evidence table's ClickHouse UUID as its snapshot_id, and a dbt rebuild swaps the relation atomically under a new UUID — so a cursor obtained before a rebuild must be refused with the documented EVIDENCE_SNAPSHOT_EXPIRED failed-precondition rather than resuming a row order the new build no longer defines. Scenario 6 covers refusing a tampered token; this is the other half, under a real rebuild. The new test takes page one of git.commits at limit 1, rebuilds only git_metric_evidence through the stand's own seed image (the same apply-ch-migrations path 'test-stand seed gold' runs, narrowed to one dbt model), asserts the pre-rebuild cursor answers 400 with the documented precondition in a problem document, and then re-walks from scratch to show the rebuilt evidence still reconciles row-for-row with the metric value — which also proves the stand is content-identical for every test that follows. The lane is opt-in and serialized: a new rebuild_lane marker is skipped unless --rebuild-lane is passed, because the trigger shells out to docker (local compose stand only) and mutates shared stand state, so the run it joins must not share the stand with anything else. Within a run the suite is a single pytest process, so nothing else holds a cursor while the UUID rotates. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The over-cap filter list is now repeats of a few distinct values, so the refusal pins the before-dedup ordering its docstring claims. The rebuild test proves stand equivalence in a finally, so the proof survives a refusal failure, and a timed-out rebuild force-removes its container instead of mutating past the declared ceiling. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The export row limit and both concurrency caps refused only on a live stand nobody can push past the limits of. Extract the row-limit guard and the query-permit acquire into named helpers and pin all three refusals — over-cap rows, exhausted export permits, exhausted query permits — as paused-time unit tests answering 429. Refs #1603 scenario 14. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…lagged comment Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The link-parity tests read class_git_commits rows by index, so inserting message mid-tuple moved the day a person-day check reads. Appending it keeps every long-standing position; the inserted data is unchanged. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
09917e7 to
ac50b6a
Compare
2 participants
Summary
Closes the open test scenarios of #1603 (Testing section scenarios 6, 7, 11, 13, 14):
distinguishable reason and no partial response: malformed / wrong-version / foreign-selection /
stale-snapshot continuation tokens (built by tampering a genuinely issued cursor), undeclared,
duplicated and over-cap filter dimensions (repeats of a few distinct values, pinning the
before-dedup cap check), undeclared display dimensions, and an unknown metric key (400
UNAVAILABLEfrom the catalogue loader, the same classification/v1/metric-resultsuses)export route as from the paged read, with zero evidence bytes and no attachment header
=+-@, an embedded tab and newline) on six of the dev lead's commits; the testcompares CSV and XLSX exports cell for cell against the paged read and asserts the export's
actual neutralization contract (CSV apostrophe-prefix on leading formula bytes; XLSX strings
never written as formulas). The seeder previously wrote no
messagecolumn at all, so everyother commit now carries an explicit empty message — row counts, RNG draws and metric values
are unchanged
--rebuild-laneflag: a scoped dbt rebuild of oneevidence relation mid-walk must expire the held cursor (
EVIDENCE_SNAPSHOT_EXPIRED), neversplice two builds; the stand-equivalence proof runs in a
finally, and a timed-out rebuildforce-removes its container
as paused-time unit tests; the row-limit guard and query-permit acquire are extracted into
named helpers to make them unit-testable (behavior unchanged)
Validation
ruff format --check,ruff check,mypyclean on the touched Pythoncargo fmt --check,cargo clippy,cargo test -p analytics --bin analytics api::metric_drilldown::tests— 4 passedtest_drilldown.py93 passed / 1 xfailed(Supporting data is unreachable for metrics whose definition schema probe stays unchecked #2268) / 3 xpassed (Metric drilldown intermittently returns 500 while the stand suite runs #2361 flake guards),
test_drilldown_rebuild.py --rebuild-lane1 passedtest(stand): address drilldown audit findingsRefs #1603
Summary by CodeRabbit
Bug Fixes
Tests