Skip to content

Conversation

@Tonkpils
Copy link

@Tonkpils Tonkpils commented Nov 21, 2025

Not sure how else to showcase it without having failing CI builds but here's an output example

image

This pull request adds support for emitting GitHub Actions error annotations for failed tests in the githubActionsFormat output. The main changes include parsing test output for failure locations, handling panics specially, and generating error annotations with file and line information for easier debugging in CI environments.

GitHub Actions error annotation support:

  • Added regex-based parsing of test output to extract file and line information for failed tests and panics (fileLinePattern, panicStackPattern in testjson/format.go). [1] [2]
  • Introduced the writeGitHubActionsError function to emit error annotations in GitHub Actions format, including special handling for panics and regular failures, with proper sanitization of annotation fields (testjson/format.go).
  • Modified the test formatter to call writeGitHubActionsError on failed test events, resulting in error annotations for each failure (testjson/format.go).

Test output updates:

  • Updated expected output in testjson/testdata/format/github-actions.out to include new error annotations for each failed test, showing file, line, and test name information. [1] [2] [3] [4] [5] [6]

@Tonkpils Tonkpils marked this pull request as ready for review November 21, 2025 20:06
var isPanic bool
var panicMessage strings.Builder
for _, outputLine := range outputLines {
if strings.Contains(outputLine, "panic:") {

Choose a reason for hiding this comment

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

I'm a bit worried this would be true for anything returning the string panic:

The next loop and if would use the regexp to extract the info

Maybe you could check for "panic:", loop, then test with regexp, and you match once, apply the panic/test results

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean to do this process

"panic:", loop, then test with regexp, and you match once, apply the panic/test results

For every time we see the panic: match or only the once?

What this does is to gather up all the messages that match panic: then attributes them to the matching stack trace in the next if/loop block.

Happy to make the necessary change. Just want to make sure I'm writing it up for the correct scenario you're thinking about 😅

Choose a reason for hiding this comment

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

I'm not sure either what I meant 😬

No matter how it could implemented I just want that gotestsum doesn't report a panic if someone called

t.Errorf("checking there is no panic: just in case")

I feel like adding a test about it would help you to figure how to catch the issue I thought about

Copy link
Author

@Tonkpils Tonkpils Nov 24, 2025

Choose a reason for hiding this comment

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

That makes sense. I can make the panic check a bit more strict but unfortunately I don't believe there's much we can do to prevent someone from writing an error message that may be similar to a panic initial error message.

I can change the condition to check if the entire output line starts out with panic: which is more strict than checking for a substring of panic: in the entire message.

Does that sound reasonable?

Either way, I'll see if I can add some more specific tests to this format so we can have a better picture of the setup/expectation.

Choose a reason for hiding this comment

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

Let's wait for the feedback of maintainers here.

I feel like I'm the only one who reviewed your PR right now

@Tonkpils Tonkpils marked this pull request as draft November 27, 2025 13:54
@Tonkpils Tonkpils marked this pull request as ready for review November 27, 2025 14:26
@Tonkpils Tonkpils marked this pull request as draft December 4, 2025 18:15
@Tonkpils Tonkpils marked this pull request as ready for review December 4, 2025 19:19
@Tonkpils Tonkpils marked this pull request as draft December 4, 2025 19:28
@Tonkpils Tonkpils force-pushed the tonkpils/github-actions-reporter branch from 7489e44 to a188b31 Compare December 4, 2025 19:37
@Tonkpils Tonkpils marked this pull request as ready for review December 4, 2025 19:39
@Tonkpils
Copy link
Author

@dnephin 👋 sorry for the direct ping, just wanted to make sure you didn't miss this PR. Do you have any feedback on whether this is good to be merged or if you'd like to see any changes? 🙇

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