Clearer failure/errors in failure_reason & failure_expanded#64
Conversation
Currently, the entire failure from PyTest (PyTest calls it longreprtext) is set as failure_reason. However, failure_reason is intended for a short one-line summary, and gets truncated to ~1KIB, which isn't great for longreprtext. Buildkite also supports failure_expanded, to included detailed information and backtraces on errors. So, derive that from the various shapes that longrepr can take, and include them in the uploaded JSON payload.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the handling of test failure information in the Buildkite Test Analytics collector by providing clearer failure summaries and detailed expanded information. The changes separate the short failure reason (for UI display) from the detailed failure information (for expanded views).
Key changes:
- Added a new
failure_expandedfield toTestResultFailedto store detailed failure information including stack traces - Implemented
failure_reasons()function to parse different PyTest longrepr formats and extract appropriate failure summaries - Updated the plugin to use the new failure parsing logic instead of the raw longreprtext
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/buildkite_test_collector/pytest_plugin/failure_reasons.py |
New module implementing failure reason parsing logic for different PyTest longrepr formats |
src/buildkite_test_collector/collector/payload.py |
Added failure_expanded field to TestResultFailed and updated JSON serialization |
src/buildkite_test_collector/pytest_plugin/buildkite_plugin.py |
Updated to use new failure parsing logic and added debug logging |
tests/buildkite_test_collector/pytest_plugin/test_plugin.py |
Added comprehensive tests for different failure scenarios |
tests/buildkite_test_collector/conftest.py |
Updated test fixture to include failure_expanded data |
tests/buildkite_test_collector/collector/test_payload.py |
Updated test assertions to verify failure_expanded field |
| case None: | ||
| return None, None | ||
|
|
||
| case str() as s: |
There was a problem hiding this comment.
[nitpick] The match case pattern 'str() as s' is redundant. Consider using 'case str(s):' or 'case s if isinstance(s, str):' for better readability.
| case str() as s: | |
| case str(s): |
| case str() as s: | ||
| lines = s.splitlines() | ||
| failure_reason = lines[0] if lines else s | ||
| return failure_reason, [{"expanded": lines[1:]}] |
There was a problem hiding this comment.
When there's only one line in the string, this will return an empty list for 'expanded'. Consider returning None for failure_expanded when there are no additional lines, similar to the single-line case at the end of the function.
| return failure_reason, [{"expanded": lines[1:]}] | |
| return failure_reason, [{"expanded": lines[1:]}] if len(lines) > 1 else (failure_reason, None) |
CI is still testing in Python down to 3.8
gchan
left a comment
gchan
left a comment
There was a problem hiding this comment.
LGTM, some minor questions
| class TestResultFailed: | ||
| """Represents a failed test result""" | ||
| failure_reason: Optional[str] | ||
| failure_expanded: Optional[Iterable[Mapping[str, Iterable[str]]]] = None |
There was a problem hiding this comment.
Not a huge fan of this data structure but understand it's because of the existing API :)
| if isinstance(longrepr, ExceptionRepr) and longrepr.reprcrash is not None: | ||
| return _handle_exception_repr_longrepr(longrepr) | ||
|
|
||
| return _handle_default_longrepr(longrepr) |
There was a problem hiding this comment.
I'm trying to understand Pytest's longrepr.
Do we need to handle when it's a TerminalRepr?
There was a problem hiding this comment.
That can be handled by the default _ handler, which just uses str(longrepr) and comes out okay.
| msg: str | ||
| ) -> tuple[str | None, Iterable[Mapping[str, Iterable[str]]] | None]: | ||
| """Handle tuple longrepr case (path, line, msg)""" | ||
| failure_reason = msg |
There was a problem hiding this comment.
Is there any chance the tuple could have a very long message?
There was a problem hiding this comment.
Possibly. We truncate it server side in that case.
I think this code path might only happen when the longrepr is for a skipped test, not a failed test, but I'm not entirely sure.
| @@ -0,0 +1,113 @@ | |||
| """Buildkite Test Engine PyTest failure reason mapping""" | |||
|
|
|||
| from __future__ import annotations | |||
There was a problem hiding this comment.
This is just for Python 3.9 which we'll drop pretty soon, it's EOL in a couple of months.
2 participants
Currently, the entire failure from PyTest (PyTest calls it
longreprtext) is set asfailure_reason.However,
failure_reasonis intended for a short one-line summary, and gets truncated to ~1KIB, which isn't great forlongreprtext.Buildkite also supports
failure_expanded, for including detailed information and backtraces on errors.So, derive that from the various shapes that
longreprcan take, and include them in the uploaded JSON payload.failure_reasonsummaries: