Adopt pytest-xdist in generate-coverage (#278)#279
Merged
Conversation
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd Changespytest-xdist integration in generate-coverage
Test output normalisation in setup-rust
Sequence Diagram(s)sequenceDiagram
participant Action as GitHub Action
participant Runner as run_python.py
participant Slipcover as slipcover
participant Xdist as pytest-xdist
Action->>Runner: set INPUT_PYTEST_WORKERS and invoke Python step
Runner->>Slipcover: build slipcover argv including pytest args and add `-n <workers>` when set
Slipcover->>Xdist: spawn workers with `-n` and merge per-worker coverage
Possibly related PRs
Suggested labels
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 3 warnings, 1 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideEnable configurable parallel Python coverage runs via pytest-xdist in the generate-coverage action, adding a validated pytest-workers input, wiring it through the coverage runner, updating docs/CHANGELOG, and hardening stderr filtering for setup-rust workspace validation tests. Sequence diagram for pytest-workers handling in generate-coveragesequenceDiagram
actor User
participant Workflow as GitHubActionsWorkflow
participant Action as generate_coverage_action
participant Script as run_python_main
participant Resolver as _resolve_pytest_workers
participant Normaliser as _normalise_pytest_workers
participant Runner as _run_coverage
participant Cmd as coverage_cmd_for_fmt
participant Pytest as pytest
User->>Workflow: dispatch workflow
Workflow->>Action: with pytest-workers
Action->>Action: set INPUT_PYTEST_WORKERS env
Action->>Script: uv run run_python.py
Script->>Resolver: _resolve_pytest_workers(pytest_workers)
alt CLI option omitted
Resolver->>Resolver: os.getenv(INPUT_PYTEST_WORKERS)
Resolver->>Resolver: fallback DEFAULT_PYTEST_WORKERS
else CLI option provided
Resolver->>Resolver: use pytest_workers
end
Resolver->>Normaliser: _normalise_pytest_workers(raw)
Normaliser-->>Resolver: workers
Resolver-->>Script: workers
Script->>Runner: _run_coverage(fmt, out, workers)
Runner->>Cmd: coverage_cmd_for_fmt(fmt, out, workers)
Cmd->>Cmd: _coverage_args(fmt, out, workers)
alt workers not empty
Cmd->>Cmd: append "-n" and workers to PYTEST_ARGS
end
Cmd-->>Runner: BoundCommand
Runner->>Pytest: run_cmd(Pytest, method="run_fg")
Pytest-->>Runner: exit status
Runner-->>Script: percent
Script->>Action: write GITHUB_OUTPUT
Action-->>Workflow: file, percent outputs
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. .github/actions/generate-coverage/scripts/run_python.py Comment on lines +523 to +531 pytest_workers: typ.Annotated[
str | None,
typer.Option(
help=(
'Worker count for pytest-xdist (-n). Use an integer, "auto", '
'"logical", or "" to disable parallelism. Defaults to "auto".'
),
),
] = None,❌ New issue: Large Method |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Run the Python coverage suite under pytest-xdist by default. A new `pytest-workers` input (default `auto`) is appended to slipcover's pytest invocation as `-n <workers>`; setting it to `""` restores serial behaviour. pytest-xdist is installed alongside the existing coverage tooling so the flag is always available. README documents the slipcover 1.0.18 omit regression so consumers with co-located in-package tests can opt out. Refs: #278 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Recent uv versions emit coloured warnings, so the literal `warning:` prefix match in `_clean_stderr` no longer filters the `VIRTUAL_ENV` notice on `validate_workspaces.py` runs. Strip ANSI escape sequences from each line before applying the prefix filters so the well-known noise is removed regardless of terminal styling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`main` exceeded CodeScene's 70-line Large Method threshold. Lifting each `typ.Annotated[…]` block into a private module-level alias and trimming the docstring to a single-line summary shrinks the function while keeping its behaviour and Typer CLI surface identical — Typer resolves `Annotated` metadata from module-level aliases the same way as inline declarations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit flagged three small inconsistencies in the new `pytest-workers` documentation: - `action.yml`'s input description omitted `logical`, even though the code and the README list it as a valid value. - The README's first mention of "CPU" was not expanded; per the repo style guidelines uncommon acronyms must be defined on first use. - The README's `--omit` caveat addressed the reader directly with "you", which the docs style guidelines prohibit. Skipped: rejecting `-n 0` (pytest-xdist treats `0` as serial execution, which is consistent with the issue spec's "an integer" wording), and the suggestion to inline the Typer option aliases (those were extracted on purpose to keep `main` under CodeScene's Large Method threshold). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When a consumer project already pins an older `slipcover` in its own `pyproject.toml`, `uv sync --inexact` installs that older version before `_install_coverage_tooling` runs `uv pip install slipcover ...`. Without `--upgrade` or a version constraint, uv leaves the existing package as-is, so the new default `pytest -n auto` runs slipcover's xdist plugin against a version that does not have it — producing failed or merged-incorrectly coverage reports. Constraining the install spec to `slipcover>=1.0.18` triggers uv's version-resolution path, which upgrades an older release in place. The integration test now asserts that the install argv carries a version floor on slipcover so this guarantee cannot regress. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- README: drop the stray comma before "because" in the slipcover xdist omit-regression warning so the restrictive clause reads cleanly. - generate-coverage integration test: set `INPUT_PYTEST_WORKERS=2` in the fake env so the subprocess actually exercises the env-reading path rather than silently falling back to the action.yml default. - setup-rust: add a unit test for `_clean_stderr` that feeds a string containing ANSI escape sequences and the `VIRTUAL_ENV` noise prefix and asserts both the escapes are stripped and the noise line is filtered, locking in the recent ANSI-handling fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After rebasing onto main, the shared `test_support.ansi` module from PR #281 supersedes the local ANSI helpers introduced earlier in this branch. Pull `ANSI_ESCAPE_RE` from that module to reuse the canonical colon-aware pattern (which already covers true-colour sequences like `\x1b[38:2:255:0:0m`), restore the standard import ordering disturbed by the merge, and add the missing blank line before the new `_clean_stderr` regression test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lodyai
Bot
force-pushed
the
issue-278-adopt-pytest-xdist-in-generate-coverage
branch
from
f5f5c0c to
a49298d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
Owner
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
|
@coderabbitai review |
1 similar comment
Owner
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Owner
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
CodeRabbit flagged two genuine gaps: - `_parse_pytest_workers` validated digit strings via `str.isdigit()`, which returns True for characters like the superscript "²" that `int()` rejects. Hypothesis-driven property testing made the divergence obvious. Switch the guard to `str.isdecimal()`, which matches `int()`'s acceptance set and keeps the canonical "Invalid pytest-workers value" error message for unusual inputs. Four new property tests pin the contract: named values normalise to lowercase under arbitrary case-mixing and surrounding whitespace; positive integer strings round-trip with whitespace padding; whitespace-only strings collapse to ""; and zero/negative integers raise ValueError. - The developer documentation was out of date: the install command in `docs/developers-guide.md` still listed `slipcover pytest coverage` (no version pin, no `pytest-xdist`) and the Public API row for `main` omitted the new `pytest_workers` parameter. The design doc had no entry recording the pytest-xdist adoption decision. Update both: refresh the install command and the API row, and add a 2026-06-04 design-decision bullet covering the new input, the slipcover floor, the `"0"` rejection rationale, and the upstream `--omit` regression. Skipped from the review: - Module-level docstring expansion: existing docstrings are concise and accurate; workflow context belongs in README/AGENTS.md. - Snapshot tests for the new "Pytest workers: ..." log lines: the two existing `test_main_*` tests already pin the exact wording with explicit string assertions, which is more diff-friendly than a snapshot for single one-line messages. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/generate-coverage/tests/test_scripts.py:
- Around line 2425-2445: The test signature for
test_parse_pytest_workers_normalises_named_values has too many parameters;
collapse leading and trailing into a single parameter (e.g. padding or pad_pair)
produced by st.tuples(_WHITESPACE_ST, _WHITESPACE_ST) so the signature becomes
(run_python_module, name, upper_mask, pad_pair) and then unpack pad_pair into
leading and trailing inside the test before constructing raw; keep the rest of
the logic and assertion unchanged and update any references to leading/trailing
accordingly in the body of the test and preserve the decorator/given settings to
maintain coverage.
In `@docs/developers-guide.md`:
- Line 61: Replace the en-GB non-oxendict spelling "parallelises" with the
oxendict "-ize" form "parallelizes" in the sentence mentioning `workers` and
`pytest-xdist` so it reads that pytest-xdist "parallelizes" when `workers` is
non-empty; edit the text near the `workers` / `pytest-xdist` reference in
developers-guide.md accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 323e687e-b828-4f71-864d-e616e1c77b4d
📒 Files selected for processing (4)
.github/actions/generate-coverage/scripts/run_python.py.github/actions/generate-coverage/tests/test_scripts.pydocs/developers-guide.mddocs/generate-coverage-design.md
- ``test_parse_pytest_workers_normalises_named_values`` previously took five parameters (plus the module fixture), exceeding the four-argument threshold. Collapse the ``leading`` and ``trailing`` whitespace parameters into a single ``pad_pair`` tuple generated by ``st.tuples(_WHITESPACE_ST, _WHITESPACE_ST)``, then unpack it inside the test body. The decorator settings, generation domain, and assertion are unchanged. - ``docs/developers-guide.md`` used the en-GB ``-ise`` form ``parallelises`` for pytest-xdist's behaviour. Switch to the Oxford spelling ``parallelizes`` to match the repository's en-GB-oxendict convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Owner
|
@coderabbitai pause |
Contributor
✅ Action performedReviews paused. |
`_resolve_pytest_workers` used to delegate validation to `_normalise_pytest_workers`, which carries Typer side-effects (`typer.echo` + `typer.Exit(2)`). That left a CLI boundary in a helper the domain code calls during resolution, violating the rule that the boundary should live exclusively in `main()`. Switch the resolver to call the already-pure `_parse_pytest_workers` so it now raises `ValueError` for invalid input. `main()` catches that `ValueError` at the single CLI boundary and converts it to the existing stderr message plus `typer.Exit(2)`, preserving observable behaviour at the action's edge. None of the existing `_resolve_pytest_workers` tests asserted Typer-style exits, so they continue to pass unchanged. Two new regression tests pin the refactored contract: `test_resolve_pytest_workers_raises_value_error_on_invalid_env` verifies the resolver itself raises `ValueError` (no Typer touched); `test_main_translates_invalid_workers_into_typer_exit` verifies that `main` catches it, prints the canonical message to stderr, and exits with code 2 — and that `run_cmd` is never invoked when validation fails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== docs/developers-guide.md: parallelise/paralleliz ==="
grep -n "parallelis\|paralleliz" docs/developers-guide.md
echo ""
echo "=== test_parse_pytest_workers_normalises_named_values signature ==="
grep -n -A 12 "def test_parse_pytest_workers_normalises_named_values" .github/actions/generate-coverage/tests/test_scripts.py
echo ""
echo "=== _resolve_pytest_workers current implementation ==="
grep -n -A 20 "def _resolve_pytest_workers" .github/actions/generate-coverage/scripts/run_python.py
echo ""
echo "=== _normalise_pytest_workers current implementation ==="
grep -n -A 20 "def _normalise_pytest_workers" .github/actions/generate-coverage/scripts/run_python.py
echo ""
echo "=== _parse_pytest_workers current implementation ==="
grep -n -A 20 "def _parse_pytest_workers" .github/actions/generate-coverage/scripts/run_python.py
echo ""
echo "=== main() workers resolution ==="
grep -n -A 55 "^def main" .github/actions/generate-coverage/scripts/run_python.py | head -65
echo ""
echo "=== Open GitHub issues related to slipcover xdist concurrency ==="
gh issue list --repo leynos/shared-actions --state open --search "slipcover xdist concurrency" 2>/dev/null || echo "no matching issues"
gh issue list --repo leynos/shared-actions --state open --limit 20 2>/dev/nullLength of output: 8638 🏁 Script executed: #!/bin/bash
gh issue create \
--repo leynos/shared-actions \
--title "generate-coverage: document and test slipcover 1.0.18 xdist per-worker coverage merging" \
--body "## Background
PR `#279` introduces pytest-xdist parallelism to the \`generate-coverage\` composite action. Per-worker coverage data is merged by slipcover's built-in xdist plugin, which requires \`slipcover>=1.0.18\`. The concurrency model of that merging is not currently documented or tested within this repository.
## Work required
1. **Documentation** — add a section to \`docs/generate-coverage-design.md\` (or a dedicated ADR) explaining how slipcover 1.0.18's xdist plugin merges per-worker \`.coverage\` files, including the lifecycle (worker startup, mid-run accumulation, teardown merge), the behaviour under partial worker failure, and the interaction with \`--omit\`.
2. **Integration tests** — add at least one integration test that actually executes pytest-xdist (with \`-n 2\` or \`-n auto\`) against a minimal Python project fixture and asserts that the merged coverage output is correct (i.e. coverage from all workers is present in the final report).
3. **Failure-mode coverage** — clarify and, where feasible, test the behaviour when a worker process crashes mid-run: does slipcover emit a partial result, a zero-coverage result, or an error exit?
## References
- PR `#279` (adopts pytest-xdist): https://github.com/leynos/shared-actions/pull/279
- Review comment raising this concern: https://github.com/leynos/shared-actions/pull/279
Raised by `@leynos`."Length of output: 1650 🏁 Script executed: #!/bin/bash
gh issue list --repo leynos/shared-actions --state open --limit 5Length of output: 641
Issue |
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.
Summary
pytest-xdistsupport to thegenerate-coveragecomposite action for Python coverage runs.pytest -n autoby default via the newpytest-workersinput.pytest-xdistin the dedicated.venv-coveragetooling environment and requireslipcover>=1.0.18so xdist worker coverage can be merged correctly.pytest-workers: "".Functional changes
Introduces a new action input:
pytest-workersAccepted values are:
autological1,2, or4""to run pytest seriallyThreads the input through the action environment as
INPUT_PYTEST_WORKERSand into the Python coverage runner.Adds validation for
pytest-workersbefore invoking slipcover/pytest:0, negative numbers, floats, and arbitrary strings are rejected2and a clear configuration errorAdds runtime logging to show whether the coverage run is using pytest-xdist or serial pytest execution.
Updates slipcover invocation so non-empty worker values append:
after the pytest arguments.
Documentation updates
pytest-workersinput, default behavior, and how to disable xdist.--omitis not propagated to worker processes.Tests
pytest-xdistwith coverage toolingslipcover>=1.0.18version floor-nbased on the workers valuemain()into the slipcover pytest commandINPUT_PYTEST_WORKERSpytest -v.Drive-by test robustness fix
setup-rustworkspace validator test helper to strip ANSI escape sequences before filtering knownuvVIRTUAL_ENVwarning noise.uvwarnings are removed while real stderr content is preserved.