fix: remove --forked from test commands to fix coverage measurement - #277
Draft
tofarr wants to merge 6 commits into
Draft
fix: remove --forked from test commands to fix coverage measurement#277tofarr wants to merge 6 commits into
tofarr wants to merge 6 commits into
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
The app-settings screen wraps everything in a <form action={formAction}>.
A nested <form> inside it is invalid HTML, so the inner form's submit
never fired and the POST to /api/v1/secrets did not happen. Replace
the nested form with a <div> and trigger the save via the button's
onClick, reading the textarea value via a ref.
Co-authored-by: openhands <openhands@all-hands.dev>
The app-settings component now uses useNavigate from react-router, which requires a Router context. Wrap the test render with MemoryRouter following the same pattern as agent-settings.test.tsx. Co-authored-by: openhands <openhands@all-hands.dev>
pytest-forked runs each test in a forked child process that exits via os._exit(), which bypasses coverage.py's atexit handlers. This silently discards all coverage data from forked children, producing wildly inaccurate numbers: Enterprise: 26% (with --forked) -> 76% (without) OSS: 27% (with --forked) -> 74% (without) The flag was added in PR #15286 as part of the initial CI workflow creation (copy-pasted from the OSS command), not to fix a specific test-isolation bug. pytest-forked is also a deprecated package. Attempts to fix coverage collection while keeping --forked (concurrency config, COVERAGE_PROCESS_START) all failed because os._exit() fundamentally cannot be worked around with config. Full suite passes cleanly without --forked: 3069 passed, 0 failed, verified across multiple runs with -n auto xdist parallelism. Co-authored-by: openhands <openhands@all-hands.dev>
Coverage report
This PR does not seem to contain any modification to coverable code. |
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
1 participant
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.
HUMAN:
AGENT:
Why
pytest-forkedruns each test in a forked child process that exits viaos._exit()(line 79 ofpy/_process/forkedfunc.py), which bypassescoverage.py'satexithandlers. Every forked child's coverage data is silently discarded, producing wildly inaccurate coverage numbers:--forked) → 76% (without)--forked) → 74% (without)This was reported as "about 35%" coverage. The actual coverage is 76% — the gap is entirely a measurement artifact.
The
--forkedflag was added in PR #15286 as part of the initial CI workflow creation (copy-pasted from the OSS command), not to fix a specific test-isolation bug.pytest-forkedis also a deprecated package (removed from pytest core, maintained as a standalone plugin).Summary
--forkedfrom both test jobs in.github/workflows/py-tests.yml(OSS + enterprise)--forkedfrom the documented enterprise test command inAGENTS.mdHow to Test
Run the enterprise test suite without
--forkedand confirm all tests pass with accurate coverage:Expected: 3,069 passed, 6 skipped, 76% coverage.
Verified across 3 runs with
-n auto(xdist parallelism) and 1 single-process run — all consistent, zero failures.Type
Notes
pytest-forkeddependency (inpyproject.toml) is left in place as an unused dependency. Removing it requires lockfile regeneration and is out of scope for this fix.--forkedwith coverage concurrency fixes) was tested and ruled out:.coveragercwithconcurrency = multiprocessingandCOVERAGE_PROCESS_STARTboth failed becauseos._exit()fundamentally cannot be worked around with config.--forkedin the future, that indicates a real test-isolation bug (shared DB/session state leaking between tests) that--forkedwas masking. No such failures exist currently.This PR was created by an AI agent (OpenHands) on behalf of Tim O'Farrell.
Enterprise server image for this PR: