[OPIK-8011] [QA] test: e2e coverage for online evaluation sampling rate - #7929
Open
AndreiCautisanu wants to merge 6 commits into
Open
[OPIK-8011] [QA] test: e2e coverage for online evaluation sampling rate#7929AndreiCautisanu wants to merge 6 commits into
AndreiCautisanu wants to merge 6 commits into
Conversation
Covers @cap:online-evaluation.sampling-rate, previously uncovered in taxonomy.yaml. Two deterministic Python-Equals rules over one batch of 30 traces: a 50% rule and a 100% control. The control is what turns "the partial rule skipped these traces" into a real negative rather than a timing guess. The backend draws per trace (OnlineScoringSampler.shouldSampleTrace: secureRandom.nextFloat() >= samplingRate) with no seed hook, so the assertion is a 15-85% band: ~1-in-29,000 false failures at n=30, versus ~1-in-300 for a 25-75% band that catches no additional real regression. SliderInputControl commits on blur, not on change — typing a rate and submitting without blurring persists 1.0 while still displaying the typed number. That is invisible to a sampling assertion (a rule left at 100% scores everything, exactly like "sampling ignored"), so the POM forces the blur and the spec gates on the persisted fraction before seeding any traces. Adds two data-testids: the sampling control had no stable handle, as the number input's only accessible name was "%", leaked from the suffix label. The SliderInputControl testid derives from its existing id prop, so every call site gains a handle with no call-site churn and no version-awareness. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
Frontend
tests
Contributor
⏱️ pre-commit per-hook timing
⏭️ 41 skipped (no matching files changed)
|
Collaborator
|
No test needed here. This PR is the sampling-rate e2e itself plus the hooks it needs: two data-testid attributes (SliderInputControl's number input, the Filtering & Sampling accordion trigger) and the id="topK" -> id="frequencyPenalty" fix in OpenRouterModelConfigs, which un-duplicates the DOM id so the Frequency penalty label and suffix stop pointing at the Top K slider. That fix is real but label-focus-only — the value binding was already correct — and the OpenRouter config panel needs a configured provider key to reach. Nothing here for a second test to cover; your spec is the coverage for online-evaluation.sampling-rate (taxonomy flipped to covered: true in this same diff). Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 20 Aug 10:07 UTC — nothing the verdict depends on changed. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
…rd cleanup waitForTraceScoresSettled decides "settled" from a stable score-set fingerprint, so with minScores: 0 a not-yet-scored trace looked stable from its first poll: the helper returned after one quiet period holding an empty score set while the control's score was still in flight, and that trace was then tallied as "the control never scored it". timeoutMs could not rescue it — the helper returns early rather than timing out. Now polls each trace for the 100% rule first (a guaranteed arrival), then settles with minScores: 1. Cleanup moves into try/finally: as a trailing test.step it was skipped whenever an earlier assertion failed, and the project fixture's teardown does not reach automation rules, so red runs leaked both rules. Verified by injecting a mid-test failure — zero rules left behind. Also fixes a pre-existing duplicate id in OpenRouterModelConfigs, where Frequency penalty passed id="topK". That already broke its label association, and with testids derived from the id it would have produced two data-testid="topK-input" nodes and a Playwright strict-mode violation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…up masking failures The band comments were wrong in three ways: bounds 5..25 are 16.7%-83.3% of 30 rather than the stated 15%-85%; 5 is 15% rounded UP, not down; and the quoted ~1-in-29,000 flake rate belonged to 4..25, so the real budget for the configured bounds was ~1-in-16,800. Maintainers reading those comments would have had a materially better flake budget in mind than the test delivered. Bounds now derive from the fractions (4..26, ~1-in-118,000), so the numbers and the prose cannot drift apart again. Cleanup in `finally` awaited two throwing calls: a failed list skipped every deletion, and deleteAutomationRule rethrows anything that is not a 404, so a cleanup error replaced the assertion or scoring error that actually explained the run. This was a regression from moving cleanup into `finally` — as a trailing step it only ran on success and could not mask anything. Both calls are now caught and warned per operation, and the loop continues past one undeletable rule. Verified by injecting a test failure and a cleanup failure together: the cleanup error logs as a warning and the original failure is what gets reported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rule deletion is workspace-global, not project-scoped: deleteEvaluatorsByIds, deleteByRuleIds and deleteBaseRules all constrain on workspace_id plus the rule id, so the project_id query parameter does not narrow the delete. Verified against a running backend — a rule created against two projects and then deleted "via project A" disappeared from project B as well. listAutomationRulesForProject returns rules shared with other projects, so the finally-block sweep could have destroyed a rule another project still used. Rules this spec creates are single-project, so the filter changes nothing today; it makes the blast radius a local property of the cleanup rather than resting on "every rule in this project is ours". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The project fixture keeps its project when OPIK_LEAVE_FAILURES is set and the test failed, but the finally-block rule cleanup deleted regardless — handing a debugger a preserved project stripped of the rules that explain the failure. Cleanup now skips when the body threw and the flag is set. Failure is tracked with a catch-set flag rather than shouldLeaveArtifacts(testInfo): inside `finally` Playwright has not finalised the status yet, so testInfo.status still reads 'passed' on a failing test (verified with a probe spec) and the helper would always return false — the guard would silently never fire. Verified all three paths: failure + flag leaves the rules with a warning; failure without the flag still cleans up; the success path is unchanged. Not paginating the cleanup listing. listAutomationRulesForProject requests size=500 and this spec creates two rules, so there is no second page to fetch; adding pagination would be dead code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The retention guard added in 3c785bf used an early `return` inside `finally`. A `return` there discards the in-flight exception and resolves the function, so a failed run with OPIK_LEAVE_FAILURES set was reported as PASSED — a green tick on a genuinely failing test, which is the worst failure mode test code can have. Reproduced in a probe spec: a failing `expect` plus a `return` in `finally` reports "1 passed". The previous round's verification missed it because the injected failure and the retention flag were never exercised in the same run, so the return branch was reachable but untested. Cleanup is now guarded by a boolean instead, leaving no `return` in the `finally`. Re-verified with both conditions together: the warning is emitted AND the run reports "1 failed"; the success path still passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
Details
Adds a
@t2-cujE2E test for@cap:online-evaluation.sampling-rate, previously uncovered intaxonomy.yaml. A wrong sampling rate is a silent failure mode — traces quietly go unscored, or get over-scored and burn LLM cost, for weeks before anyone notices — and the rate control was the one part of the rule dialog with no scripted coverage.The test creates two deterministic Python-Equals rules over one batch of 30 traces: a 50% rule and a 100% rule.
OnlineScoringSampler.shouldSampleTrace→secureRandom.nextFloat() >= samplingRate) with no seed hook, so the count is a genuine binomial variable. n=30 with a 15–85% band gives a ~1-in-29,000 false-failure rate; a tighter 25–75% band would be ~1-in-300 while catching no additional real regression (every plausible break lands at 30, 0, or ~2).data-testids were needed — the sampling control had no stable handle, as the number input's only accessible name was%, leaked from the suffix<label>.One thing reviewers should know:
SliderInputControlcommits the typed value to the form inonBlur, notonChange. Verified against the live dialog — typing25and submitting without blurring persistssampling_rate: 1.0while the UI still displays25. That failure is invisible to a naive sampling assertion, because a rule left at 100% scores everything, which is exactly what "sampling ignored" looks like. So the POM forces the blur, and the spec gates on the persisted fraction before seeding any traces, turning a silent no-op into a loud early failure. This is arguably also a real UX bug (a user who types a rate and clicks Create gets 100%), but that's product behaviour and out of scope here.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
data-testids, and the taxonomy update. Live-UI selector discovery and the blur-commit finding were done against a local OSS deployment via the Playwright MCP. The binomial band was computed rather than guessed.mainbase) — all green. The observed split was instrumented once with a temporary probe (partial=14/30 full=30/30, probe removed before commit) to confirm the test measures real sampling rather than passing vacuously. The blur-commit trap was verified empirically both ways against the running app: blur → persisted0.5; no blur → persisted1.0.Testing
Commands run (from
tests_end_to_end/e2e/, local OSS athttp://localhost:5173, workspacedefault, Chromium):Results:
online-evaluation/directory (shared POM regression)tag_lint.py39 specs checked, 1 exempt, 0 problem(s)npx tsc --noEmit(e2e suite)pre-commiton changed filesScenarios validated:
14/30vs30/30.0.5/1.0) before seeding, so a rate that didn't take fails immediately instead of degrading into a silent no-op.1.0; this is why the POM blurs explicitly.online-evaluation.page.ts.Environment note: the FE change means the frontend container must be rebuilt for the testids to exist. Locally:
docker compose --profile opik build frontend, then recreate withOPIK_FRONTEND_PULL_POLICY=never. Verified the attribute in the running container and in the live DOM before running the test.Not run: the LLM-judge smoke test in the same directory skips in this environment — it's gated on an LLM provider key (
pom/model-availability.ts), a pre-existing condition unrelated to this change. No video recording attached: this is a test-only change with no user-facing UI behaviour to demonstrate.Documentation
No documentation change needed — this adds test coverage and two test hooks, with no user-facing behaviour change. Coverage bookkeeping lives in
tests_end_to_end/coverage/taxonomy.yaml, updated in this PR: the spec is added to the area'sspecs:list andsampling-rateis flipped tocovered: true, tier: t2-cuj.