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

Jump to bottom

proposal: cmd/go: make "go test" exit 0 if *.go files exist but none match context #39426

Closed
bradfitz opened this issue Jun 5, 2020 · 9 comments
Closed

proposal: cmd/go: make "go test" exit 0 if *.go files exist but none match context #39426

bradfitz opened this issue Jun 5, 2020 · 9 comments
Labels
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Jun 5, 2020

It's a little weird right now that go test in a directory of non-context-matching *.go files (like golang.org/x/sys/windows/registry on Linux) results in an error, but go test in a directory without tests is fine (exit 0), and go test ./... is also fine, as long as one directory somewhere has a context-matching file.

dev:~ $ mkdir /tmp/foo
dev:~ $ cd /tmp/foo
dev:foo $ go mod init foo
go: creating new go.mod: module foo

dev:foo $ go test
no Go files in /tmp/foo
dev:foo $ echo $?
1

dev:foo $ cat > x_windows.go
package main

dev:foo $ go test
package foo: build constraints exclude all Go files in /tmp/foo
dev:foo $ echo $?
1

dev:foo $ cat > x.go
package main

dev:foo $ go test
?       foo     [no test files]
dev:foo $ echo $?
0

dev:foo $ go test ./...
go: warning: "./..." matched no packages
no packages to test

dev:foo $ mkdir bar
dev:foo $ cat > bar/bar.go
package bar

dev:foo $ go test ./...
?       foo/bar [no test files]
dev:foo $ echo $?
0

dev:foo $ go test golang.org/x/sys/windows/registry
go: finding module for package golang.org/x/sys/windows/registry
go: downloading golang.org/x/sys v0.0.0-20200602225109-6fdc65e7d980
go: found golang.org/x/sys/windows/registry in golang.org/x/sys v0.0.0-20200602225109-6fdc65e7d980
package golang.org/x/sys/windows/registry: build constraints exclude all Go files in /home/bradfitz/pkg/mod/golang.org/x/sys@v0.0.0-20200602225109-6fdc65e7d980/windows/registry

Can we instead make go test act more like the package has no tests? I'd like to see:

$ go test golang.org/x/sys/windows/registry
?       golang.org/x/sys/windows/registry [no matching files]
$ echo $?
0

This arose due to integration of Go in another build system (that builds non-Go stuff) and that build system was using the heuristic that if a directory contains *_test.go files, it running go test in that directory was reasonable. That's currently not true for directories like x/sys/windows/registry or other such OS-specific packages.
@gopherbot gopherbot added this to the Proposal milestone Jun 5, 2020
@rsc rsc added this to Incoming in Proposals (old) Jun 10, 2020
@rsc
Copy link
Contributor

rsc commented Jul 19, 2021

It's unclear what the change would be here exactly.

Files that exist but don't match context are treated the same as if they didn't exist at all.

go test without an argument is identical to go test ., and when you say a specific package like . or math (not a wildcard), it is required to exist.

Currently go test . says

package foo: build constraints exclude all Go files in /tmp/foo

and that is consistent with go build ., go install ., go doc ., go run ..

It seems like all of those four really need to fail instead of just printing a warning.

So then the question is whether go test should be different from those, and if so, how to draw the line.

We may have backed ourselves into a bit of a puzzle where there's no easy way out.

@rsc
Copy link
Contributor

rsc commented Aug 18, 2021

Adding to minutes; I still don't see an easy way out here.

@rsc
Copy link
Contributor

rsc commented Aug 18, 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

@rsc rsc moved this from Incoming to Active in Proposals (old) Aug 18, 2021
@rsc
Copy link
Contributor

rsc commented Aug 25, 2021

/cc @bcmills @jayconrod @matloob

@jayconrod
Copy link
Contributor

The current go test behavior seems more consistent with other commands.

Might be good to know about the other build system and whether it can avoid running go test in directories like this?

As a point of comparison, Bazel will generally have go_library and go_test targets for each package. Build constraints are applied at execution time, right before invoking the compiler. If all source files are excluded, Bazel just compiles a file with package empty.

@bcmills
Copy link
Contributor

bcmills commented Aug 25, 2021

IMO the cleaner solution here is to set up the .go source files for the package in such a way that OS-specific packages continue to exist (and continue to compile) on platforms that do not support the OS-specific functionality.

For the x/sys/windows/registry package, that might mean that on an unsupported platform the CreateKey, OpenKey, etc. functions that can return an error return an invariantly non-nil one (such as one wrapping errors.ErrUnsupported; see #41198).

Then go test could correctly report that all matching tests pass (if there are any OS-agnostic tests for the package), and could at the very least report any go vet errors on the OS-agnostic parts of the API.

@rsc
Copy link
Contributor

rsc commented Sep 1, 2021

IMO the cleaner solution here is to set up the .go source files for the package in such a way that OS-specific packages continue to exist (and continue to compile) on platforms that do not support the OS-specific functionality.

FWIW that's just echo package p >dummy.go.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Sep 1, 2021
@rsc
Copy link
Contributor

rsc commented Sep 1, 2021

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

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Sep 8, 2021
@rsc
Copy link
Contributor

rsc commented Sep 8, 2021

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

@rsc rsc closed this as completed Sep 8, 2021
@golang golang locked and limited conversation to collaborators Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
5 participants
@bradfitz @rsc @jayconrod @bcmills @gopherbot