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/review/git-codereview: skip tests that require "gofmt" binary when it's not available #36801

Open
bcmills opened this issue Jan 27, 2020 · 3 comments
Labels
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 27, 2020

https://build.golang.org/log/36fc6a856500bd206f0f994b1565fb6badbe7034

aix-ppc64 at 6fbdfe48041c883a9f6d8c981a7205b7c327862a building review at f51a73253c4da005cfdf18a036e11185c04c8ce3
…
--- FAIL: TestGofmt (0.55s)
    gofmt_test.go:46: git-codereview gofmt -l
    util_test.go:323: died
        stdout:
        stderr:
        git-codereview: invoking gofmt: exec: "gofmt": executable file not found in $PATH
…

I'm not sure whether this should be resolved by adding gofmt to the builder's PATH, by updating the git-codereview tool to invoke go fmt instead, by updating the test to install that tool if not present, or by updating the test to skip those cases if the gofmt binary is not found.

This is technically a release-blocker via #11811, but seems pretty minor.
(CC @dmitshur @cagedmantis @toothrot @trex58)

@tklauser

This comment has been minimized.

Copy link
Member

@tklauser tklauser commented Jan 27, 2020

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Jan 28, 2020

Is the problem that only the gofmt binary isn't present in PATH, or is the go binary not present on aix-ppc64 either?

The answer, from reading the failure log, is that "go" isn't available either:

exec: "gofmt": executable file not found in $PATH
...
exec: "go": executable file not found in $PATH

Tests that require the go binary usually skip themselves if the go binary isn't available (for example, see internal/testenv). So it seems reasonable to do the same for a test that requires the gofmt binary:

if !hasGofmt() {
	t.Skipf("skipping test: 'gofmt' not available on %s/%s", runtime.GOOS, runtime.GOARCH)
}

It's already being done for the git binary, see git-codereview/util_test.go.

/cc @josharian @kevinburke per owners.

If the builder can be improved to make the go, gofmt and/or git binaries available, that would be a good change to make, as it would increase the amount of tests that it is able to run. But this is a separate enhancement.

@dmitshur dmitshur changed the title x/review/git-codereview: gofmt tests consistently failing on aix-ppc64 builder x/review/git-codereview: skip tests that require "gofmt" binary when it's not available Jan 28, 2020
@Helflym

This comment has been minimized.

Copy link
Contributor

@Helflym Helflym commented Jan 28, 2020

gofmt is available on aix/ppc64. But for an unknown reason it was missing from the default path of the aix builder, likewise for go binary.
I've added them so let see if it's alright now.

@toothrot toothrot modified the milestones: Go1.14, Go1.15 Feb 25, 2020
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
You can’t perform that action at this time.