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.

Sign up for GitHub

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

Jump to bottom

testing: summary and test output interleaved [1.15 backport] #40881

Closed
gopherbot opened this issue Aug 19, 2020 · 4 comments
Closed

testing: summary and test output interleaved [1.15 backport] #40881

gopherbot opened this issue Aug 19, 2020 · 4 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Milestone
Go1.15.3

Comments

@gopherbot
Copy link
Contributor

@bcmills requested issue #40771 to be considered for backport to the next 1.15 minor release.

@gopherbot, please backport to Go 1.14 and 1.15. This appears to be a regression in the 1.14.6 point release, and there is no apparent workaround for users who require JSON-formatted test output.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Aug 19, 2020
@gopherbot gopherbot mentioned this issue Aug 19, 2020
Closed
@gopherbot gopherbot added this to the Go1.15.1 milestone Aug 19, 2020
@dmitshur dmitshur modified the milestones: Go1.15.1, Go1.15.2 Sep 1, 2020
@gopherbot
Copy link
Contributor Author

Change https://golang.org/cl/252637 mentions this issue: [release-branch.go1.15] testing: flush test summaries to stdout atomically when streaming output

@dmitshur dmitshur modified the milestones: Go1.15.2, Go1.15.3 Sep 9, 2020
@bcmills
Copy link
Contributor

bcmills commented Sep 22, 2020

Discussion on #40771 (comment) confirms that the change in question does seem to fix the reported issue, and we haven't had any reports of it regressing any other behavior on the main branch during the 1.16 cycle. I think we should aim to get this into the next minor release.

@cagedmantis
Copy link
Contributor

This has been approved because it's a serious issue without a workaround.

@cagedmantis cagedmantis added the CherryPickApproved Used during the release process for point releases label Sep 24, 2020
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Sep 24, 2020
@gopherbot
Copy link
Contributor Author

Closed by merging c25dd3f to release-branch.go1.15.

gopherbot pushed a commit that referenced this issue Oct 5, 2020
@dmitshur
[release-branch.go1.15] testing: flush test summaries to stdout atomi…
c25dd3f 
…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>
@chizhg chizhg mentioned this issue Oct 15, 2020
Merged
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
@claucece
[release-branch.go1.15] testing: flush test summaries to stdout atomi…
22571e5 
…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>
@golang golang locked and limited conversation to collaborators Oct 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Projects
None yet
Milestone
Go1.15.3
Development

No branches or pull requests
4 participants
@cagedmantis @dmitshur @bcmills @gopherbot