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

Jump to bottom

x/review/git-codereview: skip tests that require "gofmt" binary when it's not available #36801

Open
bcmills opened this issue Jan 27, 2020 · 4 comments
Open

x/review/git-codereview: skip tests that require "gofmt" binary when it's not available #36801

bcmills opened this issue Jan 27, 2020 · 4 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

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)
@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 27, 2020
@bcmills bcmills added this to the Go1.14 milestone Jan 27, 2020
@tklauser
Copy link
Member

CC @Helflym

@dmitshur
Copy link
Contributor

dmitshur commented Jan 28, 2020 •
edited

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 added NeedsFix The path to resolution is known, but the work has not been done. and removed Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 28, 2020
@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
Copy link
Contributor

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
@ianlancetaylor
Copy link
Contributor

Moving to backlog milestone.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Backlog Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@toothrot @tklauser @dmitshur @ianlancetaylor @bcmills @Helflym