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

Add IgnoreNonJSONOutputLines, --ignore-non-json-output-lines #194

Merged
merged 4 commits into from May 15, 2021

Conversation

dagood
Copy link
Contributor

@dagood dagood commented May 10, 2021

Adds a ScanConfig field IgnoreNonJSONOutputLines, enabled from the command line by --ignore-non-json-output-lines. It causes gotestsum to write non-JSON 'go test' output lines to os.Stderr instead of failing.

(For #193. This is useful to handle underlying testing tools that mix test2json lines with other output and can't easily be changed to purely JSON.)

--ignore-non-json-output-lines causes gotestsum to write non-JSON test
output lines to os.Stderr instead of failing.
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.

Thank you for the PR! This looks great. Left a couple small suggestions.

I think a test case would also be a good addition. TestScanOutput_WithMissingEvents might work as an example. If the Stdout reader passes both json lines and non-json lines, and the Handler could capture the errors.

Also the update to the README mentioned in one of the comments would be great.

I'm also happy to make those changes in the next few days if you don't have the time.

Comment on lines +62 to +63
flags.BoolVar(&opts.ignoreNonJSONOutputLines, "ignore-non-json-output-lines", false,
"write non-JSON 'go test' output lines to stderr instead of failing")
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should hide this flag (similar to no-summary below). My thinking is that this flag should only be necessary in very rare cases (possibly only when running tests for the Go project itself), and by hiding it we prevent misuse of the flag in cases where there are better options. It would also remove a potential distraction when vieweing the --help output.

We can still include it in the README in the Custom go test command section. There's already a note there about the previous limitation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hid (and updated the test baseline).

When I tried to put this in the readme, it seemed like that paragraph was getting long and the difference between the script's stderr vs. gotestsum's stderr seemed muddy. So, I split the existing bit into two bullet points to try to make it a bit clearer what the new sentence is talking about. Otherwise kept it mostly the same:

Note: when using --raw-command, the script must follow a few rules about stdout and stderr output:

  • The stdout produced by the script must only contain the test2json output, or gotestsum will fail. If it isn't possible to change the script to avoid non-JSON output, you can use --ignore-non-json-output-lines to ignore
    non-JSON lines and write them to gotestsum's stderr instead.
  • Any stderr produced by the script will be considered an error (this behaviour is necessary because package build errors are only reported by writting to stderr, not the test2json stdout). Any stderr produced by tests is not considered an error (it will be in the test2json stdout).

testjson/execution.go Outdated Show resolved Hide resolved
testjson/execution.go Outdated Show resolved Hide resolved
@dagood
Copy link
Contributor Author

dagood commented May 12, 2021

Thanks for the review! Agreed on all points, I'll try to get it done today.

dagood and others added 3 commits May 12, 2021 14:20
Co-authored-by: Daniel Nephin <dnephin@gmail.com>
Use t.Run to separate the cases, use DeepEqual to check the slices removing the need for a
separate length check, and wrap a line to make the linter happy.
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.

Thank you! Looks great!

I made some minor changes to the test in the latest commit to fix the lint error and improve the failure messages in case the test ever fails.

@dnephin dnephin merged commit ecb7c69 into gotestyourself:main May 15, 2021
@dagood dagood deleted the ignorenonjson branch May 16, 2021 23:57
@thaJeztah thaJeztah mentioned this pull request Jul 19, 2021
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