Treat pytest collection errors as terminal#553
Conversation
|
On it! Follow along with my review here: https://buildkite.com/buildkite/buildsworth-review-pull-request/builds/972 |
|
On it! Follow along with my review here: https://buildkite.com/buildkite/buildsworth-review-pull-request/builds/973 |
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug fix is scoped to the pytest JSON-result mapping, has regression coverage for tagged collection errors and empty scopes, and the PR check is green, so I read this as L1.
Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 972, then answer my questions about the findings.
The Python collector now reports collection-phase failures as failed JSON test results and tags them with test.pytest_collection_error. Those results use the pytest file node ID as the name and an empty scope because pytest never collected a function or class. bktec was rebuilding every Test Engine JSON result path as scope::name. For collection errors that produced ::tests/foo.py, which is not a valid pytest node ID and would be passed back to pytest on retry. Build pytest JSON result paths with the bare name when scope is empty, and reuse that mapping for pytest-pants. When the collector tag is present, record the failure as an error outside of tests so the existing retry loop terminal guard does not retry it. This aligns pytest collection failures with the Go build-failure handling added in #540: they are collection/setup failures, not flaky examples. The path-only behavior remains for empty-scope results without the tag, so bktec does not infer collection errors from shape alone. Add parser-level regression tests for tagged collection errors, untagged empty-scope failures, and normal scoped node IDs. Related to #485 and buildkite/test-collector-python#108. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019ede8b-da3c-71a1-93d8-925b05865b43
pda
force-pushed
the
dont-retry-pytest-collection-errors
branch
from
b911495 to
7e55182
Compare
![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: b911495c42
ℹ️ 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".
| Path: path, | ||
| }, test.Result) | ||
|
|
||
| if test.Tags[pytestCollectionErrorTag] == "true" { |
There was a problem hiding this comment.
For a normal pytest collection/import/syntax error without --continue-on-collection-errors, pytest exits with code 2 rather than 1 (the pytest exit-code docs distinguish code 1 as tests failing after collection and code 2 as interruption: https://docs.pytest.org/en/stable/reference/exit-codes.html). Because Pytest.Run returns immediately for any non-1 exit before calling runParseJSON, the JSON report that buildkite-test-collector writes for these collection errors is never parsed, so this new tag check is not reached and bktec still reports an unknown run with zero parsed results instead of the intended terminal collection-error status.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Good catch — pytest collection errors can exit 2 before the JSON parsing path ran. Fixed in 5752935 by allowing the JSON collector path to parse exit 2 results while preserving the original command error, and added a regression test for tagged collection-error JSON on exit 2.
Verified with:
go test ./internal/runner -run 'TestPytestRun_JSONExit2ParsesCollectionError|TestPytestRunParseJSON_CollectionError|TestPytestRun_CollectionError'I also tried go test ./internal/runner, but the broader local suite fails in existing Cypress/Jest tests because local JS test dependencies/paths are unavailable (cypress not found / Jest “No tests found”).
|
On it! Follow along with my review here: https://buildkite.com/buildkite/buildsworth-review-pull-request/builds/974 |
There was a problem hiding this comment.
Bug fix is scoped to pytest JSON-result mapping, has regression coverage for tagged collection errors and empty scopes, and the PR check is green, so I read this as L1.
Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 974, then answer my questions about the findings.
Amp-Thread-ID: https://ampcode.com/threads/T-019edeb3-b729-71ec-8ffa-96a21e734076 Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Focused pytest JSON-result mapping bug fix with regression coverage for tagged collection errors and empty scopes, no public API surface change, and the current PR check is green, so I read this as L1.
Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 973, then answer my questions about the findings.
|
On it! Follow along with my review here: https://buildkite.com/buildkite/buildsworth-review-pull-request/builds/976 |
|
@codex review |
There was a problem hiding this comment.
I found one issue in the new exit-2 JSON parsing path: untagged exit-2 reports can become retryable, which can mask the original pytest interruption/error.
Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 976, then answer my questions about the findings.
| return cmdErr | ||
| // pytest exits 2 for collection errors, and buildkite-test-collector can | ||
| // still write tagged JSON results for those errors. | ||
| if p.useJUnit || exitError.ExitCode() != 2 { |
There was a problem hiding this comment.
Blocking: This lets every JSON-mode exit 2 go through the normal failed-test path. pytest also uses exit 2 for interrupted runs, and the collector can still have already written failed test entries before the interrupt; those entries do not have test.pytest_collection_error, so runParseJSON leaves the result as RunStatusFailed. runTestsWithRetry will then retry those failures and, if the retry passes, return the later nil error, masking the original interrupted run. Could we keep exit 2 terminal unless parsing actually found the collection-error tag (for example by setting result.error after parsing when exit code 2 did not produce RunStatusError)?
There was a problem hiding this comment.
🤖 Fixed in b4fc68d. Exit-2 JSON is still parsed so tagged collection errors can set RunStatusError, but an untagged exit-2 report now sets result.error to the original command error after parsing. That keeps interrupted/non-collection exit 2 terminal and prevents runTestsWithRetry from retrying or masking it.
Verified with:
go test ./internal/runner -run 'TestPytestRun_JSONExit2|TestPytestRunParseJSON_CollectionError|TestPytestRunParseJSON_EmptyScopeWithoutCollectionErrorTag|TestPytestPantsRun_JSONExit2'
go test ./internal/runner -run 'TestPytestRun_JSONExit2|TestPytestPantsRun_JSONExit2|TestPytestPantsRun_CommandFailed'
go test ./internal/command -run 'TestRunTestsWithRetry_RunResultError'
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 575293588d
ℹ️ 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".
| if exitError := new(exec.ExitError); errors.As(cmdErr, &exitError) && exitError.ExitCode() != 1 { | ||
| return cmdErr | ||
| // pytest exits 2 for collection errors, and buildkite-test-collector can | ||
| // still write tagged JSON results for those errors. | ||
| if p.useJUnit || exitError.ExitCode() != 2 { | ||
| return cmdErr |
There was a problem hiding this comment.
When a JSON pytest run exits 2 for an actual interruption rather than a tagged collection error, this now parses any partial collector JSON and leaves the run as RunStatusFailed if that partial report contains failed tests. With retries enabled, runTestsWithRetry will retry those tests and a successful retry can return success, masking that the original pytest process was interrupted. Please only make the exit-code-2 path retry-safe when the parsed report is tagged as a collection error; otherwise preserve the previous terminal non-1 behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in b4fc68d. Exit-2 JSON is still parsed only far enough to detect tagged collection errors; if parsing does not produce RunStatusError, the run is marked with the original exit-2 command error so it remains terminal and cannot be retried into a false pass. Added TestPytestRun_JSONExit2WithoutCollectionErrorIsTerminal for this case.
Verified with focused runner and retry tests.
| for _, test := range tests { | ||
| result.RecordTestResult(plan.TestCase{ | ||
| Identifier: test.ID, | ||
| Format: plan.TestCaseFormatExample, | ||
| Scope: test.Scope, | ||
| Name: test.Name, | ||
| // pytest can execute individual test using node id, which is a filename, classname (if any), and function, separated by `::`. | ||
| // Ref: https://docs.pytest.org/en/6.2.x/usage.html#nodeids | ||
| Path: fmt.Sprintf("%s::%s", test.Scope, test.Name), | ||
| }, test.Result) | ||
| recordPytestJSONTestResult(result, test) |
There was a problem hiding this comment.
For pytest-pants commands that propagate pytest's collection-error exit code 2 after writing the collector JSON, this shared recordPytestJSONTestResult call is never reached because the guard above still returns for every non-1 exit. Those tagged collection errors therefore won't become RunStatusError or preserve their file-only path, so pytest-pants misses the terminal handling added for the plain pytest runner.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in b4fc68d. pytest-pants now mirrors the pytest JSON behavior for exit 2: it parses tagged collection-error JSON so the shared collection-error mapping is reached, while untagged exit-2 JSON is marked terminal with the original command error. Added pytest-pants coverage for both tagged collection-error exit 2 and untagged terminal exit 2.
Verified with focused runner tests.
Allow pytest JSON exit 2 to be parsed for tagged collection errors while marking untagged exit 2 results as terminal so retries cannot mask interrupted runs. Apply the same behavior to pytest-pants and cover both tagged and untagged exit 2 JSON reports. Amp-Thread-ID: https://ampcode.com/threads/T-019edec0-20c7-7012-a2f0-d40f13e9c3cc Co-authored-by: Amp <amp@ampcode.com>
|
On it! Follow along with my review here: https://buildkite.com/buildkite/buildsworth-review-pull-request/builds/977 |
buildsworth-bk-app
Bot
dismissed
their stale review
Previous concerns have been addressed.
There was a problem hiding this comment.
The prior exit-2 retryability concern is addressed: untagged exit-2 JSON now restores the original command error for both pytest and pytest-pants, with regression coverage. This remains a focused pytest runner bug fix with a clear reproduction, tests, and green CI, so I read it as L1.
Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 977, then answer my questions about the findings.
|
@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
Description
When
buildkite-test-collectorreports a pytest collection error, the JSON result has an emptyscopeand a file-onlynamesuch astests/test_broken.py. bktec was rebuilding every pytest JSON result path asscope::name, producing invalid retry selectors like::tests/test_broken.py.This fixes the selector and treats explicitly tagged pytest collection errors as terminal errors outside of tests. Retrying individual pytest node IDs cannot fix import/syntax/collection failures, so this aligns pytest with the Go build-failure behavior from #540.
Context
buildkite/test-collector-python#108made collection errors visible in the JSON report and tags them withtest.pytest_collection_error=true. bktec can now use that explicit tag instead of guessing from empty scope alone.Changes
RunStatusError, which uses the existing terminal retry guard.pytest-pants.Testing
go test ./internal/runner -run '^TestPytestRun_CollectionError$' -vin a temporarymise/uvvirtualenv withpytestandbuildkite-test-collector>=1.3.0installedgo test ./internal/runner -run 'TestPytestRunParseJSON|TestPytestPathFromTestEngineResult'go test ./internal/runner -run '^TestPytestPantsRun_TestFailed$'go test ./internal/command -run 'TestRunTestsWithRetry_RunResultError|TestRunTestsWithRetry_GoTestBuildFailed'I also attempted
go test ./internal/runner ./internal/command, but this local environment is missing some runner dependencies such ascypressand the broader runner integration suite is not green here.Related