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

cmd/go: go test -json outputs too verbose #40588

Open
hyangah opened this issue Aug 5, 2020 · 9 comments
Open

cmd/go: go test -json outputs too verbose #40588

hyangah opened this issue Aug 5, 2020 · 9 comments
Labels
Milestone

Comments

@hyangah
Copy link
Contributor

@hyangah hyangah commented Aug 5, 2020

Without -json

golang.org/x/sync$ go test ./...
ok      golang.org/x/sync/errgroup      (cached)
ok      golang.org/x/sync/semaphore     (cached)
ok      golang.org/x/sync/singleflight  (cached)
ok      golang.org/x/sync/syncmap       (cached)

With -json

golang.org/x/sync$ go test -json ./...
{"Time":"2020-08-05T10:45:49.695703-04:00","Action":"run","Package":"golang.org/x/sync/semaphore","Test":"TestWeighted"}
{"Time":"2020-08-05T10:45:49.698457-04:00","Action":"output","Package":"golang.org/x/sync/semaphore","Test":"TestWeighted","Output":"=== RUN   TestWeighted\n"}
{"Time":"2020-08-05T10:45:49.69849-04:00","Action":"output","Package":"golang.org/x/sync/semaphore","Test":"TestWeighted","Output":"=== PAUSE TestWeighted\n"}
...
{"Time":"2020-08-05T10:57:34.15016-04:00","Action":"output","Package":"golang.org/x/sync/syncmap","Test":"TestConcurrentRange","Output":"--- PASS: TestConcurrentRange (0.34s)\n"}
{"Time":"2020-08-05T10:57:34.150165-04:00","Action":"pass","Package":"golang.org/x/sync/syncmap","Test":"TestConcurrentRange","Elapsed":0.34}
{"Time":"2020-08-05T10:57:34.150172-04:00","Action":"output","Package":"golang.org/x/sync/syncmap","Output":"PASS\n"}
{"Time":"2020-08-05T10:57:34.150176-04:00","Action":"output","Package":"golang.org/x/sync/syncmap","Output":"ok  \tgolang.org/x/sync/syncmap\t(cached)\n"}
{"Time":"2020-08-05T10:57:34.150855-04:00","Action":"pass","Package":"golang.org/x/sync/syncmap","Elapsed":0.006}

Recently vscode-go switched to use go test -json and outputs Output field, but found this is too verbose (more like -v mode).

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 5, 2020

Change https://golang.org/cl/246918 mentions this issue: src/testUtil.ts: switch to -json mode only if -v is set

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 5, 2020

Could you be more specific about what should and should not be printed? Why did vscode-go switch to -json? Can it filter the output on its own instead?

When the -json flag is passed to go test, it passes -test.v to test processes, so they generate verbose output, and testing.Verbose() returns true, which may affect test behavior. -json causes go test to reformat that output as JSON (using code based on cmd/test2json). The content is generally the same though.

Changing -json output tends to be a very delicate operation. Code that depends on it tends to be very tightly coupled, so almost any change is a breaking change.

@hyangah
Copy link
Contributor Author

@hyangah hyangah commented Aug 5, 2020

For example, the followings are 'output' type Action but these lines wouldn't appear in the non-verbose mode output.
Non-verbose mode outputs include only a single line result per package if tests are successful,
and the details will be printed only for failed tests.

{"Time":"2020-08-05T10:57:34.15016-04:00","Action":"output","Package":"golang.org/x/sync/syncmap","Test":"TestConcurrentRange","Output":"--- PASS: TestConcurrentRange (0.34s)\n"}
{"Time":"2020-08-05T10:57:34.150165-04:00","Action":"pass","Package":"golang.org/x/sync/syncmap","Test":"TestConcurrentRange","Elapsed":0.34}

vscode-go has to rewrite the the file names printed in the log messages to the absolute paths. Without knowing the package path of the test, we can't do this rewrite. Unfortunately, in the normal test output, the package path is printed at the end. For example,

$ go test ./... -v
=== RUN   TestParseLine
--- PASS: TestParseLine (0.00s)
=== RUN   TestParseSet
--- PASS: TestParseSet (0.00s)
=== RUN   TestString
=== RUN   TestString/nsTest
    TestString/nsTest: parse_test.go:206: assume it's failed: nsTest
=== RUN   TestString/mbTest
    TestString/mbTest: parse_test.go:206: assume it's failed: mbTest
=== RUN   TestString/allocatedBytesTest
    TestString/allocatedBytesTest: parse_test.go:206: assume it's failed: allocatedBytesTest
=== RUN   TestString/allocsTest
    TestString/allocsTest: parse_test.go:206: assume it's failed: allocsTest
--- FAIL: TestString (0.00s)
    --- FAIL: TestString/nsTest (0.00s)
    --- FAIL: TestString/mbTest (0.00s)
    --- FAIL: TestString/allocatedBytesTest (0.00s)
    --- FAIL: TestString/allocsTest (0.00s)
FAIL
FAIL    golang.org/x/tools/benchmark/parse      0.022s

We can't rewirte parse_test.go to its full path until we see the last line FAIL golang.org/x/tools/benchmark/parse ....
Previously, we were buffering the test output until we get the last line in addition to various hacks to parse this unstructured test output. But that made the recent test output streaming feature (available go1.14?) not very useful.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 5, 2020

cc @bcmills @matloob

I don't think we can drop those lines though. It may break other tools that depend on them to reconstruct the complete output of the test. Can vscode-go filter them out?

@hyangah
Copy link
Contributor Author

@hyangah hyangah commented Aug 5, 2020

@jayconrod Filtering was what i initially attempted to do to fix our regression bug, and it's harder than I thought. Can it add an extra field or some sign whether the Output is for verbose or not?

Currently we switched back not to use -json if users didn't set -v. It's sad that we can't utilize it even though this -json output was introduced to ease integration with other tools including IDEs.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 5, 2020

Can it add an extra field or some sign whether the Output is for verbose or not?

Maybe... I think all the messages from the test process to go test start with something like ^\s*(---|===) , plus the lines that start with PASS or FAIL at the end. go test reads those, but the output is mixed together with the test's own output, so doing anything with it is pretty error-prone.

We have a lot of open issues related to this actually, and I wonder if we need a more complete redesign for this mechanism.

@ALTree ALTree added the NeedsDecision label Aug 5, 2020
@toothrot toothrot added this to the Backlog milestone Aug 5, 2020
gopherbot pushed a commit to golang/vscode-go that referenced this issue Aug 5, 2020
CL/242540 made runTest uses -json mode to ease streaming
test output processing. However, it turned out -json makes
the test output more verbose than the standard test output.
(golang/go#40588)

Verbose output by default is not ideal especially when
running tests on many packages. This change brings back
the old test output processing logic when users do not use
the `-v` flag. The `go test` command will stream test
output only if the `-v` flag is specified, so the original
issue CL/242540 remains addressed.

Fixes #471
Updates #316
Updates golang/go#40588

Change-Id: I29d6d1319acccab2457300c118216ceebb4c4033
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/246918
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 5, 2020

Change https://golang.org/cl/247017 mentions this issue: [release] src/testUtil.ts: switch to -json mode only if -v is set

gopherbot pushed a commit to golang/vscode-go that referenced this issue Aug 5, 2020
CL/242540 made runTest uses -json mode to ease streaming
test output processing. However, it turned out -json makes
the test output more verbose than the standard test output.
(golang/go#40588)

Verbose output by default is not ideal especially when
running tests on many packages. This change brings back
the old test output processing logic when users do not use
the `-v` flag. The `go test` command will stream test
output only if the `-v` flag is specified, so the original
issue CL/242540 remains addressed.

Fixes #471
Updates #316
Updates golang/go#40588

Change-Id: I29d6d1319acccab2457300c118216ceebb4c4033
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/246918
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
(cherry picked from commit bec9cf3)
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/247017
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
@dnephin
Copy link
Contributor

@dnephin dnephin commented Aug 13, 2020

If you are looking to emulate the non-verbose (I call it quiet) go test output from the json output, I have done that here: https://github.com/gotestyourself/gotestsum/blob/83c209d7c72e6c0889f40e965d71647fd6ea6600/testjson/format.go#L29-L38. You may be able to do the same.

Edit: (to respond to the below comment without creating too much noise on this thread)

There is preprocessing that happens in gotestsum, but none of it is required for this function (*Execution is not used). The linked function, plus the two small helpers it calls, are the only logic required to emulate the standard quiet format.

@hyangah
Copy link
Contributor Author

@hyangah hyangah commented Aug 13, 2020

Thanks @dnephin. I see the code and it assumes some preprocessing and filtering of the test events before the standard quiet formatter is applied. So, I assume majority of the heuristic is outside of the linked function.

Currently we are working around by simply not using -json if -v isn't set, which turned out to be noticeably faster in our specific case.

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
6 participants
You can’t perform that action at this time.