Skip to content

Conversation

@erikburt
Copy link
Contributor

@erikburt erikburt commented Jun 6, 2025

Fix bug where panic'd test is swallowed when there is no terminal event

Changes

  • Refactored a little section for readability
  • Fixed edge case, by inserting a fake terminal event if a panic/race was left 'unterminated'.
  • Added two tests which load real test output which caused this edge case

@erikburt erikburt self-assigned this Jun 6, 2025
@erikburt erikburt marked this pull request as ready for review June 6, 2025 23:38
@erikburt erikburt requested a review from a team as a code owner June 6, 2025 23:38
@erikburt erikburt changed the title fix: placeholder tests reproducing panic swallow fix: flakeguard swallowing panic Jun 7, 2025
@sebawo sebawo requested a review from Copilot June 11, 2025 12:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Fixes a bug where tests that panic without a terminal event get swallowed by injecting a fake fail event at the end.

  • Refactored panic/race detection in parser.go and changed detectedEntries to use rawEventData.
  • Added logic to append a fake terminal ("fail") event when no pass/fail/skip follows a panic or race.
  • Introduced fixtures and two new tests to cover panic-only scenarios.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
tools/flakeguard/runner/parser/testdata/events-with-panic-single.json New JSON fixture representing a single-test panic without termination
tools/flakeguard/runner/parser/parser_test.go Added TestParseFiles_WithPanicEventFile and specific single-test panic case
tools/flakeguard/runner/parser/parser.go Refactored output handling, updated detectedEntries type, and inserted fake terminal event
Comments suppressed due to low confidence (2)

tools/flakeguard/runner/parser/parser_test.go:1151

  • The failure message is misleading: it asserts 1 result but mentions 6. Update the message to "Expected 1 test result from file."
assert.Equal(t, 1, len(results), "Expected 6 test results from file.")

tools/flakeguard/runner/parser/parser.go:262

  • Logic for unterminated race events was added alongside panic handling, but no test covers the fake terminal event insertion for race cases. Consider adding a fixture and test that simulates a race-only event sequence to ensure this path is validated.
if state.panicDetectionMode || state.raceDetectionMode {

@erikburt erikburt merged commit 9687993 into main Jun 11, 2025
62 checks passed
@erikburt erikburt deleted the fix/debug-panic-parsing branch June 11, 2025 17:56
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.

3 participants