cmd/dist: add json output support #37486
Comments
|
Tentatively marking for 1.15, but it's unclear what's involved and there's not much time in the dev cycle so it may slip. |
andybons
added
the
NeedsInvestigation
|
Thanks for filing this Andrew! We haven't covered ground on this during Go1.15, so I shall move this issue to Go1.16. |
|
I've partially implemented
I'm new to the Go codebase, so I'm not sure how much coverage these tests represent. For me, What I've done so far is super simple: dagood@1fa7f46. Does this kind of partial implementation seem acceptable into |
|
Change https://golang.org/cl/360865 mentions this issue: |
|
I guess this needs to go through the proposal process @rsc |
|
I've been slowly putting together a proposal for this, which I'm hoping to get out pretty soon. There are a surprisingly large number of subtleties to actually implementing it, so I've been prototyping as I design in order to suss these out. @dagood had a good overview of some of them above. |
|
Change https://go.dev/cl/431257 mentions this issue: |
|
Change https://go.dev/cl/431259 mentions this issue: |
|
Change https://go.dev/cl/431258 mentions this issue: |
|
Change https://go.dev/cl/431256 mentions this issue: |
We haven't used this in a while and it's going to complicate later changes to dist, so drop support. This was primarily for supporting slow QEMU-based builders, but an alternative and simpler way to do that if we need to in the future is to supply a go_exec wrapper to run tests in QEMU, like we do for other emulated platforms. Simplification for #37486. Change-Id: Idc0383f59c61d8546ea3b4d2eede4acdaf30d9b6 Reviewed-on: https://go-review.googlesource.com/c/go/+/431256 TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Austin Clements <austin@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Austin Clements <austin@google.com>
|
Change https://go.dev/cl/443067 mentions this issue: |
|
Change https://go.dev/cl/443069 mentions this issue: |
|
Change https://go.dev/cl/443068 mentions this issue: |
The testasan test was added back in 2013 (CL 10126044), many years before Go added ASAN support in 2021 (CL 298611). So, in fact, testasan does not test Go ASAN support at all, as you might expect (misc/cgo/testsanitizers does that). It's intended to test whether the Go memory allocator works in a mixed C/Go binary where the C code is compiled with ASAN. The test doesn't actually use ASAN in any way; it just simulates where ASAN of 2013 put its shadow mappings. This made sense to test at the time because Go was picky about where its heap landed and ASAN happened to put its mappings exactly where Go wanted to put its heap. These days, Go is totally flexible about its heap placement, and I wouldn't be surprised if ASAN also works differently. Given all of this, this test adds almost no value today. Drop it. For #37486, since it eliminates a non-go-test from dist. Change-Id: I0292f8efbdc0e1e39650715604535c445fbaa87f Reviewed-on: https://go-review.googlesource.com/c/go/+/443067 Reviewed-by: Ian Lance Taylor <iant@google.com> Auto-Submit: Austin Clements <austin@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
|
Change https://go.dev/cl/443336 mentions this issue: |
This code was introduced in CL 17903 but has never executed. It's also fundamentally non-deterministic. Delete it. Simplification for #37486. Change-Id: I049564123fb4fba401154e2ea0fc429e552d4749 Reviewed-on: https://go-review.googlesource.com/c/go/+/431258 Reviewed-by: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com>
This migrates testsigfwd, which uses some one-off build infrastructure, to be part of the runtime's testprogcgo. The test is largely unchanged. Because it's part of a larger binary, this CL renames a few things and gates the constructor-time signal handler registration on an environment variable. This CL also replaces an errant fmt.Errorf with fmt.Fprintf. For #37486, since it eliminates a non-go-test from dist. Change-Id: I0efd146ea0a0a3f0b361431349a419af0f0ecc61 Reviewed-on: https://go-review.googlesource.com/c/go/+/443068 Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Currently, the entry-point to this test is a Bash script that smoke tests the FORTRAN compiler and then runs a FORTRAN-containing Go test. This CL rearranges things so a pure Go Go test smoke tests the FORTRAN compiler and then runs a non-test FORTRAN-containing Go binary. While we're here, we fix a discrepancy when the host is GOARCH=amd64, but the target is GOARCH=386. Previously, we would pick the wrong libgfortran path because we didn't account for the cross-compilation, causing the link to fail. Except for some reason this was ignored and the test nevertheless "passed". In the new test we're a little more strict, so this build failure will cause the test to fail, so we add a little logic to account for cross-compilation with the host toolchain. For #37486. Change-Id: Ie6f70066885d6fbb4e1b5a2b1e13b85dee5b359b Reviewed-on: https://go-review.googlesource.com/c/go/+/443069 Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Austin Clements <austin@google.com>
The go1 benchmark suite does a lot of work at package init time, which makes it take quite a while to run even if you're not running any of the benchmarks, or if you're only running a subset of them. This leads to an awkward workaround in dist test to compile but not run the package, unlike roughly all other packages. It also reduces isolation between benchmarks by affecting the starting heap size of all benchmarks. Fix this by initializing all data required by a benchmark when that benchmark runs, and keeping it local so it gets freed by the GC and doesn't leak between benchmarks. Now, none of the benchmarks depend on global state. Re-initializing the data on each benchmark run does add overhead to an actual benchmark run, as each benchmark function is called several times with different values of b.N. A full run of all benchmarks at the default -benchtime=1s now takes ~10% longer; higher -benchtimes would be less. It would be quite difficult to cache this data between invocations of the same benchmark function without leaking between different benchmarks and affecting GC overheads, as the testing package doesn't provide any mechanism for this. This reduces the time to run the binary with no benchmarks from 1.5 seconds to 10 ms, and also reduces the memory required to do this from 342 MiB to 17 MiB. To make sure data was not leaking between different benchmarks, I ran the benchmarks with -shuffle=on. The variance remained low: mostly under 3%. A few benchmarks had higher variance, but in all cases it was similar to the variance between this change. This CL naturally changes the measured performance of several of the benchmarks because it dramatically changes the heap size and hence GC overheads. However, going forward the benchmarks should be much better isolated. For #37486. Change-Id: I252ebea703a9560706cc1990dc5ad22d1927c7a0 Reviewed-on: https://go-review.googlesource.com/c/go/+/443336 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Pratt <mpratt@google.com> Run-TryBot: Austin Clements <austin@google.com>
|
Change https://go.dev/cl/447355 mentions this issue: |
|
Change https://go.dev/cl/450017 mentions this issue: |
|
Change https://go.dev/cl/450019 mentions this issue: |
|
Change https://go.dev/cl/450018 mentions this issue: |
|
Change https://go.dev/cl/450020 mentions this issue: |
The cmd/dist cgo_test enumerates a large number of platforms in various special cases. Some combinations are suspiciously absent. This CL completes the combinations. I've confirmed using trybots that the newly-enabled tests pass on android/* (this is not surprising because the gohostos is never "android" anyway), windows/arm64, linux/ppc64 (no cgo), linux/loong64 (except for one test, filed #56623), linux/mips*, netbsd/arm (except for one test, filed #56629), and netbsd/arm64. The windows/arm builder is out to lunch, so I'm assuming that works. Since netbsd/arm and arm64 mostly passed these tests, I've also enabled them on netbsd/386 and netbsd/amd64, where they seem to work fine as well. Preparation for #37486. Change-Id: I04c3348e4f422d74d51e714647ca3db379e6e919 Reviewed-on: https://go-review.googlesource.com/c/go/+/448016 Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Austin Clements <austin@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
This is unused, and eliminating it lets us simplify the whole registerTest mechanism. Preparation for #37486. Change-Id: Ia6221e48192cd17775a5d662bdb389d67a9265bc Reviewed-on: https://go-review.googlesource.com/c/go/+/448800 Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
The registerTest function has a special case for commands that start with "time", but we don't use this case anywhere. Delete this special case and its support code. Preparation for #37486. Change-Id: Ica180417e7aa4e4fc260cb97467942bae972fdb6 Reviewed-on: https://go-review.googlesource.com/c/go/+/448801 Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
We're going to use this for another test, so make it more accessible. Preparation for #37486. Change-Id: If194cc4244c4b9e1b1f253759b813555b39ad67e Reviewed-on: https://go-review.googlesource.com/c/go/+/449502 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Currently, cmd/go's testterminal18153 is implemented as a special test that doesn't run as part of cmd/go's regular tests. Because the test requires stdout and stderr to be a terminal, it is currently run directly by "dist test" so it can inherit the terminal of all.bash. This has a few problems. It's yet another special case in dist test. dist test also has to be careful to not apply its own buffering to this test, so it can't run in parallel and it limits dist test's own scheduler. It doesn't run as part of regular "go test", which means it usually only gets coverage from running all.bash. And since we have to skip it if all.bash wasn't run at a terminal, I'm sure it often gets skipped even when running all.bash. Fix all of this by rewriting this test to create its own PTY and re-exec "go test" to check that PTY passes through go test. This makes the test self-contained, so it can be a regular cmd/go test, we can drop it from dist test, and it's not sensitive to the environment of all.bash. Preparation for #37486. Updates #18153. Change-Id: I6493dbb0143348e299718f6e311ac8a63f5d69c5 Reviewed-on: https://go-review.googlesource.com/c/go/+/449503 Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
This introduces an abstraction for constructing and running "go test" commands. Currently, dist test is basically a shell script written in Go syntax: it mostly just invokes lots of subprocesses, almost all of which are "go test" invocations, and it constructs those command lines directly from strings all over the place. This CL raises the level of abstraction of invoking go test. The current level of abstraction is not serving us very well: it's conveniently terse, but the actual logic for constructing a command line is typically so spread out that it's difficult to predict what command will actually run. For example, the `gotest` function constructs the basic command, but many tests want to override at least some of these flags, so flattenCmdLine has logic specific to `go test` for eliminating duplicate flags that `go test` itself would reject. At the same time, the logic for constructing many common flags is conditional, leading to a bevy of helpers for constructing flags like `-short` and `-timeout` and `-run` that are scattered throughout test.go and very easy to forget to call. This CL centralizes and flattens all of this knowledge into a new `goTest` type. This type gives dist a single, unified point where we can change anything about how it invokes "go test". There's currently some "unnecessary" abstraction in the implementation of the goTest type to separate "build" and "run" flags. This will become important later when we convert host tests and to do separate build and run steps. The following CLs will convert dist test to use this type rather than directly constructing "go test" command lines. Finally, we'll strip out the scattered helper logic for building command lines. For #37486. Change-Id: I9f1633fe6c0921696419ce8127ed2ca7b7a4e01b Reviewed-on: https://go-review.googlesource.com/c/go/+/448802 Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
This CL rewrites everywhere in dist that manually constructs an exec.Cmd to run "go test" to use the goTest abstraction. All remaining invocations of "go test" after this CL construct the command line manually, but ultimately use addCmd to execute it. I traced all exec calls from cmd/dist on linux/amd64 and this makes only no-op changes (such as re-arranging the order of flags). For #37486. Change-Id: Idc7497e39bac04def7ddaf2010881c9623e76fd4 Reviewed-on: https://go-review.googlesource.com/c/go/+/450015 Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Run-TryBot: Austin Clements <austin@google.com>
The overall goal is to make registerTest the primary entry point for adding dist tests and to convert nearly all dist tests to be represented by a goTest, registered via registerTest. This will centralize the logic for creating dist tests corresponding to go tool tests. I traced all exec calls from cmd/dist on linux/amd64 and this makes only no-op changes (such as re-arranging the order of flags). For #37486. Change-Id: I4749e6f3666134d3259b54ee6055d76a4235c60c Reviewed-on: https://go-review.googlesource.com/c/go/+/450016 Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
Currently, dist test has a single test called "cgo_test" that runs a large number of different "go test"s. This commit restructures cgo_test into several individual tests, each of which runs a single "go test" that can be described by a goTest object and registered with registerTest. Since this lets us raise the abstraction level of constructing these tests and these tests are mostly covering the Cartesian product of a small number of orthogonal dimensions, we pull the common logic for constructing these tests into a helper function. For consistency, we now pass -tags=static to the static testtls and nocgo tests, but this tag doesn't affect the build of these tests at all. I traced all exec calls from cmd/dist on linux/amd64 and this is the only non-trivial change. For #37486. Change-Id: I53c1efa1c38d785dc71968f05e8d7d636b553e96 Reviewed-on: https://go-review.googlesource.com/c/go/+/450017 Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
This converts most of the remaining manual "go test" command line construction in cmd/dist to use the goTest abstraction and registerTest. At this point, the only remaining place that directly constructs go test command lines is runHostTest. This fixes a bug in the "nolibgcc:os/user" test. It was clearly supposed to pass "-run=^Test[^CS]", but the logic to override the "-run" flag for "nolibgcc:net" caused "nolibgcc:os/user" to pass *both* "-run=^Test[^CS]" and "-run=". This was then rewritten into just "-run=" by flattenCmdline, which caused all os/user tests to run, and not actually skip the expensive tests as intended. (This is a great example of why the new abstraction is much more robust than command line construction.) I traced all exec calls from cmd/dist on linux/amd64 and, other than the fix to nolibgcc:os/user, this makes only no-op changes (such as re-arranging the order of flags). For #37486. Change-Id: Ie8546bacc56640ea39f2804a87795c14a3fe4c7d Reviewed-on: https://go-review.googlesource.com/c/go/+/450018 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
This adds support for host tests to goTest and registerTest and modifies all uses of registerHostTest to use goTest and registerTest. This eliminates the last case where go test command lines are constructed by hand. Next we'll clean up all of the infrastructure support for that. I traced all exec calls from cmd/dist on linux/amd64 and this makes only no-op changes (such as re-arranging the order of flags). Preparation for #37486. Change-Id: Icb7ec8efdac72bdb819ae24b2f585375d9d9d5b9 Reviewed-on: https://go-review.googlesource.com/c/go/+/450019 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Run-TryBot: Austin Clements <austin@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Now that all uses of "go test" have been converted over to the new abstraction, we can delete the old helpers for building "go test" commands and simplify some code that's only used by the new abstraction now. For #37486. Change-Id: I770cd457e018160d694abcc0b6ac80f7dc2e8425 Reviewed-on: https://go-review.googlesource.com/c/go/+/450020 Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Austin Clements <austin@google.com>
|
Change https://go.dev/cl/451437 mentions this issue: |
In -race mode, the dist test command only registers the std, race, osusergo, and amd64ios tests before returning early from (*tester).registerTests. Prior to CL 450018, the osusergo and amd64ios tests weren't even affected by -race mode, so it seems their inclusion was unintentional. CL 450018 lifted the logic to run tests in race mode, which means these tests went from running without -race to running with -race. Unfortunately, amd64ios is not compatible with -race, so it is now failing on the darwin-amd64-race builder. Fix this by omitting the osusergo and amd64ios tests from -race mode, since it seems like they were really intended to be included anyway. This should fix the darwin-amd64-race builder. Updates #37486. Change-Id: I554bb60bc729dbb6f1bc926f1ea329768b0d6d81 Reviewed-on: https://go-review.googlesource.com/c/go/+/451437 Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Austin Clements <austin@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Austin Clements <austin@google.com>
|
We're now in a much better place to add JSON output to cmd/dist. As far as I'm aware, the following tasks remain:
In the general theme of cleaning up cmd/dist, I have a new scheduler mostly done that's much simpler and will help with host tests and misc output. Host tests.. just don't make a lot of sense the way they're currently implemented. I think we could eliminate the separate compilation step relatively easily with a little help from the test itself to set GOOS/GOARCH inside the test. There's also a bigger problem that a lot of other tests also build and execute binaries and these currently get no coverage on cross-compiled architectures. That isn't blocking JSON output, but might be possible to solve at the same time. Once dist itself supports JSON output, there's more work to do on the CI side to make use of this. I think we'll probably file a separate tracking issue for that with a top-level task list once we start planning it in more detail. |
|
Here's my plan for host tests. One problem is that dist compiles and runs them separately, which causes a lot of complexity. My plan for this is to make these tests take a For the problem that we skip many other tests that need access to the Go toolchain but aren't currently "host" tests, my plan is to have dist scan for the t.Skip message produced by testenv.MustHaveGoBuild and MustHaveGoRun and to re-run these tests on the host. This may, again, benefit from having an abstraction to invoke the go toolchain. Many of these tests will still need to be skipped because they expect to interact with the subprocess in non-trivial ways, like by sending it signals. Since there's a fair amount of test infrastructure that would be nice to share here, I think we should move the misc tests into std as much as possible. The cgo tests can move to cmd/cgo/internal/test. For the cgo tests that are run in multiple build modes (the "../misc/cgo/test" dist group), I suspect they can run in the default build mode as part of "cmd" and then dist can re-run them in the other build modes as a special case. |
|
Change https://go.dev/cl/463276 mentions this issue: |
As motivated on the issue, we want to move the functionality of the run.go program to happen via a normal go test. Each .go test case in the GOROOT/test directory gets a subtest, and cmd/go's support for parallel test execution replaces run.go's own implementation thereof. The goal of this change is to have fairly minimal and readable diff while making an atomic changeover. The working directory is modified during the test execution to be GOROOT/test as it was with run.go, and most of the test struct and its run method are kept unchanged. The next CL in the stack applies further simplifications and cleanups that become viable. There's no noticeable difference in test execution time: it takes around 60-80 seconds both before and after on my machine. Test caching, which the previous runner lacked, can shorten the time significantly. For #37486. Fixes #56844. Change-Id: I209619dc9d90e7529624e49c01efeadfbeb5c9ae Reviewed-on: https://go-review.googlesource.com/c/go/+/463276 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Austin Clements <austin@google.com> Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
|
Change https://go.dev/cl/478155 mentions this issue: |
|
I used CL 478155 to get a not-fully-complete but fairly representative all.bash run with go-issue-37486-sample.html (115 KB gzip compressed) That isn't meant to be what we'll end up using, just something I quickly put together using <details> HTML elements. (Also filed #59181 for one minor inconsistency I ran into.) It's quite interesting to see which tests passed and which ones were skipped, and a good example of the kinds of things structured output enables. |
This is a top-level issue to add support for JSON output via a
-jsonflag.The immediate use-case is to more easily process output from the builders.
Dependencies:
@bcmills @toothrot
The text was updated successfully, but these errors were encountered: