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/env/android-*-emu: adb does not propagate GO_BUILDER_NAME to tests #39460

Open
matloob opened this issue Jun 8, 2020 · 8 comments
Open

x/build/env/android-*-emu: adb does not propagate GO_BUILDER_NAME to tests #39460

matloob opened this issue Jun 8, 2020 · 8 comments

Comments

@matloob
Copy link
Contributor

@matloob matloob commented Jun 8, 2020

The GO_BUILDER_NAME environment variable doesn't seem to be set on tests running on the android-amd64-emu builders.

How to reproduce:

I created CL 236960 with the following test in it:

package a_test

import (
	"os"
	"testing"
)

func TestPlatform(t *testing.T) {
	t.Fatalf("GO_BUILDER_NAME = %q", os.Getenv("GO_BUILDER_NAME"))
}

on the android-amd64-emu builder, the test fails with the following error:

--- FAIL: TestPlatform (0.00s)
    platform_test.go:9: GO_BUILDER_NAME = ""
FAIL
exitcode=1FAIL	golang.org/x/tools/a	84.167s
@gopherbot gopherbot added this to the Unreleased milestone Jun 8, 2020
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Jun 8, 2020

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Jun 8, 2020

From the linked CL:

Failed on android-amd64-emu: https://storage.googleapis.com/go-build-log/bddf75d8/android-amd64-emu_280341b8.log

The printed go test invocation there looks right and includes the GO_BUILDER_NAME env var:

:: Running /workdir/go/bin/go with args
["/workdir/go/bin/go" "test" "-short" "golang.org/x/tools/..."]
and env
[[...]
"GO_BUILDER_NAME=android-amd64-emu"
[...]]
in dir /workdir/gopath/src/golang.org/x/tools

[...]
--- FAIL: TestPlatform (0.00s)
    platform_test.go:9: GO_BUILDER_NAME = ""
FAIL

That is interesting.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jan 13, 2021

I just got bitten by this while working on #43554. I thought that I'd disabled regtests on android, similar to other builders, and my guard wasn't working.

Before I found this issue, I looked into it a bit. I think the problem is that only certain environment variables are sent via adb:
https://cs.opensource.google/go/go/+/master:misc/android/go_android_exec.go;l=168;drc=fb63ed2babb535b19ae1755742e672ef17e57262

@dmitshur dmitshur changed the title x/build/env/android-amd64-emu: GO_BUILDER_NAME not set on test in x/tools x/build/env/android-amd64-emu: GO_BUILDER_NAME is not propagated to tests Jan 13, 2021
@findleyr findleyr changed the title x/build/env/android-amd64-emu: GO_BUILDER_NAME is not propagated to tests x/build/env/android-*-emu: GO_BUILDER_NAME is not propagated to tests Jan 13, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented Jan 13, 2021

Updated the title as this affects 386 as well.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 13, 2021

This should be fixed, but OTOH varying a test's behavior based on GO_BUILDER_NAME is a huge code smell.

If a test is failing when our builders run it, it's probably failing when our users run it too, and they shouldn't need to set GO_BUILDER_NAME just to get tests to pass reliably.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jan 13, 2021

This should be fixed, but OTOH varying a test's behavior based on GO_BUILDER_NAME is a huge code smell.

@bcmills GO_BUILDER_NAME doesn't vary behavior within the test, it causes tests to be skipped when -short is set. As described in #43554, disabling the tests was considered a temporary measure. The signal:noise ratio is particularly low on android where we likely have very few users (if any), but even so understanding these failures has been my primary focus over the past week, and with my next stack of CLs I think I'll have fixed the largest source of flakes.

I don't want to stray too far off-topic here, but I do think that skipping tests based on GO_BUILDER_NAME is a justifiable approach. gopls is heavily dependent on operating system behavior, and is also a client application unlikely to be deployed in certain environments. As long as the scope and nature of failures are understood, IMO it could easily be the case that the engineering effort to fix a test on certain builders would not be worthwhile.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jan 13, 2021

@bcmills it occurred to me that perhaps you meant simply that if the test fails on Android, it should be guarded with GOOS rather than GO_BUILDER_NAME. I'd agree with that -- I used GO_BUILDER_NAME as I didn't understand the nature of the problem at the time.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 20, 2021

Change https://golang.org/cl/284935 mentions this issue: gopls/internal/regtest: re-enable android builder

gopherbot pushed a commit to golang/tools that referenced this issue Jan 20, 2021
Remove the exception for the Android amd64 builder, now that flakes are
fixed (though the exception didn't actually work anyway due to
golang/go#39460).

Also, accept the import reorganization applied by gopls.

Fixes golang/go#43554

Change-Id: I9a7cce35998cfa673699d74a487111e4daecf7ec
Reviewed-on: https://go-review.googlesource.com/c/tools/+/284935
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@dmitshur dmitshur changed the title x/build/env/android-*-emu: GO_BUILDER_NAME is not propagated to tests x/build/env/android-*-emu: adb does not propagate GO_BUILDER_NAME to tests Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants