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

test: add JSON output support to the run.go program #56844

Closed
dmitshur opened this issue Nov 18, 2022 · 11 comments
Closed

test: add JSON output support to the run.go program #56844

dmitshur opened this issue Nov 18, 2022 · 11 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

This is a tracking issue for adding JSON output support to the test/run.go program, which is invoked by cmd/dist to run tests in the GOROOT/test directory. This is a subset of #37486, broken out into a smaller tracking issue.

This can ideally be implemented by refactoring this program into a normal Go test that can be invoked with go test (which will then automatically have support JSON output via go test's -json flag), or possibly adding a lightweight Go test on top of the existing test/run.go program. It is in principle also possible to add JSON output support to test/run.go directly, without making it a normal Go test. These are implementation details, need to investigate what approach works out better.

CC @aclements, @golang/release.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 18, 2022
@dmitshur dmitshur added this to the Backlog milestone Nov 18, 2022
@aclements
Copy link
Member

One complication to this I just discovered is that some of the misc/cgo tests (at least misc/cgo/stdio and misc/cgo/life) actually use the run.go runner. We'll need to rewrite those to not use run.go either by 1) just doing the tests directly because they're really not complicated or 2) exporting the functionality of run.go so tests can use it directly. For other tests, having some of run.go's functionality would actually be really nice, but I don't think it's necessary for these tests.

@dmitshur dmitshur self-assigned this Dec 7, 2022
@dmitshur
Copy link
Contributor Author

dmitshur commented Jan 20, 2023

I've started working on this more actively recently and made enough progress to post an update. I think I have a good idea for the general implementation plan here: as expected, it'll be to migrate to using go test for running these test cases, though leave test cases in GOROOT/test as they are. Details follow.

The test/run.go runner handles tests, one for each .go file in the directories listed here (relative to GOROOT/test):

go/test/run.go

Line 103 in 27b6ace

dirs = []string{".", "ken", "chan", "interface", "syntax", "dwarf", "fixedbugs", "codegen", "runtime", "abi", "typeparam", "typeparam/mdempsky"}

For example, using a recent tip commit, there's a total of 2424 test cases:

~/gotip/test $ go run run.go -summary
 2424 ok  

(That number is before any skips are applied due to build constraints or other factors, so it doesn't depend on the GOOS/GOARCH you're trying it on.)

For each of those test cases, I plan to make a subtest in a new internal Go package. For prototyping I've used internal/issue56844 as a temporary placeholder, and so far my best idea for the real thing is probably internal/testdir. We'll see. (Edit: Added cmd/ prefix in #60059.) Assuming that's used, the diff to test/README.md would be something like:

 The test directory contains tests of the Go tool chain and runtime.
 It includes black box tests, regression tests, and error output tests.
 They are run as part of all.bash.
 
 To run just these tests, execute:
 
-    ../bin/go run run.go
+    ../bin/go test internal/testdir
 
 To run just tests from specified files in this directory, execute:
 
-    ../bin/go run run.go -- file1.go file2.go ...
+    ../bin/go test internal/testdir -run='Test/(file1.go|file2.go|...)'
 
 Standard library tests should be written as regular Go tests in the appropriate package.
 
 [...]

I've done some basic timing, and by using the aforementioned subtest arrangement with testing.T.Parallel, all 2424 test cases run in approximately 60-80 seconds on my machine, both before and after. So no regression in test timing. (And, repeated test runs benefit from go test's test caching!)

It's likely important to keep -shard and -shards flags operational for purposes of sharded test execution, but that turns up to be trivial. There are more flags in run.go, and I plan to keep most of them as they are. A few will go away because they're obsolete (e.g., -n is obsolete given go test's own parallelism controls; -showSkips will likely get replaced by t.Skip, etc.).

I haven't yet decided how I'll deal with misc/cgo/stdio and misc/cgo/life, but I'm aiming to just simplify them.

Given that GOROOT/test/run.go is fairly large (2139 lines), I'm looking to do this in a way that makes it easier to review and be confident we're not dropping some tests on the floor. At least one of the CLs will be large to do migration atomically, but fortunately I should be able to make the diff between GOROOT/test/run.go and GOROOT/src/internal/testdir/main_test.go quite small and readable in it. The rest can be split off into smaller preparatory and clean up changes.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 20, 2023
@gopherbot
Copy link

Change https://go.dev/cl/463276 mentions this issue: cmd/dist, test: convert test/run.go runner to a cmd/go test

@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 6, 2023

A few more things for me not to forget to update:

  • in main repo: test/codegen directory README has instructions for running all codegen tests beyond that what all.bash runs on linux-amd64 builder; the command shown ../bin/go run run.go needs to be update to the new one (CL 463276)
  • in x/website: the go.dev/doc/contribute contribution guide describes the top-level test suite in GOROOT/test and shows the command to run, it'll need to be updated (CL 471895)
  • in x/build: x/build/internal/logparser/parse.go has some special handling of the GOROOT/test portion of output from all.bash; I think it's used by watchflakes and may need some attention afterwards

@gopherbot
Copy link

Change https://go.dev/cl/466155 mentions this issue: internal/testdir: simplify and clean up

@gopherbot
Copy link

Change https://go.dev/cl/465755 mentions this issue: misc/cgo/{life,stdio}: remove reliance on test/run.go

@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Feb 7, 2023
gopherbot pushed a commit that referenced this issue Feb 28, 2023
The misc/cgo/life and misc/cgo/stdio tests started out as fairly simple
test cases when they were added, but the machinery to execute them has
grown in complexity over the years.

They currently reuse the test/run.go runner and its "run" action without
needing much of the additional flexibility that said runner implements.
Given that runner isn't well documented, it makes it harder to see that
ultimately these tests just do 'go run' on a few test programs and check
that the output matches a golden file.

Maybe these test cases should move out of misc to be near similar tests,
or the machinery to execute them can made available in a package that is
easier and safer to reuse. I'd rather not block the refactor of the test
directory runner on that, so for now rewrite these to be self-contained.

Also delete misc/cgo/stdio/testdata/run.out which has no effect on the
test. It was seemingly accidentally kept behind during the refactor in
CL 6220049.

For #56844.

Change-Id: I5e2f542824925092cdddb03b44b6295a4136ccb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/465755
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Feb 28, 2023
Now that the bare minimum change to make the run.go test runner into
a normal go test is done, there remain many opportunities to simplify,
modernize and generally clean up the test code.

Of all the opportunities available, this change tries to fit somewhere
between doing "not enough" and "way too much". This ends up including:

• replace verbose flag with testing.Verbose()
• replace custom temporary directory creation, removal, -keep flag
  with testing.T.TempDir
• replace custom code to find the go command with testenv.GoToolPath
• replace many instances of "t.err = err; return" with "return err",
  or with t.Fatal when it's clearly a test infrastructure error
• replace reliance on changing working directory to GOROOT/test to
  computing and using absolute paths
• replace uses of log.Fatal with t.Fatal
• replace uses of deprecated ioutil package with their replacements
• add some missing error checks, use more idiomatic identifier names

For #56844.

Change-Id: I5b301bb83a8e5b64cf211d7f2f4b14d38d48fea0
Reviewed-on: https://go-review.googlesource.com/c/go/+/466155
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/471895 mentions this issue: _content/doc: update GOROOT/test invocation in contribution guide

gopherbot pushed a commit to golang/website that referenced this issue Feb 28, 2023
The top-level test suite in $GOROOT/test is implemented as a normal
go test in the new internal/testenv package as of CL 463276. Update
the contribution guide accordingly.

Updates golang/go#56844.

Change-Id: I73bfa8e4fc7c35f63efdde42ddf3552b5a518136
Reviewed-on: https://go-review.googlesource.com/c/website/+/471895
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/493876 mentions this issue: internal/testdir: move to cmd/internal/testdir

@gopherbot
Copy link

Change https://go.dev/cl/493915 mentions this issue: _content/doc: update testdir import path in contribution guide

@gopherbot
Copy link

Change https://go.dev/cl/494656 mentions this issue: cmd/dist: use registerStdTestSpecially for normal Go tests only

gopherbot pushed a commit to golang/website that referenced this issue May 12, 2023
It's changed slightly to be inside 'cmd'.

For golang/go#56844.
For golang/go#60059.

Change-Id: I244f0ae627978a7b59d6a56d20aebc3ff81b3179
Reviewed-on: https://go-review.googlesource.com/c/website/+/493915
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue May 12, 2023
The effect and motivation is for the test to be selected when doing
'go test cmd' and not when doing 'go test std' since it's primarily
about testing the Go compiler and linker. Other than that, it's run
by all.bash and 'go test std cmd' as before.

For #56844.
Fixes #60059.

Change-Id: I2d499af013f9d9b8761fdf4573f8d27d80c1fccf
Reviewed-on: https://go-review.googlesource.com/c/go/+/493876
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue May 12, 2023
It was my oversight in CL 463276 to skip registerStdTestSpecially
packages in the race bench test registration loop. Package testdir
has no benchmarks and doesn't need to be skipped. (And if it had
benchmarks, it's very unlikely they'd need any special handling.)

By now there are more cmd/cgo/internal/... packages that are registered
specially, and there isn't a need for their few benchmarks not to be
used for the purpose of race bench tests. If the 3 benchmarks in
cmd/cgo/internal/test were to require something special, then we can
add it to a new registerRaceBenchTestSpecially map with a comment, and
do register them specially in registerTests instead of forgetting to.

This restores the automatic 'go_test_bench:cmd/cgo/internal/test'
registration and reduces prevalence of registerStdTestSpecially a bit.

For #37486.
For #56844.

Change-Id: I1791fe5bf94cb4b4e0859c5fff4e7a3d5a23723e
Reviewed-on: https://go-review.googlesource.com/c/go/+/494656
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/498271 mentions this issue: cmd/internal/testdir: stop manually adding GOROOT/bin to PATH

gopherbot pushed a commit that referenced this issue May 26, 2023
The go command already places $GOROOT/bin at the beginning of $PATH in
the test's environment as of Go 1.19¹, so there's no need for the test
to do it anymore. Start enjoying yet another benefit of using 'go test'.

¹ See go.dev/issue/57050.

For #56844.

Change-Id: If7732cd8b8979eabf185485d3c73858a4e546d69
Reviewed-on: https://go-review.googlesource.com/c/go/+/498271
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Nasfame pushed a commit to golangFame/go that referenced this issue Jun 4, 2023
The go command already places $GOROOT/bin at the beginning of $PATH in
the test's environment as of Go 1.19¹, so there's no need for the test
to do it anymore. Start enjoying yet another benefit of using 'go test'.

¹ See go.dev/issue/57050.

For golang#56844.

Change-Id: If7732cd8b8979eabf185485d3c73858a4e546d69
Reviewed-on: https://go-review.googlesource.com/c/go/+/498271
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
Archived in project
Development

No branches or pull requests

3 participants