Parametrize acceptance action with pytest parallel worker count#730
Parametrize acceptance action with pytest parallel worker count#730mwojtyczka wants to merge 7 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds configurability for pytest-xdist parallelism in the acceptance GitHub Action and reduces staticcheck noise from a tracked deprecation migration.
Changes:
- Introduce a new
naction input (default10) and propagate it viaPYTEST_Ninto the acceptance test runtime. - Update
pytest_run.pyto read the worker count fromPYTEST_Ninstead of hard-coding-n 10. - Add an
acceptance/staticcheck.confto suppressSA1019warnings duringmake fmtuntil a separate migration completes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| acceptance/action.yml | Adds the new n input with default 10. |
| acceptance/main.go | Forwards the n input into the action’s environment as PYTEST_N. |
| acceptance/ecosystem/pytest_run.py | Uses PYTEST_N to set pytest-xdist -n. |
| acceptance/staticcheck.conf | Globally disables SA1019 for the acceptance module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| n: | ||
| description: 'Number of parallel pytest workers (pytest-xdist -n).' | ||
| required: false | ||
| default: '10' |
There was a problem hiding this comment.
The action is implemented via shim.js, which currently pins the downloaded binary version (e.g., acceptance/shim.js:1 is v0.4.4). Adding the new n input here will have no effect for anyone running the action from this branch/commit until the shim version is bumped to a release that includes the updated Go binary; otherwise the action advertises n but the downloaded binary ignores it. Consider updating shim.js in the same change set (and cutting the corresponding acceptance/vX.Y.Z release), or defer exposing the input until the release commit so the action definition and runtime stay in sync.
| n: | |
| description: 'Number of parallel pytest workers (pytest-xdist -n).' | |
| required: false | |
| default: '10' |
There was a problem hiding this comment.
The release PR after merge should handle the shim bump, same as the repo has done historically. However, new release cut is necessary
| if __name__ == '__main__': | ||
| sys.exit(pytest.main([ | ||
| '-n', '10', | ||
| '-n', os.environ.get('PYTEST_N', '10'), |
There was a problem hiding this comment.
os.environ.get('PYTEST_N', '10') will return an empty string if PYTEST_N is present-but-empty, which results in passing -n '' to pytest-xdist (a hard failure). Consider treating empty/whitespace values as unset (e.g., fallback to 10 after .strip()), or validate and fail with a clear message.
| '-n', os.environ.get('PYTEST_N', '10'), | |
| '-n', (os.environ.get('PYTEST_N') or '').strip() or '10', |
| if n := a.Action.GetInput("n"); n != "" { | ||
| ctx = env.Set(ctx, "PYTEST_N", n) | ||
| } |
There was a problem hiding this comment.
This change introduces new behavior (propagating the n action input into the test environment) but there’s no unit/integration coverage asserting the input-to-env wiring. Given the existing Go test suite in this module, consider adding a targeted test around the env injection (or factoring it into a helper that can be unit-tested) to prevent regressions.
There was a problem hiding this comment.
i can add unit tests, but this will require much bigger changes as the existing code is not readily testable. This has historically be manually tested. Let me know how to proceed. Happy to add unit tests but this will require refactor of the existing code to make it testable
Addresses PR #730 review: - Trim + validate `n` in main.go as positive integer or "auto", returning a clear error for invalid values instead of letting pytest-xdist fail with an opaque CLI error. - Treat an empty/whitespace PYTEST_N env var as unset in pytest_run.py so `-n ''` is never passed to pytest. Co-authored-by: Isaac
pytest_run.py already strips and defaults PYTEST_N, and pytest-xdist surfaces clear CLI errors for non-integer values. Remove the Go-side validation so the action is a simple pass-through — this also keeps `n: 'auto'` working as a pytest-xdist native mode. Co-authored-by: Isaac
Exercises the n → PYTEST_N wiring when the skipped integration test is run manually with real credentials. Co-authored-by: Isaac
Summary
ninput to the acceptance GitHub Action (default10) for configuring pytest-xdist parallel workers. Propagates via thePYTEST_Nenv var topytest_run.py; Go tests are unaffected.make fmtTest plan
make fmtpasses locallymake testpasses locallyn: '4'and confirm pytest uses-n 4nand confirm default of10Context: we need to be able to reduce parallel test execution in DQX as 10 parallel runs creates a high contention of testing clusters. This would require a new release.