Skip to content

Add defensive handling for missing test results#103

Closed
jasonwbarnett wants to merge 2 commits intobuildkite:te-5406-test-doesnt-have-result-when-skipped-during-setup-phasefrom
altana-ai:defensive-result-handling
Closed

Add defensive handling for missing test results#103
jasonwbarnett wants to merge 2 commits intobuildkite:te-5406-test-doesnt-have-result-when-skipped-during-setup-phasefrom
altana-ai:defensive-result-handling

Conversation

@jasonwbarnett
Copy link
Copy Markdown
Contributor

@jasonwbarnett jasonwbarnett commented Apr 14, 2026

Context

Follow-up to #102. While reviewing the setup-skip fix, I identified a few additional hardening opportunities that prevent the same class of bug (result=None → missing "result" key → test-engine-client treats it as unknown) from resurfacing through other code paths.

See: buildkite/test-engine-client#464

Changes

Warning logs for unset results — Both as_json() and finalize_test() now log a warning when a test has no result set. This makes any future result=None bug immediately visible in logs instead of silently producing bad JSON. We intentionally do not emit a default "result" value — test-engine-client treats any unrecognized value the same as a missing key, so a default wouldn't help downstream.

if/elif in update_test_result() — Changed independent if statements to if/elif/elif to make the mutual exclusivity of pytest outcomes (passed, failed, skipped) explicit.

Integration tests for skip scenarios — Subprocess-based tests covering @pytest.mark.skip, @pytest.mark.skipif, pytest.skip() in fixtures, and pytest.skip() in the test body. Verifies the actual JSON output contains "result": "skipped" end-to-end.

Note on test-engine-client hardening

The root cause of #464 is that RunResult.Status() lets a single test with an unrecognized status poison the entire run — the passed + skipped + failedMuted == len(r.tests) check fails and falls through to RunStatusUnknown. A more defensive pattern would be to either:

  • Exclude tests with unrecognized statuses from the total count
  • Treat missing/unrecognized results as "skipped" (the least harmful default)
  • At minimum, log which tests had unrecognized statuses so the source collector can be debugged

This would prevent any future collector bug in any language from breaking OnlyMutedFailures().

Test plan

  • All 99 tests pass (existing + new integration tests)
  • New integration tests cover all four skip mechanisms
  • test_all_tests_have_result_key asserts every test entry in JSON has a result key

🤖 Generated with Claude Code

Jason Barnett and others added 2 commits April 14, 2026 02:38
… for skips

Hardens the collector against any code path that leaves result=None:

- as_json() now always emits a "result" key, defaulting to "unknown" with
  a warning log when result is unset. Previously the key was silently
  omitted, causing test-engine-client to miscount test statuses.
- finalize_test() logs a warning when a test reaches finalization without
  a result, making the issue visible before serialization.
- update_test_result() uses elif instead of independent if statements to
  make the mutual exclusivity of pytest outcomes explicit.
- Adds integration tests exercising @pytest.mark.skip, @pytest.mark.skipif,
  pytest.skip() in fixtures, and pytest.skip() in the test body.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Emitting "result": "unknown" doesn't improve downstream behavior —
test-engine-client's RunResult.Status() treats unrecognized values
the same as a missing key, so it wouldn't prevent the accounting
bug. Keep the warning log for observability without introducing a
new result value that no consumer handles.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@nprizal
Copy link
Copy Markdown
Contributor

nprizal commented Apr 14, 2026

The changes for logging the unknown result sound good. The warning logs in as_json() and finalize_test() will make any future result=None bugs immediately visible, which is a nice observability improvement.

However, regarding the:

Note on test-engine-client hardening

We're intentionally keeping the current behavior where unrecognized test statuses cause the run to be treated as unknown.

The design principle in test-engine-client is: propagate the test runner's exit code unless we can prove that all failures come from muted tests, and nothing else. This is the only case where we suppress a non-zero exit code. If we can't fully account for every test's outcome, we don't have that proof, so the error propagates.

In a correctly functioning collector, every test should have a known result: passed, failed, or skipped. A test showing up as "unknown" is not a normal condition to handle gracefully; it means something is broken, either in the collector or somewhere deep in the test suite. Silencing that signal (by excluding unknowns from the count, or defaulting them to "skipped") would hide the problem rather than surface it.

We've been bitten by this before. There have been cases across multiple collectors where legitimate failures were reported without a recognized result, for example runtime errors in Vitest/Jest (#377) and rspec exiting non-zero with no reported failures (#436). In each case, the fact that test-engine-client treated the unknown result as untrusted is what surfaced the bug. If we had been lenient, those failures would have silently passed CI.

In fact, #102 and this PR are proof that we need this principle. If we had been suppressing unknown test results, we wouldn't have known there was a bug in the Python collector in the first place.

@nprizal nprizal deleted the branch buildkite:te-5406-test-doesnt-have-result-when-skipped-during-setup-phase April 15, 2026 00:43
@nprizal nprizal closed this Apr 15, 2026
@nprizal
Copy link
Copy Markdown
Contributor

nprizal commented Apr 15, 2026

Hi @jasonwbarnett, I merged #102 and the branch gets deleted. Since this PR targets that branch, GitHub automatically closed this PR. Can you please reopen and change the target to buildkite:main?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants