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.

Sign up for GitHub

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

Jump to bottom

cmd/go: test -c should error on unknown flags #39484

Closed
mvdan opened this issue Jun 9, 2020 · 9 comments
Closed

cmd/go: test -c should error on unknown flags #39484

mvdan opened this issue Jun 9, 2020 · 9 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.16

Comments

@mvdan
Copy link
Member

mvdan commented Jun 9, 2020 

$ go version
go version devel +cacac8bdc5 Tue Jun 9 02:49:06 2020 +0000 linux/amd64

The way go test works, one can mix test's own flags as well as the user's custom flags in the test package. For example, one can run go test -foo -v -bar -count=10, which is roughly equivalent to go test -c -o=test && ./test -test.v -test.count=10 -foo -bar.

I assume that the way this works is that any flags go test doesn't know will be passed to the test binary when running the tests, since they might be declared by the user. This design is perhaps a bit confusing, but works.

However, I think that go test -c shouldn't allow uknown flags. It will not run the test binary at all, so it doesn't make any sense to me. For example, on a simple Go package with tests:

$ go test -c
$ go test -c -badflag
$ go test -badflag
flag provided but not defined: -badflag

This is especially bad because it makes typos very easy to miss. I had written go test -c -glags=all=-dwarf=false, which was succeeding. It took me another hour to realise the -gcflags typo, and that the flag wasn't doing anything at all.

If -c is used, any unknown flag should be an error, even if it precedes -c. If this change seems fine, I can send a CL.

/cc @bcmills @jayconrod @matloob @dominikh
@mvdan mvdan added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go labels Jun 9, 2020
@mvdan
Copy link
Member Author

mvdan commented Jun 9, 2020

The only possible reason I see in favor of the current behavior is that one can take an entire go test command with any custom flags, and add -c to the list of flags without worrying about the command suddenly failing.

However, that doesn't seem like a good decision to me. go test -c -customflag should not succeed. If it does, it signals that it actually used -customflag somehow, but it didn't.

@bcmills bcmills added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jun 9, 2020
@cherrymui
Copy link
Member

What about flags known to go test, like -run or -count? They don't make sense for go test -c either. Currently the command succeeds, though.

@mvdan
Copy link
Member Author

mvdan commented Jun 11, 2020

That's a bit of an open question. To me, they should error as well, but it doesn't seem like such a clear case. Perhaps we could leave those for a follow-up issue, to reduce the chances that the entire change gets reverted due to regressions.

@bcmills
Copy link
Contributor

bcmills commented Jun 11, 2020

If the flag is known to go test, then it's less likely to be a typo of some other known flag. So I would continue to accept those, on the theory that continuing to accept them does make it easier to just tack a -c on the end of a command to get a binary.

@bcmills
Copy link
Contributor

bcmills commented Jun 11, 2020 •
edited
Loading

That is: the cost of rejecting flags is the same for both known and unknown flags, but the benefit is higher for unknown ones.

(The cost is the need to remove the flag before attaching -c. The benefit is the likelihood of catching an accidental typo of some other flag.)

@mvdan
Copy link
Member Author

mvdan commented Jun 11, 2020

Thinking about it again, I'm not sure I agree that "known flags that do nothing" should error. We similarly silently ignore those in other commands, such as go get -d -ldflags=-w golang.org/x/tools working for me even though -ldflags will never do anything due to -d. So it seems to me like that should be a separate issue to tackle the corner case in all cmd/go commands consistently.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/237697 mentions this issue: cmd/go: error when -c or -i are used with unknown flags

@mvdan mvdan added this to the Go1.16 milestone Jun 12, 2020
@mvdan
Copy link
Member Author

mvdan commented Jun 12, 2020

I've sent a patch with a test, mainly to get some eyes during the freeze - it's meant for 1.16, and I agree with the early-in-cycle label.

@dmitshur
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.16.
That time is now, so this is a friendly ping so the issue is looked at again.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 24, 2020
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 24, 2020
@golang golang locked and limited conversation to collaborators Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
5 participants
@dmitshur @mvdan @bcmills @gopherbot @cherrymui