Go: cmd/go: TestBuildIDContainsArchModeEnv/386 fails on linux/386 in Go 1.14 and 1.13, not 1.15

[dmitshur]

This test failure is uncovered as of linux-386-longtest builder now testing linux/386, but currently masked by #39309. It became exposed when I was testing a fix for #39309 in CL 237617 and CL 237619.

TestBuildIDContainsArchModeEnv was added in CL 43855 to test that GOARM and GO386 environment variables contribute to the build ID and staleness checks. It's always skipped in short mode.

I haven't confirmed it, but there's a chance that the TestBuildIDContainsArchModeEnv/386 subtest may not have taken into account that it might be run on linux/386, where GOARCH=386 is already the starting condition.

Either because of that, or for another reason, TestBuildIDContainsArchModeEnv/386 has a different stale reason when run on linux/386:

go_test.go:4901: wrong reason for Stale=true: "build ID mismatch", want "stale dependency"

The check for stale _reason_ changed from "build ID mismatch" to "stale dependency: runtime/internal/sys" as part of CL 73212, later changed to just "stale dependency" in CL 112975 (/cc @ysmolsky), and finally was completely removed in the test refactor in CL 214387 (/cc @matloob) (it still checks for staleness, just not the exact reason). This is why this issue is happening on 1.14 and 1.13, but not 1.15.

Next step in this issue is to investigate if this failure suggests there is a problem in cmd/go, or whether the test on 1.14 and 1.13 release branches is bad and should be fixed for linux/386.

/cc @bcmills @matloob @jayconrod

[jayconrod]

I was able to reproduce this on a linux-386 gomote after setting GOOS=linux GOARCH=386 GOHOSTOS=linux GOHOSTARCH=386.

I confirmed GOARM, GO386 and other environment variables are incorporated into the action ID (cache key) at least as far back as go1.13.12 (link). So this seems like an overly brittle test.

Perhaps we should just backport CL 214387, the new version of that test? @matloob WDYT?

[dmitshur]

@jayconrod What we need here for the 1.15 milestone is to confirm the current 1.15 test behavior (which is different from 1.14 and 1.13 test behavior) is intended and correct. If it isn't, it should be fixed before the 1.15 release.

Based on your comment and @bcmills's reaction, it sounds like that's the case, and are now just thinking about how to address the issue in 1.14 and 1.13. Is that the case? Or is there more investigation to do for 1.15 here? Thanks.

[bcmills]

I don't think there's anything left to do for 1.15.

(Users don't typically check for staleness explicitly anyway, so they don't care what specific text we use to report it as long as we correctly deduce whether targets actually need to be rebuilt. Since they don't care about the specific text, we shouldn't either.)

[bcmills]

I think Jay's suggestion to backport CL 214387 is probably the right approach for 1.14 and 1.13.

[gopherbot]

Change https://golang.org/cl/239738 mentions this issue: [release-branch.go1.14] cmd/go: convert TestBuildIDContainsArchModeEnv to the script framework

[dmitshur]

Thanks for confirming.

@bcmills FWIW, if backporting the test script test proves to be tricky due to merge conflicts or missing code in previous versions, another way to fix the test in 1.14 and 1.13 release branches would be with by removing the stale reason from the wantStale helper:

-tg.wantStale("mycmd", "stale dependency", "should be stale after environment variable change")
+tg.wantStale("mycmd", "", "should be stale after environment variable change")

When the reason parameter to wantStale helper is the empty string, it disables the reason check (the staleness is still checked independently of the reason), see here.

[dmitshur]

@gopherbot Please backport to the last 2 releases. This is a test fix. It makes a cmd/go test less strict and enables it to pass in all environments, which matches the behavior of the same test in the latest version, which has been confirmed to be most optimal.

[gopherbot]

Backport issue(s) opened: #39823 (for 1.13), #39824 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[dmitshur]

I've opened 2 backport issues. Closing this, since this is resolved for 1.15.

[dmitshur]

When the reason parameter to wantStale helper is the empty string, it disables the reason check (the staleness is still checked independently of the reason), see here.

Hmm, I wrote that based on memory, but now that I look at the code, it looks like it would fail even if reason parameter is the empty string, if isStale gives a non-empty why.

In that case, the smallest direct test fix I can think of is to add a new wantStaleIgnoreReason helper for use in TestBuildIDContainsArchModeEnv. That's probably better than the hack of using a single space as the reason string.