Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't treat debug messages as errors #336

Merged
merged 2 commits into from Jul 8, 2023
Merged

Conversation

Pawka
Copy link
Contributor

@Pawka Pawka commented Jun 12, 2023

Teaching gotestsum to recognize Golang cache debug output. When gocachehash=1 or gocachetest=1 debug options are enabled, output is sent to stderr. gotestum treats unrecognised stderr messages as errors what makes test suite run marked as failed.

Example of testing gotestsum repository without this PR. Each debug info line is treated as error:

$ GODEBUG=gocachehash=1,gocachetest=1 ./gotestsum
...
DONE 183 tests, 2 skipped, 13819 errors in 0.726s

After fixing the issue:

$ GODEBUG=gocachehash=1,gocachetest=1 ./gotestsum
...
DONE 183 tests, 2 skipped in 0.523s

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

This is great! Thank you for the fix.

Do you know if there is any other way to enable this debug output? (ex: a flag to go build).

If this env var is the only way to enable it maybe we should add a check for the env var?

if os.Getenv("GODEBUG") == "" {
    return false
}

A test for this, something similar to TestScanOutput_WithNonJSONLines, would also be great.

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thinking about it more, I don't think we need to guard on the env var because this is only scanning stderr, which we expect to be empty most of the time.

I added the test in a second commit.

@dnephin dnephin merged commit 049fc26 into gotestyourself:main Jul 8, 2023
7 checks passed
@Pawka
Copy link
Contributor Author

Pawka commented Jul 10, 2023

Thanks for merge! Sorry for not picking this up earlier - I was a bit distracted other things (summer, etc.).

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.

None yet

2 participants