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: cmd/go: add -testlabel to print a label on the test package names #60128

Closed
rsc opened this issue May 11, 2023 · 12 comments
Closed

proposal: cmd/go: add -testlabel to print a label on the test package names #60128

rsc opened this issue May 11, 2023 · 12 comments
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented May 11, 2023

When running go test with various different settings, it can be useful to distinguish those results from others in the test output. I propose we add a new go test flag -label=arg that will add :arg to the printed package names in test results, so

% GODEBUG=cpu/sse2=off go test -label=sse2off strings math
ok  	strings:sse2off	0.886s
ok  	math:sse2off	0.138s
% 

This label would also be included in the Package field in go test -json output. This is particularly useful when running a test in different modes in a larger wrapper - it avoids the need to post-process the JSON output to adjust the names to keep different modes separated.

A different option would be to indicate the label some other way, such as an extra JSON field, but currently tooling that understands test2json separates different tests running in parallel by the Package field. If we break that invariant we'd probably want to introduce an ID field that would hold a random identifier chosen for each test2json invocation, to allow disentangling merged streams of JSON output. Given the choice, I think I'd rather just use the :arg syntax in the Package.

/cc @aclements @bcmills

@rsc rsc added this to the Proposal milestone May 11, 2023
@bcmills bcmills added the GoCommand cmd/go label May 11, 2023
@gopherbot
Copy link

Change https://go.dev/cl/494498 mentions this issue: cmd/go/internal/test: add -label flag

@bcmills
Copy link
Member

bcmills commented May 12, 2023

As far as I am aware, the only use-case we have for this at the moment is in cmd/dist. But cmd/dist runs cmd/go as a subprocess — couldn't it parse and transform the JSON output to add the labels for each subprocess itself?

@aclements
Copy link
Member

One advantage of doing it in cmd/go is that it can easily transform the text format as well. See, for example, CL 494407.

Before this change, dist test prints inscrutable blocks like:

##### Testing cgo
...
ok  	cmd/cgo/internal/test	0.889s
ok  	cmd/cgo/internal/test	0.921s
ok  	cmd/cgo/internal/test	0.968s
ok  	cmd/cgo/internal/testtls	0.013s
ok  	cmd/cgo/internal/testtls	0.008s
ok  	cmd/cgo/internal/testtls	0.009s

With this change, it prints:

##### Testing cgo
...
ok  	cmd/cgo/internal/test:auto	0.847s
ok  	cmd/cgo/internal/test:internal	0.718s
ok  	cmd/cgo/internal/test:external-g0	0.720s
ok  	cmd/cgo/internal/testtls:auto	0.012s
ok  	cmd/cgo/internal/testtls:external	0.004s
ok  	cmd/cgo/internal/testtls:external-static	0.003s

These package names are also dist's internal test names now, so if you want to reproduce, you can do, e.g., dist test -run cmd/cgo/internal/test:auto. Currently it's reliably extremely frustrating to figure out how to get dist to re-run the right test in these variant cases.

(This doesn't do much for your argument that dist may be the only user of this.)

@dmitshur
Copy link
Contributor

dmitshur commented May 12, 2023

A possible alternative proposal for cmd/go that would make this flag unnecessary would be to add first-class support for test variants. That is, if there was a way for any Go package to declare that it should be tested multiple times with alternative different settings. To illustrate the point:

As a possible line directive:

//go:testvariant <variant name> <alternative build or test settings for the test>

Or registered at runtime:

package p_test

import "testing"

func init() {
	testing.RegisterTestAlternative("<variant name>", [...])
}

func TestA(t *testing .T) { [...] }

Such that by default go test <import path pattern> would test all alternatives, but presumably there'd be some way to control that, e.g., go test <import path pattern>:<variant pattern> or via new flags. This is meant only to illustrate the point, the proposal would need to think this through more and evaluate the effects on the ecosystem.

If something like that were available, the special case registration done in dist test for some Go packages could be moved over into the packages themselves, and this flag wouldn't be needed.

Of course this -label (or -testlabel as written in proposal title) flag for go test has the benefit of being much smaller compared to the aforementioned alternative, and solves the problem for dist/test better than dist could itself (as @aclements points out above). In addition, it doesn't preclude that larger proposal from possibly happening in the future; the flag can simply stay around or become deprecated if it stops being needed.

Perhaps some large Go projects that currently have a Makefile with multiple targets of differing go test invocations will find use for it too.

@dmitshur
Copy link
Contributor

For the proposed flag and behavior, this seems very reasonable and cleaner than dist doing the JSON event post-processing itself, at the cost of one additional fairly unobtrusive flag added to go test.

It might help to initially constrain the permitted characters (and possibly max length) in the label value. It'll be easy to expand the allowed set if allowing more characters turns out to be worthwhile, not so easy to take it back otherwise.

@bcmills
Copy link
Member

bcmills commented May 12, 2023

@aclements, I think that for the inscrutable dist blocks I would rather see the actual go test command anyway, so I don't have to go digging through the cmd/dist source code to figure out how to reproduce the bug using go test.

Maybe something more like:

##### Testing cgo
...
# go test cmd/cgo/internal/test cmd/cgo/internal/testtls -linkmode=auto
ok  	cmd/cgo/internal/test	0.847s
ok  	cmd/cgo/internal/testtls	0.012s
# go test cmd/cgo/internal/test cmd/cgo/internal/testtls -linkmode=external
ok  	cmd/cgo/internal/test	0.720s
ok  	cmd/cgo/internal/testtls	0.004s
# go test cmd/cgo/internal/test -linkmode=internal
ok  	cmd/cgo/internal/test	0.718s
# CGO_CFLAGS='-g0 -fdiagnostics-color' go test cmd/cgo/internal/test -linkmode=external
ok  	cmd/cgo/internal/test	0.720s

@bcmills
Copy link
Member

bcmills commented May 12, 2023

@dmitshur

A possible alternative proposal for cmd/go that would make this flag unnecessary would be to add first-class support for test variants.

See previously #39005.

@aclements
Copy link
Member

@bcmills, that's pretty reasonable for the text output. We still need some way to distinguish the test variants in the JSON output, preferably in a way that existing CI systems can immediately take advantage of. Were you thinking we'd still do the pkg:variant renaming in the JSON output, but just show the command for the text output? (If we have the logic to print the command, maybe we also inject some Action:"output" record to the JSON with the command...)

For some context, originally I'd proposed test label support in cmd/go to @rsc as a dist-specific back door, largely because we control both cmd/dist and cmd/go and it's much less annoying to implement this in cmd/go and also much easier to test.

@bcmills
Copy link
Member

bcmills commented May 12, 2023

Were you thinking we'd still do the pkg:variant renaming in the JSON output, but just show the command for the text output?

I hadn't really though that far into it, but that sounds like a pretty reasonable approach to me. 😅

@rsc
Copy link
Contributor Author

rsc commented May 17, 2023

Retracted for now; we'll do what we need in cmd/dist and get some experience.

@aclements
Copy link
Member

For reference, CL 494958 does what we need in dist using a stdout filter.

@rsc
Copy link
Contributor Author

rsc commented May 17, 2023

This proposal has been declined as retracted.
— rsc for the proposal review group

@rsc rsc closed this as completed May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Declined
Development

No branches or pull requests

5 participants