testing: streaming output loses parallel subtest associations #38458
Comments
rsc
added
the
NeedsFix
rsc
changed the title
|
Isn't the TestTestdata/MIT-0.t1: license_test.go:138: testdata/MIT-0.t1:1,3: ↑ subtest name ↑ source location ↑ start of user-generated message |
|
I didn't notice the prefix. Having that prefix on every line is pretty noisy. It's turned the output into something that requires a machine to read. I think we should change this output again. I know we just did in Go 1.14, but the current output is simply not human-friendly. The prefix is stuttering, and "file:line:" should always be at the start of a line. |
|
Let's replace the prefix with tracking what test name has been printed in an === line most recently, and print a new === line if needed to change the "currently printing" test. |
|
This will change the work done for #24929 CC @jadekler @randall77 |
|
One consequence of #24929 (streaming t.Log output) is that |
|
Change https://golang.org/cl/229085 mentions this issue: |
|
I spent time looking into this today, and am working on a cl that implements @rsc's suggestion: https://go-review.googlesource.com/c/go/+/229085. Russ, your solution seems reasonable. Here's what that looks like: Before After Does this look right? As you can see, it's quite a bit more chatty when there's a high degree of interleaving, which could be a bummer. On the other hand, it does separate subtests better. If so, happy to continue on that CL and figure out a real way to implement this, instead of my hacky way. 🙂 (I also figured out a better way to capture and test subtest output, so minimally I'd like to get that into the testsuite) |
|
The indented === lines look wrong. They have no verb and they are indented. |
|
OK, I looked at the CL test. Starting a subtest inside a kicked-off goroutine is non-standard to say the least. The important part is that the output should not require any changes to test2json. Test2json expects === lines to be unindented and contain a verb. The obvious verb here is "CONT" for continue, so that no new verb is needed either. Instead of introducing the output redirect in package testing, I'd rather see a test script in cmd/go, testing both go test -v and go test -json. Then the diff should be very small. That is, I think the output should be: |
t.Parallel was blocking for some reason, so after some investigation I threw my hands up and went with the easy out to get at what I wanted. And, it was a prototype CL, so wasn't too concerned. But good to know that's unexpected for future tests!
SG, thanks for the direction! That helps me orient.
SG, thanks for the output. I'll work on updating the CL. 👍 |
|
I've updated https://go-review.googlesource.com/c/go/+/229085:
I'm not in love with the solution, though. It seems not-great to lock, record, and unlock everywhere we print. That seems like something that's easy to forget to do in the future. I explored adding a writer that keeps track of the test name on all outgoing writes, but quickly realized it would have to interpolate each outgoing line to figure out test name (no way to pass test name through io.Writer.Write AFAICT), which doesn't seem great. Would love thoughts if there's a better approach. (also on anything else, but particularly that) |
Don't do it as a generic |
Done. I've created a non-io.Writer writer (called testPrinter).
What would a prefix look like? Something like
Done.
I've just used fmt.Sprintf at the call site. Is that bad? Should I have |
|
Change https://golang.org/cl/232157 mentions this issue: |
Ignore this, bad mail :) |
Replaces #7559 See golang/go#38458 and golang/go#38382 (comment) Running tests in parallel, with background goroutines, results in test output not being associated with the correct test. `go test` does not make any guarantees about output from goroutines being attributed to the correct test case. The previous solution did not address this problem because test output could still be hidden when it was associated with a test that did not fail. You would have to look at all of the log output to find the relevant lines. It also made debugging test failures more difficult because each log line was very long and contained the test name twice. This commit attempts a new approach. Instead of printing all the logs, only print when a test fails. This should work well when there are a small number of failures, but may not work well when there are many test failures at the same time. In those cases the failures are unlikely a result of a specific test, and the log output is likely less useful. All of the logs are printed from the test goroutine, so they should be associated with the correct test. Also removes some test helpers that were not used, or only had a single caller. Packages which expose many functions with similar names can be difficult to use correctly.
|
I’ll add the release blocker label for Go1.15. |
|
@jadekler (and anyone else): Checking in on the status of this as an issue blocking the Go 1.15 beta. |
|
@toothrot AFAICT this is ready, just waiting for a +2. I have a +1 from bcmills already. |
Replaces #7559 Running tests in parallel, with background goroutines, results in test output not being associated with the correct test. `go test` does not make any guarantees about output from goroutines being attributed to the correct test case. The previous solution did not address this problem because test output could still be hidden when it was associated with a test that did not fail. You would have to look at all of the log output to find the relevant lines. It also made debugging test failures more difficult because each log line was very long and contained the test name twice. This commit attempts a new approach. Instead of printing all the logs, only print when a test fails. This should work well when there are a small number of failures, but may not work well when there are many test failures at the same time. In those cases the failures are unlikely a result of a specific test, and the log output is likely less useful. All of the logs are printed from the test goroutine, so they should be associated with the correct test. Also removes some test helpers that were not used, or only had a single caller. Packages which expose many functions with similar names can be difficult to use correctly. Related: golang/go#38458 (may be fixed in go1.15) golang/go#38382 (comment)
|
Backport issue(s) opened: #39308 (for 1.14). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
|
@jadekler, I noticed some awkwardness remaining with this fix using |
|
Ack - sounds like some test cases missing. Could you show some output? Bonus: if you have a test on hand that demonstrates this, that'd be nice too. :) |
|
@jadekler, I'm not sure how to do it without a |
|
Thanks @bcmills. Got some free cycles today - taking a look. |
This fixes an issue raised at #38458 (comment) in which --- PASS and --- FAIL lines would not trigger --- CONT lines of other tests. Change-Id: I0d8cc54d682a370d0a6ea6816a11b2e462a92efe Reviewed-on: https://go-review.googlesource.com/c/go/+/235997 Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Replaces #7559 Running tests in parallel, with background goroutines, results in test output not being associated with the correct test. `go test` does not make any guarantees about output from goroutines being attributed to the correct test case. The previous solution did not address this problem because test output could still be hidden when it was associated with a test that did not fail. You would have to look at all of the log output to find the relevant lines. It also made debugging test failures more difficult because each log line was very long and contained the test name twice. This commit attempts a new approach. Instead of printing all the logs, only print when a test fails. This should work well when there are a small number of failures, but may not work well when there are many test failures at the same time. In those cases the failures are unlikely a result of a specific test, and the log output is likely less useful. All of the logs are printed from the test goroutine, so they should be associated with the correct test. Also removes some test helpers that were not used, or only had a single caller. Packages which expose many functions with similar names can be difficult to use correctly. Related: golang/go#38458 (may be fixed in go1.15) golang/go#38382 (comment)
|
Change https://golang.org/cl/241660 mentions this issue: |
Updates #37419. Updates #38458. Updates #24929. Change-Id: I793bb20fa9db4432fc3a5b69956b7108e4695081 Reviewed-on: https://go-review.googlesource.com/c/go/+/241660 Run-TryBot: Jean de Klerk <deklerk@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
|
Change https://golang.org/cl/242058 mentions this issue: |
|
Change https://golang.org/cl/242057 mentions this issue: |
|
Change https://golang.org/cl/242059 mentions this issue: |
In #24929, we decided to stream chatty test output. It looks like, foo_test.go:138: TestFoo/sub-1: hello from subtest 1 foo_test.go:138: TestFoo/sub-2: hello from subtest 2 In this CL, we refactor the output to be grouped by === CONT lines, preserving the old test-file-before-log-line behavior: === CONT TestFoo/sub-1 foo_test.go:138 hello from subtest 1 === CONT TestFoo/sub-2 foo_test.go:138 hello from subtest 2 This should remove a layer of verbosity from tests, and make it easier to group together related lines. It also returns to a more familiar format (the pre-streaming format), whilst still preserving the streaming feature. Updates #38458. Fixes #39308. Change-Id: Iaef94c580d69cdd541b2ef055aa004f50d72d078 Reviewed-on: https://go-review.googlesource.com/c/go/+/229085 Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Andrew Bonventre <andybons@golang.org> Reviewed-on: https://go-review.googlesource.com/c/go/+/242057 Reviewed-by: Jean de Klerk <deklerk@google.com> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com> Run-TryBot: Jean de Klerk <deklerk@google.com>
…amd64 builder The parallel chatty tests added in CL 229085 fail on the solaris-amd64-oraclerel builder, because a +NN:NN offset time zone is used. Allow for the `+` character in the corresponding regex to fix these tests. Also move the '-' to the end of the character class, so it is not interpreted as the range 9-T. Updates #38458. For #39308. Change-Id: Iec9ae82ba45d2490176f274f0dc6812666eae718 Reviewed-on: https://go-review.googlesource.com/c/go/+/234978 Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-on: https://go-review.googlesource.com/c/go/+/242059 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
… FAIL lines This fixes an issue raised at #38458 (comment) in which --- PASS and --- FAIL lines would not trigger --- CONT lines of other tests. Updates #38458. For #39308. Change-Id: I0d8cc54d682a370d0a6ea6816a11b2e462a92efe Reviewed-on: https://go-review.googlesource.com/c/go/+/235997 Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-on: https://go-review.googlesource.com/c/go/+/242058 Reviewed-by: Jean de Klerk <deklerk@google.com> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Replaces #7559 Running tests in parallel, with background goroutines, results in test output not being associated with the correct test. `go test` does not make any guarantees about output from goroutines being attributed to the correct test case. Attaching log output from background goroutines also cause data races. If the goroutine outlives the test, it will race with the test being marked done. Previously this was noticed as a panic when logging, but with the race detector enabled it is shown as a data race. The previous solution did not address the problem of correct test attribution because test output could still be hidden when it was associated with a test that did not fail. You would have to look at all of the log output to find the relevant lines. It also made debugging test failures more difficult because each log line was very long. This commit attempts a new approach. Instead of printing all the logs, only print when a test fails. This should work well when there are a small number of failures, but may not work well when there are many test failures at the same time. In those cases the failures are unlikely a result of a specific test, and the log output is likely less useful. All of the logs are printed from the test goroutine, so they should be associated with the correct test. Also removes some test helpers that were not used, or only had a single caller. Packages which expose many functions with similar names can be difficult to use correctly. Related: golang/go#38458 (may be fixed in go1.15) golang/go#38382 (comment)
|
Change https://golang.org/cl/249026 mentions this issue: |
While debugging #40771, I realized that the chatty printer should only ever print to a single io.Writer (normally os.Stdout). The other Writer implementations in the chain write to local buffers, but if we wrote a test's output to a local buffer, then we did *not* write it to stdout and we should not store it as the most recently logged test. Because the chatty printer should only ever print to one place, it shouldn't receive an io.Writer as an argument — rather, it shouldn't be used at all for destinations other than the main output stream. On the other hand, when we flush the output buffer to stdout in the top-level flushToParent call, it is important that we not allow some other test's output to intrude between the test summary header and the remainder of the test's output. cmd/test2json doesn't know how to parse such an intrusion, and it's confusing to humans too. No test because I couldn't reproduce the user-reported error without modifying the testing package. (This behavior seems to be very sensitive to output size and/or goroutine scheduling.) Fixes #40771 Updates #38458 Change-Id: Ic19bf1d535672b096ba1c8583a3b74aab6d6d766 Reviewed-on: https://go-review.googlesource.com/c/go/+/249026 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
|
Change https://golang.org/cl/252637 mentions this issue: |
|
Change https://golang.org/cl/252638 mentions this issue: |
…cally when streaming output While debugging #40771, I realized that the chatty printer should only ever print to a single io.Writer (normally os.Stdout). The other Writer implementations in the chain write to local buffers, but if we wrote a test's output to a local buffer, then we did *not* write it to stdout and we should not store it as the most recently logged test. Because the chatty printer should only ever print to one place, it shouldn't receive an io.Writer as an argument — rather, it shouldn't be used at all for destinations other than the main output stream. On the other hand, when we flush the output buffer to stdout in the top-level flushToParent call, it is important that we not allow some other test's output to intrude between the test summary header and the remainder of the test's output. cmd/test2json doesn't know how to parse such an intrusion, and it's confusing to humans too. No test because I couldn't reproduce the user-reported error without modifying the testing package. (This behavior seems to be very sensitive to output size and/or goroutine scheduling.) Fixes #40880 Updates #40771 Updates #38458 Change-Id: Ic19bf1d535672b096ba1c8583a3b74aab6d6d766 Reviewed-on: https://go-review.googlesource.com/c/go/+/249026 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> (cherry picked from commit 51c0bdc) Reviewed-on: https://go-review.googlesource.com/c/go/+/252638 TryBot-Result: Go Bot <gobot@golang.org> Trust: Bryan C. Mills <bcmills@google.com>
…cally when streaming output While debugging #40771, I realized that the chatty printer should only ever print to a single io.Writer (normally os.Stdout). The other Writer implementations in the chain write to local buffers, but if we wrote a test's output to a local buffer, then we did *not* write it to stdout and we should not store it as the most recently logged test. Because the chatty printer should only ever print to one place, it shouldn't receive an io.Writer as an argument — rather, it shouldn't be used at all for destinations other than the main output stream. On the other hand, when we flush the output buffer to stdout in the top-level flushToParent call, it is important that we not allow some other test's output to intrude between the test summary header and the remainder of the test's output. cmd/test2json doesn't know how to parse such an intrusion, and it's confusing to humans too. No test because I couldn't reproduce the user-reported error without modifying the testing package. (This behavior seems to be very sensitive to output size and/or goroutine scheduling.) Fixes #40881 Updates #40771 Updates #38458 Change-Id: Ic19bf1d535672b096ba1c8583a3b74aab6d6d766 Reviewed-on: https://go-review.googlesource.com/c/go/+/249026 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> (cherry picked from commit 51c0bdc) Reviewed-on: https://go-review.googlesource.com/c/go/+/252637 TryBot-Result: Go Bot <gobot@golang.org> Trust: Bryan C. Mills <bcmills@google.com>
…cally when streaming output While debugging golang#40771, I realized that the chatty printer should only ever print to a single io.Writer (normally os.Stdout). The other Writer implementations in the chain write to local buffers, but if we wrote a test's output to a local buffer, then we did *not* write it to stdout and we should not store it as the most recently logged test. Because the chatty printer should only ever print to one place, it shouldn't receive an io.Writer as an argument — rather, it shouldn't be used at all for destinations other than the main output stream. On the other hand, when we flush the output buffer to stdout in the top-level flushToParent call, it is important that we not allow some other test's output to intrude between the test summary header and the remainder of the test's output. cmd/test2json doesn't know how to parse such an intrusion, and it's confusing to humans too. No test because I couldn't reproduce the user-reported error without modifying the testing package. (This behavior seems to be very sensitive to output size and/or goroutine scheduling.) Fixes golang#40881 Updates golang#40771 Updates golang#38458 Change-Id: Ic19bf1d535672b096ba1c8583a3b74aab6d6d766 Reviewed-on: https://go-review.googlesource.com/c/go/+/249026 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> (cherry picked from commit 51c0bdc) Reviewed-on: https://go-review.googlesource.com/c/go/+/252637 TryBot-Result: Go Bot <gobot@golang.org> Trust: Bryan C. Mills <bcmills@google.com>
Normal
go testprints output like this for parallel subtests:My memory is that we made a change to try to print output earlier for
-v, but this is completely breaking the output for parallel subtests. There is no obvious way to tell which test printed:In this case, the subtest name happens to be echoed in the message, but that's because of a print in the test; the testing framework has failed at its job and omitted the information.
In contrast, here is Go 1.12:
And here is current go with the
t.Parallelcall commented out:Rolling back the change for parallel subtests would be unfortunate, since it would make the output placement dependent on use of
t.Parallel. (It's already unfortunate that the output placement is dependent on-vbut that ship has sailed.)Perhaps the best fix would be to print a new
=== CONTline each time the test generating output changes (before the output).The text was updated successfully, but these errors were encountered: