Skip to content

test(performance): allow RVF search setup to complete - #718

Merged
proffesor-for-testing merged 1 commit into
proffesor-for-testing:mainfrom
rudycelekli:fix/rvf-search-benchmark-timeout
Sep 27, 2026
Merged

proffesor-for-testing merged 1 commit into
proffesor-for-testing:mainfrom
rudycelekli:fix/rvf-search-benchmark-timeout

Conversation

@rudycelekli

Copy link
Copy Markdown
Contributor

Summary

The native RVF Performance Gates job can fail while preparing the search-latency benchmark: it ingests 1,000 patterns before measuring search, and the setup exceeded Vitest's default 10-second per-test timeout on #712's CI run. That run measured search p95 at 0.40 ms, below the existing 50 ms search bound, but reported a timeout failure. The separate ingest benchmark passed at 7.75 seconds and retains its strict 10-second throughput bound.

Give only the combined ingest-and-search test a 30-second execution budget so it can complete setup and evaluate the existing search-latency assertion. Neither the ingest threshold nor the search threshold changes.

Verification

  • The linked GitHub job reproduces the timeout after roughly 15 seconds in the combined test; its measured search p95 was 0.40 ms.
  • The real native benchmark passes locally: 4/4 tests, with 1,000-pattern ingest at 7.96 seconds and search p95 at 0.27 ms (Vitest 4.1.4). npm run typecheck and git diff --check pass.

Failure modes

  • The combined benchmark's setup exceeds the framework timeout before its measured search assertion is evaluated. The linked CI job exercises this; the real benchmark passes with the explicit budget.
  • A genuine ingest or search regression must still fail. The separate ingest test keeps ingestMs < 10000, and the combined test keeps p95 < 50 in tests/performance/rvf-pattern-store.test.ts.

Required check (issue #401)

  • Every failure mode mentioned in this PR description has either (a) a test that exercises it, or (b) a linked tracking issue. "Unlikely" is not an acceptable substitute. If you wrote "I don't think this can happen but...", that sentence is a failure mode and needs a test or an issue link.

Optional context

@proffesor-for-testing proffesor-for-testing left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rudycelekli — nicely scoped, and the linked #712 log backs it up exactly: search p95 was 0.40 ms and the only failure was setup running past Vitest's 10 s default. Raising the budget on just the combined test while keeping the strict < 10000 ms standalone ingest bound and p95 < 50 intact is the right trade. Verified locally (4/4 green) and CI is green.

Optional follow-up: logging the combined test's ingest duration would make future slow-runner diagnosis easier. Merging — thanks!

@proffesor-for-testing
proffesor-for-testing merged commit a49b603 into proffesor-for-testing:main Sep 27, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants