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

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

Open
mvdan opened this issue Jun 9, 2020 · 8 comments
Open

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

mvdan opened this issue Jun 9, 2020 · 8 comments

Comments

@mvdan
Copy link
Member

@mvdan 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
Copy link
Member Author

@mvdan 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.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jun 11, 2020

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 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
Member

@bcmills 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
Member

@bcmills bcmills commented Jun 11, 2020

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 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

@gopherbot gopherbot commented Jun 12, 2020

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 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.