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

testing: stdout output with no newline produces confusing/corrupt -v output #26325

Open
neild opened this issue Jul 11, 2018 · 7 comments
Open

testing: stdout output with no newline produces confusing/corrupt -v output #26325

neild opened this issue Jul 11, 2018 · 7 comments

Comments

@neild
Copy link
Contributor

@neild neild commented Jul 11, 2018

package out_test

import (
	"fmt"
	"testing"
)

func TestOut(t *testing.T) { fmt.Print("TestOut") }
$ go test .
ok  	testout	0.016s
$ go test . -v
=== RUN   TestOut
Test--- PASS: TestOut (0.00s)
PASS
ok  	testout	0.017s

Output to stdout during tests is captured and printed along with t.Logf et al. output when -v is supplied. If the output doesn't end with a newline, the output is as above.

We have some tools that parse test output which get confused in this situation. It's easy enough to avoid (just print a newline in your test) but it would also be pretty trivial for the testing package could add a newline when one is missing.

@mvdan
Copy link
Member

@mvdan mvdan commented Jul 11, 2018

This does seem like something to fix, but note that you should be running go test -json to parse the test results.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Jul 11, 2018

There are some tools like https://github.com/rakyll/gotest which parses command line go test output. This will break those I believe.

@mvdan
Copy link
Member

@mvdan mvdan commented Jul 11, 2018

The plaintext output has already been changed in the past, for example in 3a8b9cf. And now that there is go test -json, I'd imagine that it's less of a problem to make small changes to the plaintext output.

@neild
Copy link
Contributor Author

@neild neild commented Jul 11, 2018

I don't think we should change anything other than, possibly, adding a newline at the end of the test output when one isn't present. I'd be surprised if anything depends on the current behavior. e.g., https://github.com/rakyll/gotest will fail to parse test output that doesn't end in a newline.

I actually didn't know about go test -json! That's definitely a better approach for automated parsing. I lean towards saying we should add that missing newline in the text output anyway when needed; I can't imagine anyone actually wants to see the output in the example above.

@neild
Copy link
Contributor Author

@neild neild commented Jul 11, 2018

Heh. So it turns out go test -json works by parsing the normal text output...and it gets just as confused by this case.

{"Time":"2018-07-11T09:30:22.855058114-07:00","Action":"run","Package":"_/tmp/testout","Test":"TestOut"}
{"Time":"2018-07-11T09:30:22.855222613-07:00","Action":"output","Package":"_/tmp/testout","Test":"TestOut","Output":"=== RUN   TestOut\n"}
{"Time":"2018-07-11T09:30:22.855255176-07:00","Action":"output","Package":"_/tmp/testout","Test":"TestOut","Output":"Out--- PASS: TestOut (0.00s)\n"}
{"Time":"2018-07-11T09:30:22.855268402-07:00","Action":"output","Package":"_/tmp/testout","Output":"PASS\n"}
{"Time":"2018-07-11T09:30:22.855331884-07:00","Action":"output","Package":"_/tmp/testout","Output":"ok  \t_/tmp/testout\t0.011s\n"}
{"Time":"2018-07-11T09:30:22.856056764-07:00","Action":"pass","Package":"_/tmp/testout","Elapsed":0.012}

Contrast to the same test with a newline in the output, which correctly includes a "pass" action for the TestOut test:

{"Time":"2018-07-11T09:30:54.037472791-07:00","Action":"run","Package":"_/tmp/testout","Test":"TestOut"}
{"Time":"2018-07-11T09:30:54.037651722-07:00","Action":"output","Package":"_/tmp/testout","Test":"TestOut","Output":"=== RUN   TestOut\n"}
{"Time":"2018-07-11T09:30:54.037666684-07:00","Action":"output","Package":"_/tmp/testout","Test":"TestOut","Output":"Out\n"}
{"Time":"2018-07-11T09:30:54.037685506-07:00","Action":"output","Package":"_/tmp/testout","Test":"TestOut","Output":"--- PASS: TestOut (0.00s)\n"}
{"Time":"2018-07-11T09:30:54.037695155-07:00","Action":"pass","Package":"_/tmp/testout","Test":"TestOut","Elapsed":0}
{"Time":"2018-07-11T09:30:54.037708614-07:00","Action":"output","Package":"_/tmp/testout","Output":"PASS\n"}
{"Time":"2018-07-11T09:30:54.037761824-07:00","Action":"output","Package":"_/tmp/testout","Output":"ok  \t_/tmp/testout\t0.011s\n"}
{"Time":"2018-07-11T09:30:54.038279369-07:00","Action":"pass","Package":"_/tmp/testout","Elapsed":0.011}

@suzmue
Copy link
Contributor

@suzmue suzmue commented Jul 14, 2021

Could the start of the line requirement be removed when parsing the output?

This will introduce new false positives, but the current implementation is not protected from false positives if the program prints a matching string to os.Stdout.

The consumers of go test -json are likely tools, which could also be built to handle multiple "pass"/"fail"/"skip" events and update its status if it receives more than one (eg the last one is the result). Sending too few events prevents tools from recovering without having to parse every output themselves, which defeats the point of using go test -json.

This does not address the issues with interleaving output presented in other issues, but is an easy change that does not prevent fixing these other changes in the future.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jul 14, 2021

+1 to @suzmue's suggestion, we should look at whether this is feasible.

We're always going to have false positives and negatives parsing test output, as long as test messages are sent on os.Stdout with arbitrary user data. If we drop the requirement for starting newlines, that seems like a better point on the tradeoff than when we are now though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants