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

proposal: go/test: add a package to facilitate test tooling #47101

Closed
firelizzard18 opened this issue Jul 9, 2021 · 20 comments
Closed

proposal: go/test: add a package to facilitate test tooling #47101

firelizzard18 opened this issue Jul 9, 2021 · 20 comments

Comments

@firelizzard18
Copy link
Contributor

Developing tooling around go test is painful. To do anything meaningful beyond showing the output of go test in plaintext, you have to parse the output of go test -v. go test -json is an option, but that's doing the same thing - parsing the output of a command that was not designed to be parsed. This is brittle - see #31969, #37555, #24929, #23037, etc.

This is especially painful for benchmarks. For a successful benchmark, test2json never produces lifecycle events - all it produces are output events where the Test field is empty. To turn that into lifecycle events, you have to use a regex or equivalent to find BenchmarkXxx\n and BenchmarkXxx-<maxProcs> <results>\n in the output stream.

I propose a new package, e.g. go/test, with an approximate interface below. The two areas I am personally most interested in are improving test and coverage reporting for CI and IDE integration. With respect to the latter, I am working on a PR to vscode-go that implements the new VSCode test controller API. If this proposal is accepted, this feature could be integrated into gopls to enable much more robust IDE test execution.

package test

import (
	"go/ast"
	"go/build"
)

type Context struct {
	Build *build.Context

	// options
}

type Function struct {
	Kind FunctionKind
	Decl *ast.FuncDecl
}

type FunctionKind int

const (
	TestFunction FunctionKind = iota + 1
	BenchmarkFunction
	ExampleFunction
)

func (*Context) Load(path string, srcDir string) ([]*Function, error)
func (*Context) LoadDir(dir string) ([]*Function, error)

func (*Function) Execute() <-chan Event

type Event struct {
	Time   time.Time
	Kind   EventKind
	Test   *Function
	Output string
}

type EventKind int

const (
	RunEvent EventKind = iota + 1
	PauseEvent
	ContEvent
	PassEvent
	BenchEvent
	FailEvent
	OutputEvent
	SkipEvent
)
@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

It seems like this is something that can and should be done outside the standard library, at least to start.
Is there some reason it needs to be in the standard library?
The API does not seem particularly firm.

@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@firelizzard18
Copy link
Contributor Author

firelizzard18 commented Oct 27, 2021

As I see it, the only options for doing it outside of the standard library involve one of the following:

  • Build a wrapper around go test. This is problematic, as it involves parsing go test output, and that is messy and fragile. For one, a test that prints to stdout without a new line at the end can break go tool test2json.
  • Reinvent the build system. go test is tightly integrated with go build, and pretty much all of that code is either internal or part of package main, and thus almost none of it can be reused.
  • Provide some way to reuse go test's implementation. This is what I am most interested in. Regardless of how it is done, I want some way to write an external tool that reuses go test's build system, without having to rewrite the whole thing, and without having to parse the output from a tool that was not designed to be parsed.

The API proposal above was my attempt at a minimal interface to allow reuse of go test's internals.

@ianlancetaylor
Copy link
Member

If we fix go tool test2json to handle all problem cases, does that address your need?

@firelizzard18
Copy link
Contributor Author

Test events (go tool test2json) is part of it, and that would help. ISTM that it would be simpler to implement that functionality directly in go test, instead of as a separate layer. And ISTM that all of the issues with writes to stdout/stderr could be circumvented entirely by optionally using a fourth file descriptor. Something like go test --event-fd 3 that would write test events to FD 3.

Aside from that, two features would be extremely helpful for IDE integration:

  1. A (sub)command to list all tests by file path and line number, e.g. go test --list.
  2. Additional metadata on the run event, specifically file path and line number.

vscode-go currently identifies tests by using the document symbol provider and looking for functions that match a regex. (1) would eliminate the need to do that. When a test is run and vscode-go sees a run event for a subtest, it adds an entry for that subtest. vscode-go does not currently attempt to identify subtests semantically, so entries for subtests do not include a source location. (2) would allow vscode-go to add a source location for subtests.

I recognize that the only feasible way to provide a file path and line number for a subtest is for *testing.T.Run to look at the stack trace when it is called, and it can be called from anywhere, so some times that won't be meaningful. But *testing.T.Run is usually called directly from within a test, so the file path and line number of the caller will be what we want to know most of the time. My goal is an improved experience, not perfection.

@ianlancetaylor
Copy link
Member

We already have go test -list=., although it just lists the names and it doesn't list subtests (the only way to list subtests is to actually run the test).

The problem with a go/test package is that it locks us into a specific API in an area that we know from experience is subject to change over time. It's clearly preferable to get this information by invoking cmd/go if at all possible. That is, I think we would rather lock down the cmd/go command line API than we would a programmatic API for test information.

@firelizzard18
Copy link
Contributor Author

One of the persistent issues is that go test is not particularly tooling-friendly. In Git-speak, it is far more of a porcelain command than it is a plumbing command. If Go had a plumbing version of go test, whether a separate subcommand or a flag, that would go a long ways towards reducing the difficulty of making tooling.

In other words, the output of go test is formatted in a way that is friendly to users and unfriendly to tooling. go tool test2json does a good job of converting test output to a tool-friendly format, but it has had numerous issues. Besides that, go test effectively completely ignores -json if there is a build failure. And go tool test2json interprets the output of go test -list=. as output events.

If go test was updated to interpret -json as "produce all output in a machine-readable format suitable for tooling", that would solve a lot of my problems. Besides that, I would like to see source locations included in the output of go test -list=. -json and in run events for subtests. I think these changes, plus more robust go tool test2json, would do everything I need.

IIRC, go test creates a test binary and executes it. So, go test could open FD 3 before it fork-execs the test binary, and the test binary could write test events to FD 3. I think that would completely avoid the issue of stdout/stderr writes breaking go tool test2json. Though I'm not sure if that would work on Windows.

@ianlancetaylor
Copy link
Member

We do have to consider the case of go test -c followed by copying the binary to a different machine and executing it there. That is how we routinely do tests for Android and iOS, for example.

@rsc
Copy link
Contributor

rsc commented Nov 3, 2021

go/packages is a nice Go API around the go list command line,
although it also adds significantly more in terms of loading other useful information.
We added it because we had experience showing it was very useful.

It would be fine to write a third-party go/test package outside the standard library and see if that is useful.
I don't think we have evidence that it is needed in the standard library right now.

@firelizzard18
Copy link
Contributor Author

It would be fine to write a third-party go/test package outside the standard library and see if that is useful.
I don't think we have evidence that it is needed in the standard library right now.

Without changes to go test, this is not feasible. Parsing the output of go test is problematic, and given that most of go test's implementation is internal, the only other option is basically rewriting the whole thing.

@ianlancetaylor
Copy link
Member

A key point here is that the output you are concerned about isn't really coming from go test. It's coming from the test binary itself. A test binary can in general do anything. So you don't need to redesign go test. You need to redesign the testing package.

@firelizzard18
Copy link
Contributor Author

One of the issues parsing go test is that a build failure has completely different output. Is that not part of go test?

@ianlancetaylor
Copy link
Member

You're right: a failure building the test is reported by go test. If there are steps we can take to make it easier to distinguish a build failure when using go test, let us know. Or, use go test -c, in which case any failure is a build failure, and if the build didn't fail then run the test binary directly.

@meling
Copy link

meling commented Nov 4, 2021

Just wanted to jump in and give my support for an API for interacting with go test.

My specific use case is auto grading programming assignments that we run in a custom CI environment; I use go test along with my own scoring API to score tests. I recently added a test that compare the results of two benchmarks. Right now, it feels a bit awkward to have to run the benchmarks via exec.Command from within a test and then parse the output using the x/tools/benchmark/parse package to find what I need.

I don't have a clear idea right now what my API needs are, but I will certainly follow this issue and think more about this.

Edit: Another use case for me is that if the student submitted code didn't compile -- then, no test binary runs, then there won't be any score output since it's the same binary the generates the score. I've not tried to fix this yet, just telling students that their code must compile... A wrapper around go test would solve this.

@rsc
Copy link
Contributor

rsc commented Nov 10, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@firelizzard18
Copy link
Contributor Author

@rsc @ianlancetaylor Would a proposal along the lines, "add a --machine-readable-output flag to go test" be more successful?

@ianlancetaylor
Copy link
Member

We already have machine readable output from go test via the -json flag. Adding another option isn't going to help. What is needed is a way to fix the problems that people have with the -json flag. Simply adding a new --machine-readable-output option is going to have exactly the same problems.

@dnephin
Copy link
Contributor

dnephin commented Nov 13, 2021

gotestsum has been consuming the test2json output since it was first added in go1.10. For the most part it has been great, but the bugs mentioned in the OP have definitely been a problem for users. I've been able to work around a few, but not others.

The main bugs seemed to be related to #24929, and tests or sub-tests not being correctly identified as passed or failed, either because of missing newlines or panics. But t.Parallel has also been a problem, and often stdout is not attributed to the correct test when t.Parallel is used. Thankfully the exit code is generally right, but #45508 makes it hard to trust even that.

Something like go test --event-fd 3 that would write test events to FD 3.

I have been wondering if this would address the problems. Since build failures go to stderr (and nothing else goes to stderr) it has been easy to handle them by reading stderr. I have not run into any problems with build errors. If events were emitted as JSON directly to a separate FD that seems like it might address many of the problems we've encountered.

If I understand correctly that would mostly be a change to the testing package, but I guess we still need some way to tell it which FD to use?

@firelizzard18 I don't have any of the requirements that you mentioned for line numbers, but my impression is that sending events to a separate FD might be a prerequisite for making that happen. I guess file names and line numbers can't be added to the existing go test format (the human readable one) because it could be a breaking change. But adding that data to a JSON event would be backwards compatible. So if we had a way of emitting JSON events directly without parsing the standard output format, new keys could be added without changing the standard format.

Would you like to create that proposal? If you don't I will try to propose something. I'm not all that familiar with the internals of go test or testing, so if anyone has thoughts on what to include in the proposal please do share!


Some more context for why I think sending events to another FD will likely fix the problems.

The challenge with t.Parallel and any goroutines started by tests is that they could run at any time, so output gets attributed to the test that happens to be running at that time.

If there were a different FD then at least the run/pass/fail events would be reliable. t.Log output is easy enough to capture and assign to the right test case, but stdout and stderr are still a challenge.

If we could add a testing.T.Stdout() io.Writer and testing.T.Stdout() io.Writer then tests could use those io.Writer to ensure that any output produced by any goroutines started by that test will be attributed to the correct test, regardless of which parallel test happens to be running. Edit: Maybe even just a single io.Writer would be enough.

Any writes directly to os.Stdout or os.Stderr would have to fall back to the existing system of attributing them to the currently running test, which works fine as long as t.Parallel is not used with any background goroutines. These would continue to be on a different FD from the event stream, so the different formats shouldn't be a problem.

@firelizzard18
Copy link
Contributor Author

@dnephin I do intend to make a proposal along those lines, but I'm not sure when I'll have time to work on that. Feel free to do it yourself if you want it to happen soon.

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Dec 1, 2021
@rsc rsc moved this to Declined in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@golang golang locked and limited conversation to collaborators Dec 1, 2022
@rsc rsc removed this from Proposals Dec 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants