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: streaming output loses parallel subtest associations [1.14 backport] #39308

Closed
gopherbot opened this issue May 28, 2020 · 10 comments
Closed

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented May 28, 2020

@bcmills requested issue #38458 to be considered for backport to the next 1.14 minor release.

@gopherbot, please backport to Go 1.14: the 1.14 output format breaks go test -json for parallel subtests, and backporting the fix to the output format is likely to be more maintainable than a one-off test2json fix that is only used for the 1.14 branch.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jun 4, 2020

@bcmills Do I understand correctly that backporting this to 1.14 will improve readability of the test output for humans, and fix a bug in go test -json (used by machines), so the behavior change from backporting it should be safe for a minor release?

Does the fix involve backporting both CL 229085 and CL 235997?

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 4, 2020

Yes, your understanding of the behavior change is correct.

I would recommend backporting both changes, although only the first one is strictly necessary to fix the bug. You'll also want to pull in CL 234978 either way.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jun 5, 2020

Thanks.

We've discussed this in a release meeting yesterday, and while it's unfortunate to be making a change, it is fixing a serious issue without a workaround. Approving for 1.14 (this issue doesn't affect 1.13).

@mdwhatcott
Copy link

@mdwhatcott mdwhatcott commented Jun 11, 2020

@bcmills - Is this on track for 1.14.5?

@medyagh
Copy link

@medyagh medyagh commented Jun 16, 2020

in minikube CI we have a bug (kubernetes/minikube#8349) that depends on fixing this issue
more info here
#33419 (comment)

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jul 10, 2020

Change https://golang.org/cl/242058 mentions this issue: [release-branch.go1.14] testing: capture testname on --- PASS and --- FAIL lines

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jul 10, 2020

Change https://golang.org/cl/242057 mentions this issue: [release-branch.go1.14] testing: reformat test chatty output

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jul 10, 2020

Change https://golang.org/cl/242059 mentions this issue: [release-branch.go1.14] cmd/go: fix parallel chatty tests on solaris-amd64 builder

@andybons andybons modified the milestones: Go1.14.5, Go1.14.6 Jul 14, 2020
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jul 16, 2020

Closed by merging 2ba9d45 to release-branch.go1.14.

@gopherbot gopherbot closed this Jul 16, 2020
gopherbot pushed a commit that referenced this issue Jul 16, 2020
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>
gopherbot pushed a commit that referenced this issue Jul 16, 2020
…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>
gopherbot pushed a commit that referenced this issue Jul 16, 2020
… 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>
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jul 16, 2020

Now that this change was submitted to the release branch, I'll remove release-blocker label to reduce visual noise when viewing the release history details.

dnephin added a commit to hashicorp/consul that referenced this issue Jul 17, 2020
Includes the security patches from go14.5 and golang/go#39308
to fix our test logs.
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