Skip to content

x/build/cmd/golangbuild: also move or delete untested nested modules for run-tests-outside-repository check #65267

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

Closed
dmitshur opened this issue Jan 24, 2024 · 1 comment
Assignees
Labels
Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Jan 24, 2024

One of the items implemented in #60666 was:

  • run subrepo tests from outside their repositories (equivalent to x/build/cmd/coordinator's go.dev/issue/34352)

The repoToModules function is responsible for finding modules in repoDir to be tested, and for repos without local replace directives, it also moves nested modules to directories that aren't predictably-relative to each other.

It's right for repoToModules to skip over directories that aren't intended to have testable modules (for example, directories with "." or "_" prefixes, or named "testdata"), but they should still be visited for the purpose of moving them to an unpredictable location (or, removing them entirely, since their content isn't needed for running tests).

Filing this to track that fix. When rolling it out, it's worth paying attention to how many newly failing tests it begins to catch, and to give them an opportunity to be fixed or otherwise handled without undue time pressure.

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 24, 2024
@dmitshur dmitshur added this to the Unreleased milestone Jan 24, 2024
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Jan 24, 2024
@dmitshur dmitshur moved this to Planned in Go Release Feb 21, 2024
@dmitshur dmitshur moved this from Planned to In Progress in Go Release Sep 18, 2024
@dmitshur dmitshur self-assigned this Sep 18, 2024
@dmitshur
Copy link
Contributor Author

dmitshur commented Sep 26, 2024

Fixed in crrev.com/c/5873674.

As a simple demonstration, CL 615681 contains a test that intentionally reaches across module boundary and reports what happens. Build 8735795593855449825 failed before, and build 8735722460980213777 is the new passing behavior after.

Newly failing tests

Five of the x/ repos (x/debug, x/pkgsite, x/pkgsite-metrics, x/vuln, and x/vulndb) currently place go.mod files inside testdata directories. For example:

$ git clone https://go.googlesource.com/vulndb
$ cd vulndb
$ find . -name go.mod | grep /testdata/
./internal/symbols/testdata/module/submodule/testdata/go.mod
./internal/symbols/testdata/module/submodule/go.mod
./internal/symbols/testdata/module/go.mod
./internal/symbols/testdata/fixed-module/submodule/testdata/go.mod
./internal/symbols/testdata/fixed-module/submodule/go.mod
./internal/symbols/testdata/fixed-module/go.mod

This works only when package tests are running in a git checkout of the repository, not from the module cache:

$ cd $(mktemp -d)
$ go mod init example
go: creating new go.mod: module example
$ go get -t golang.org/x/vulndb
go: added golang.org/x/vulndb v0.0.0-20240925174358-74aba449dd50
$ go test golang.org/x/vulndb/internal/symbols
--- FAIL: TestPatchedSymbols (0.00s)
    patched_functions_test.go:42: lstat testdata/module: no such file or directory
    patched_functions_test.go:46: lstat testdata/fixed-module: no such file or directory
    patched_functions_test.go:54: (-got, want+):
          map[symbols.symKey]bool{
        + 	{pkg: "golang.org/module", symbol: "Foo"}:          true,
        + 	{pkg: "golang.org/module/internal", symbol: "Bar"}: true,
          }
    patched_functions_test.go:42: lstat testdata/module: no such file or directory
    patched_functions_test.go:46: lstat testdata/fixed-module: no such file or directory
    patched_functions_test.go:54: (-got, want+):
          map[symbols.symKey]bool{
        + 	{pkg: "golang.org/nestedmodule", file: "main_linux.go", symbol: "main"}: true,
          }
--- FAIL: TestModuleSymbols (0.00s)
    patched_functions_test.go:85: lstat testdata/module: no such file or directory
    patched_functions_test.go:89: (-got, want+):
          map[symbols.symKey]bool{
        + 	{pkg: "golang.org/module", symbol: "Foo"}:          true,
        + 	{pkg: "golang.org/module", symbol: "main"}:         true,
        + 	{pkg: "golang.org/module/internal", symbol: "Bar"}: true,
          }
    patched_functions_test.go:85: lstat testdata/module/submodule: no such file or directory
    patched_functions_test.go:89: (-got, want+):
          map[symbols.symKey]bool{
        + 	{pkg: "golang.org/nestedmodule", file: "main_linux.go", symbol: "main"}:   true,
        + 	{pkg: "golang.org/nestedmodule", file: "main_windows.go", symbol: "main"}: true,
          }
--- FAIL: TestModuleRootAndFiles (0.00s)
    patched_functions_test.go:127: lstat testdata/module: no such file or directory
    patched_functions_test.go:132: got []; want [bar.go foo.go main.go]
    patched_functions_test.go:137: module root: got ; want module
    patched_functions_test.go:127: lstat testdata/module: no such file or directory
    patched_functions_test.go:132: got []; want [main_linux.go main_windows.go]
    patched_functions_test.go:137: module root: got ; want module/submodule
    patched_functions_test.go:127: lstat testdata/module: no such file or directory
    patched_functions_test.go:127: lstat testdata/module: no such file or directory
--- FAIL: TestModuleRoots (0.00s)
    patched_functions_test.go:157: lstat testdata/module: no such file or directory
FAIL
FAIL	golang.org/x/vulndb/internal/symbols	0.633s
FAIL

This can now be caught on builders - see build 8735791928943668417 for example.

For now, they're considered opted-out of this strict behavior (as listed here). The repo owners can choose to do the work to remove go.mod files and opt-in. It's also possible to use the golang.force_test_outside_repository LUCI experiment to force a build to include this test behavior even in repos that are opted out, which can be used to check that everything's fixed or if anything new started to reach across module boundaries.

Other than these deliberate uses of go.mod files in testdata directories for convenience of representing test module data, there fortunately weren't other unintended cases of tests reaching across module boundaries unnecessarily. The new test behavior will prevent such accidental cases – in the repos that aren't opted-out – from happening in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done.
Projects
Archived in project
Development

No branches or pull requests

2 participants