perf(tests): stop paying the 0.5s serve_forever poll on every teardown - #1078
perf(tests): stop paying the 0.5s serve_forever poll on every teardown#1078seonghobae wants to merge 3 commits into
Conversation
The suite stands hundreds of throwaway http.server instances in for provider endpoints, all started as threading.Thread(target=server.serve_forever). The stop flag is only checked once per poll_interval, and shutdown() blocks until that check, so every teardown pays up to the 0.5s default. All 326 call sites under tests/ use the default; none passes the argument. Overriding the default in the root conftest takes the full suite from 652.57s to 46.68s with an identical 3395 passed / 1 skipped, measured back to back on one tree. Repeat patched runs land between 47s and 84s depending on machine load, so the honest range is roughly 8-14x. The override goes in conftest rather than the call sites because tests/test_telemetry.py pins production serve() to calling serve_forever() with no arguments, and because it is one file instead of 233. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough테스트 설정이 Changes테스트 서버 종료 동작
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change shortens test-server shutdown waits during pytest while retaining explicitly requested polling intervals. No current merge-blocking risk is identified. 🚥 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 |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Fresh fleet review found the performance shim had no executable contract of its own. I moved the PR to Draft, added tests/test_test_server_poll_interval.py, and kept the repair limited to pytest-only infrastructure. Exact head is now c045c79b0221d4f58f8231a8be2c35c00762d077, directly ahead of protected main@a080297d2546bb61e89520d637cabc202db331ec by two commits / behind 0; effective delta is only conftest.py plus the new regression. The regression proves the no-argument test path is translated to poll_interval=0.01, an explicit caller-supplied interval remains unchanged, and the class-level test override is the wrapper under test. An isolated exact-file pytest execution is 2 passed in 0.07s; this is not hosted/protected GREEN. No production server path, provider routing, workflow, dependency, or release semantics are changed. Re-promote only to materialize fresh exact-head gates; do not transfer the predecessor queued runs or the earlier wall-clock observations as acceptance evidence.
독립 로컬 검증 + 후속 측정 (autoresearch)이 PR의 주장("약 11분 → 약 1분")을 이 PR을 읽기 전에 설정한 계측으로 재현했습니다. 같은 명령( 12.6×, 3396 passed — 주장 재현됩니다. 통과 수가 2개 늘어난 건 이 PR의 계약 테스트입니다. 왜
|
|
Merged Why a push and not a code fix: all six failing checks were created 2026-09-05T10:03Z by the previous head, and none is caused by this PR's change:
Required runs bind What it will and will not change: Gates on |
|
The three All three fail on run Two things worth being precise about:
|
|
|
|
|
seonghobae
added
priority: medium
ChatGPT Codex Connector
|
All five failing checks are upstream of this pull request; none is a finding about the change, and nothing was pushed. The durations separate them before any log is read: The four fast ones are half a handshake. These jobs dispatch their work and fail deliberately in seconds to release the runner, expecting to be rerun once a verdict is published. Their annotations say so: The verdict never arrives because the central dispatch is rejected at its identity allowlist — The slow one is a gateway transport failure, not a review.
Independent of all of the aboveThis pull request cannot merge regardless, and for a different reason again: So there are three separate causes stacked here, and none of them is in the diff — which remains One thing worth recordingThe scope-gating behaviour documented for |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
conftest.py— Python module behaviortests/test_test_server_poll_interval.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Python: conftest.py"]
S1 --> I1["Python module behavior"]
I1 --> R1["Review risk: Python: conftest.py"]
R1 --> V1["pytest plus coverage"]
Evidence --> S2["Test: test_test_server_poll_interval.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_test_server_poll_interval.py"]
R2 --> V2["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
917d53e1541432787879ff32573eaa51dbe09017 - Workflow run: 34078358514
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Python: conftest.py"]
S1 --> I1["Python module behavior"]
I1 --> R1["Review risk: Python: conftest.py"]
R1 --> V1["pytest plus coverage"]
Evidence --> S2["Test: test_test_server_poll_interval.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_test_server_poll_interval.py"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
|
OpenCode did run and publish a formal review this time, with no product findings; the block comes entirely from the coverage gate. Following it to the actual failure: Step 9 never reached a coverage number. It failed while materializing the base Python environment. Why this is not the diffThis branch changes exactly two files, neither of them a dependency manifest: And the lock it complains about is almost entirely pinned already. Parsing The single unhashed entry is the project's own virtual root, which cannot carry a hash by construction — it is the local package, not a fetched dependency. So the condition the gate objects to is a property of I am reading that virtual root as the likely trigger rather than asserting it: the message allows two acceptable forms ("fully hash-pinned or exact organization VCS-pinned"), and whether a virtual root is meant to be exempted is a question for whoever owns that gate. What is not in question is that no change to Practical consequenceAny pull request in this repository reviewed through this path will reach the same block, because the input it fails on is the base lock rather than the PR. That makes Note on the run id in the review bodyThe review cites Two coverage results also exist for this same head, from different runs, and they disagree: The dispatch run is the newer one and is what the verdict reflects. Nothing pushed; the diff remains |
1 participant
Outcome
The pytest suite spends a material amount of wall time waiting for throwaway
socketserver.BaseServerinstances to noticeshutdown(). Test fixtures start hundreds ofhttp.serverinstances withserve_forever()and therefore inherit its 0.5-second polling default. This PR changes only the pytest process: rootconftest.pypreserves the production call sites while wrappingBaseServer.serve_foreverwith a 0.01-second default poll interval.The performance claim is deliberately scoped to test execution. Back-to-back local measurements on the predecessor implementation tree retained the same collected/passed count (
3395 passed, 1 skipped) while wall time moved from 652.57 s to 46.68 s; patched repeats were 46.68 s, 72.08 s, and 83.98 s. These local runs support the shutdown-poll mechanism but are not production latency, API p95, or hosted-runner evidence.Review → RED → causal GREEN
Fresh fleet review found that the test-only global override had no executable contract of its own. The PR was moved to Draft before repair.
Normal descendant
c045c79b0221d4f58f8231a8be2c35c00762d077addstests/test_test_server_poll_interval.pyand keeps the implementation unchanged. The regression proves:serve_forever()call is delegated to the original method withpoll_interval=0.01;0.25is preserved instead of overwritten;An isolated exact-file run on this descendant is
2 passed in 0.07s. This is focused local evidence only, not protected/hosted GREEN.Exact authority
main@a080297d2546bb61e89520d637cabc202db331ecc045c79b0221d4f58f8231a8be2c35c00762d077conftest.py,tests/test_test_server_poll_interval.pyThe branch was returned to Ready only to materialize fresh exact-head review/check evidence. Current exact-head Security and Quality
33959628627, Security Scan33959628647, SAST Semgrep33959628620, and CodeQL PR33959628663are queued. Earlier measurements and predecessor workflow runs do not transfer as merge acceptance.Promotion boundary
Before normal protected merge, require the unchanged exact head to satisfy all live required contexts, current review/thread requirements, and independent approval. If hosted execution exposes test-order leakage, third-party server incompatibility, or materially different test counts, repair this same descendant rather than weakening the gate. No source-neutral retrigger, self-approval, bypass, force-push, destructive rebase, or predecessor-evidence transfer.
Summary by CodeRabbit
버그 수정
테스트