Repository navigation
Conversation
…ead (#1784) The `pytest (windows-latest, 3.12)` job's margin arithmetic (`tests/test_pytest_leg_timeout_1658.py`, `tests/test_posthang_diagnostics_1660.py`) compared the job's `timeout-minutes` cap only against pytest's own self-reported runtime, ignoring checkout/setup-python/the Windows Defender exclusion step/`pip install`, all of which run as separate steps before pytest ever starts and none of which pytest's own clock sees. Measured directly from the GitHub Actions API (job start to the "Run tests" step's own start) for the six CI jobs already cited in these files, that pre-pytest overhead ran 26-43s -- already comparable to, and at its worst exceeding, the entire ~26.9s margin the old arithmetic treated as free. Adds `PRE_PYTEST_OVERHEAD_SECONDS` (43, the measured worst case) and subtracts it in all three affected margin checks (the job-cap test, the worker-dump-delay test, and the controller-dump-delay test). Raises the `pytest` job's `timeout-minutes` 30 -> 32 to restore a real, if still thin, margin once the overhead is subtracted -- sized to the now-measured, largely-fixed overhead, not to chase the suite's own separate, unexplained runtime growth the surrounding comments already argue against re-raising for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two reviewers (Explore, oss:auditor) spawned against the #1784 commit each found one stale-prose issue: - .github/workflows/tests.yml still carried the #1660-era sentence "This cap is left at 30 rather than raised a third time... raising it again is not expected to hold either" directly above the new #1784 comment that raises it to 32 anyway -- flatly contradicting it. Reconciled: the sentence now says that reasoning held when the cap was last left at 30, and points at the #1784 comment for why it was raised anyway. - tests/test_pytest_leg_timeout_1658.py's docstring for test_the_pytest_job_cap_clears_the_observed_worst_case_with_margin still quoted "margin is ~0.45 minutes" against "the corrected constant above" -- true when written (before PRE_PYTEST_OVERHEAD_SECONDS existed), but ambiguous now that a second corrected term exists and stale as a description of what the test currently asserts (~1.73 minutes). Clarified which constant/margin that sentence describes and pointed at the current figure. Both reviewers found the arithmetic, units and coverage of the original fix correct; retargeted tests plus the Python-floor guard still pass (21 passed). Co-Authored-By: Claude Sonnet 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
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.
Margin arithmetic in tests/test_pytest_leg_timeout_1658.py and tests/test_posthang_diagnostics_1660.py computed margin = timeout-minutes cap minus pytest's own self-reported suite runtime only, ignoring the pre-pytest overhead (checkout, setup-python, the Windows Defender exclusion step, pip install) that also counts against the job's wall-clock timeout-minutes cap.
Measured the real per-job gap directly from the GitHub Actions API (
gh api repos/Digital-Process-Tools/claude-oss/actions/jobs/<id>) against the six real CI job IDs already cited in these test files' own docstrings, rather than guessing or launching a fresh instrumented run: 43s, 29s, 33s, 28s, 26s, 29s. AddedPRE_PYTEST_OVERHEAD_SECONDS = 43(the max of the six) and subtracted it in all three affected margin assertions.With the old 30-minute job cap, the corrected margin goes negative (30*60 - 1773.10 - 43 = -16.1s) -- once overhead is honestly accounted for, the previously-"passing" test was already unsafe. Raised
.github/workflows/tests.yml's pytest jobtimeout-minutesfrom 30 to 32 to restore a real (if still thin, ~103.9s / ~1.73min) margin, targeted specifically at the now-measured pre-pytest overhead -- not at the suite's own separate, unexplained runtime growth (873s -> 1176.55s -> 1773.10s), which an adjacent pre-existing comment already argues against chasing with further cap raises.Self-review (Explore + oss:auditor spawned concurrently against the committed diff) found two findings, both fixed in a second commit: (1) an older #1660-era workflow comment left unreconciled against the new #1784 paragraph raising the cap; (2) a stale docstring figure ("margin is ~0.45 minutes") not updated to reflect the corrected constant and current live margin (~1.73 minutes).
trap.d/1660.pytest-leg-margin-arithmetic-ignores-pre-run-tests-step-overhead.md, the fragment this issue was routed from, appears resolved by this fix; left for a future /oss:curate pass to remove rather than touched here.
Closes #1784
🤖 Generated with Claude Code
[AI-generated]