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

x/build/cmd/coordinator: switch to "./..." import path pattern for testing packages in a module #51455

Closed
dmitshur opened this issue Mar 3, 2022 · 3 comments
Assignees
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Mar 3, 2022

Back when GOPATH mode was default, assuming one GOPATH workspace, running "go test ./..." in $GOPATH/src/root had the same effect as running "go test {root}/...". The latter was more explicit and did the stated thing even if it was accidentally run from the wrong directory. Maybe that's why it was chosen, or maybe it was chosen arbitrarily, since both worked and did the same thing.

That invocation persisted to today, when module mode is used, and for each module to be tested the coordinator runs "go test {module-root}/..." invocations at the module root to test packages contained inside that module:

// A goTestRun represents a single invocation of the 'go test' command.
type goTestRun struct {
	Dir      string   // Directory where 'go test' should be executed.
	Patterns []string // Import path patterns to provide to 'go test'.
}
// The default behavior is to test the pattern "golang.org/x/{repo}/..."
// in the repository root.
repoPath := importPathOfRepo(st.SubName)
testRuns := []goTestRun{{
	Dir:      "gopath/src/" + repoPath,
	Patterns: []string{repoPath + "/..."},
}}

(source)

From #51283 I've learned there's a difference in that "{module-root}/..." may select a package with import path "{module-root}" even if that package is in another module, but "./..." won't do that.

Since there's no need for the pattern to match any packages outside of the very module it's meant to cover, we could consider switching. It would not fail if a go.sum entry that go mod tidy doesn't currently insert isn't added in some cases (see #51283), remove possibility of unintentional duplicate coverage of packages in nested modules, and least importantly it is shorter visually.

CC @golang/release, @bcmills, @matloob.

@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Mar 3, 2022
@gopherbot gopherbot added this to the Unreleased milestone Mar 3, 2022
@bcmills
Copy link
Contributor

bcmills commented Mar 3, 2022

In my opinion this is the appropriate fix for the builders for the semantics of cmd/go as it exists today.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Mar 8, 2022
@dmitshur
Copy link
Contributor Author

dmitshur commented Mar 8, 2022

Moved to NeedsFix after bringing this up and discussing with the team.

There's not a lot of code that needs to change to implement this, but doing what's needed to validate that it's working as expected (and follow up as needed) may end up being the bulk of the work. (Issue #22437 is also relevant.)

@dmitshur dmitshur changed the title x/build/cmd/coordinator: consider switching to "./..." import path pattern for testing packages in a module x/build/cmd/coordinator: switch to "./..." import path pattern for testing packages in a module May 18, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/418543 mentions this issue: cmd/coordinator: use "./..." pattern when testing golang.org/x repos

@dmitshur dmitshur self-assigned this Jul 21, 2022
@golang golang locked and limited conversation to collaborators Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Archived in project
Development

No branches or pull requests

3 participants