x/build/cmd/coordinator: reduce nesting in buildStatus.runSubrepoTests

[dmitshur]

As part of working on resolving issues #32528, #30233, and #34190, I've made incremental changes to the runSubrepoTests method with the goal of keeping each diff minimal and easy to be confident in. By the end, the method has become quite large as it now deals with both module and GOPATH modes, with local variables spanning large parts of the code.

I've attempted to factor out parts of the method into smaller pieces in CL 194684, but in the short time I had, couldn't get to an outcome that was an improvement. Part of the problem was the difficulty of creating helpers methods due to dual-error return pattern that's prevalent. That's issue #34246, it probably needs to be tackled before or at the same time as this.

/cc @bradfitz @andybons

[gopherbot]

Change https://golang.org/cl/195277 mentions this issue: cmd/coordinator: factor out GOPATH dep fetching from runSubrepoTests

[gopherbot]

Change https://golang.org/cl/195278 mentions this issue: cmd/coordinator: simplify fetchDependenciesToGOPATHWorkspace

[dmitshur]

Edit: Moved this comment to the right issue, see #32528 (comment).

[bcmills]

while correctly ignoring other go.mod files in testdata directories:

go.mod files in testdata directories still produce module splits; see #27852. (CC @heschik)

A more robust alternative is to copy (or symlink) the testdata into a temporary directory and drop in the go.mod file at that point. See https://github.com/golang/go/blob/master/misc/cgo/test/overlaydir_test.go for one example of how to do that.

[dmitshur]

@bcmills I'm aware that go.mod files in testdata directories create module boundaries.

I've discussed it with @ianthehat and we concluded that such modules have module paths unsuitable for external use. No one should require the module with the module path golang.org/x/tools/go/packages/testdata/TestName_Modules/pkg/mod/github.com/heschik/tools-testrepo@v1.0.0. It's not even allowed because it contains '@'. It's not expected that they will contain valid Go packages that need to be tested inside. Instead, it's expected that they are testdata for other valid modules, which we do run tests for.

[bcmills]

we concluded that such modules have module paths unsuitable for external use.

Yeah, the problem is not those modules themselves — the problem is that the tests for the parent module will fail when run from within any other module (because their testdata is missing), which implies that a user of (say) golang.org/x/go/packages cannot re-run its tests from within their own module to verify that their selected configuration has not introduced an incompatibility.

[dmitshur]

That's a fair point. But I think that's a problem that needs to be addressed by the module authors themselves.

The scope of this issue is about increasing the test coverage of golang.org/x repos to include supported nested modules in addition to the module at the root. Doing additional work to test that modules can be tested successfully without the rest of the git repository contents should be a separate issue.

[bcmills]

Or, to put it another way: ignoring the testdata modules causes the builders to miss test failures that will be observed by users outside the module in general.

We should probably set up builders for all of the x/ modules to verify that their tests pass when they are not the main module. I would argue that that is actually more important than running the tests when they are the main module, since individual developers will presumably notice those test failures.

[bcmills]

Doing additional work to test that modules can be tested successfully without the rest of the git repository contents should be a separate issue.

Agreed. I thought we had an issue filed for that, but I can't find it — do you happen to recall one way or the other?

[dmitshur]

I haven't seen such an issue.

[dmitshur]

while correctly ignoring other go.mod files in testdata directories:

Also, I should clarify, what I meant there was "as intended". It's not that one way or another is correct, there are different testing strategies with trade-offs. What I observed was that the change I made to coordinator was working as intended.