Make coverage opt-in instead of running on every pytest - #622
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ec5d02a9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fc67ba964
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Nothing consumed the coverage numbers: no threshold, no `--cov-fail-under`, no codecov, and the CI HTML artifact and job summary went unread. It cost ~9% of suite wall time plus ~1.3 s of `pytest-cov` session startup on every invocation, which dominates a single-test run. `coverage` and `pytest-cov` stay in the dev group, so `pytest --cov=positronic --cov=pimm` still works on demand — `docs/CONTRIBUTING.md` documents that path. Also drop the `time.sleep(0.2)` in `test_background_process`: `start_in_subprocess` appends to `background_processes` before it returns, so the following assertion never needed the wait.
`dummy_process` yields nothing when it observes a set `_stop_event` on its first condition, so `_bg_wrapper` never runs a loop body and `test_background_process` passed without exercising the control loop it names. `HeartbeatProcess` emits from inside the body, and the test blocks on that message against a deadline before requesting shutdown.
`start_in_subprocess` takes a `ControlLoop`, which is what the class supplies through `run`; the class itself manages no process. Its only user is `TestWorld.test_background_process`, so it belongs directly above that class rather than in the helper block 200 lines up.
`[tool.coverage.run] source` names the packages, so spelling a subset of them again as `--cov=` flags gives the list a second owner that silently under-reports whatever it misses.
1de07bb to
0cba30c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cba30cd44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The class carried a single field the caller already holds as a local. A module-level generator plus `functools.partial` pickles to a spawned child just as well, and mirrors `dummy_process` above it.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Coverage ran on every
pytestinvocation viaaddopts, and nothing consumed the result — no threshold, no--cov-fail-under, no codecov, and the CI HTML artifact and job summary went unread. It cost ~9% of suite wall time plus ~1.3 s ofpytest-covsession startup per invocation, which dominates a single-test run.--cov*entries fromaddoptsinpyproject.tomlcorejob inunit-test.yaml--no-covthat every other test command carried to opt out —CLAUDE.md,README.md, the two lerobot jobs,libero-e2e.yaml, and thetest_e2e.pydocstringcoverageandpytest-covstay in thedevgroup, sopytest --cov=positronic --cov=pimm --cov-report=term-missingstill works on demand;docs/CONTRIBUTING.mdalready documents that pathAlso drops the
time.sleep(0.2)intest_background_process:start_in_subprocessappends tobackground_processesbefore it returns, so the assertion that follows never needed the wait.Closes #529. The macOS flake that issue also reports was already fixed in #501, which replaced the 0.5 s
joinwith a 30 s one.Test plan
uv run --locked pytest— 1117 passed, 9 skipped, no coverage output.