x/tools/go/analysis/internal/checker: tests failing on Plan 9 because of dependence on numeric exit codes

millerresearch commented 5 days ago

Go version

gotip

Output of `go env` in your module/workspace:

GOARCH=arm
GOBIN=
GOCACHE=/home/swarming/.swarming/w/ir/x/w/gocache
GOHOSTARCH=amd64
GOHOSTOS=linux
GOMAXPROCS=4
GOOS=plan9
GOPATH=/home/swarming/.swarming/w/ir/x/w/gopath
GOPLSCACHE=/home/swarming/.swarming/w/ir/x/w/goplscache
GOROOT=
GOROOT_BOOTSTRAP=/home/swarming/.swarming/w/ir/cache/tools/go_bootstrap
GOTOOLCHAIN=local
GO_BUILDER_NAME=x_tools-gotip-plan9-arm

What did you do?

In x/tools subrepo, Plan 9 builder ran go test -json -short ./...

What did you see happen?

Tests TestFixes and TestNoEnd are consistently failing:

--- FAIL: TestFixes (27.93s)
    fix_test.go:80: $ /home/swarming/.swarming/w/ir/x/t/go-build221040081/b410/checker.test -fix rename duplicate
        os.TempDir//analysistest609283431/src/rename/foo.go:4:2: renaming "bar" to "baz"
        os.TempDir//analysistest609283431/src/rename/foo.go:5:6: renaming "bar```
" to "baz"
        os.TempDir//analysistest609283431/src/duplicate/dup.go:4:2: renaming "bar" to "baz"
        os.TempDir//analysistest609283431/src/duplicate/dup.go:5:6: renaming "bar" to "baz"
        os.TempDir//analysistest609283431/src/rename/intestfile_test.go:4:2: renaming "bar" to "baz"
        os.TempDir//analysistest609283431/src/rename/intestfile_test.go:5:6: renaming "bar" to "baz"
        os.TempDir//analysistest609283431/src/rename/foo_test.go:4:2: renaming "bar" to "baz"
        os.TempDir//analysistest609283431/src/rename/foo_test.go:5:6: renaming "bar" to "baz"
    fix_test.go:84: exit code was 1, want 3
--- FAIL: TestNoEnd (2.93s)
    fix_test.go:80: $ /home/swarming/.swarming/w/ir/x/t/go-build221040081/b410/checker.test -fix a
        os.TempDir//analysistest659219173/src/a/a.go:3:1: say hello
    fix_test.go:84: exit code was 1, want 3
FAIL
FAIL    golang.org/x/tools/go/analysis/internal/checker 213.128s

What did you expect to see?

Expected all tests to pass or be skipped.

The underlying problem is that process exit codes in UNIX-family operating systems are numeric, but in Plan 9 they are strings. The Plan 9 convention is that an empty string exit code means "success"; any other string means "failure" and the content of the string explains the reason, in more detail than a numeric errno value would allow.

The current Plan 9 implementation of os.ProcessState.ExitCode() is to return 0 if the Plan 9 exit string is empty, and 1 otherwise. This is sufficient in most cases, but not for applications which attempt to pass more information via exit codes than a simple fail/succeed indication -- as in the case of these failing tests where an exit code of 3 is expected.

My opinion is that it would not be unreasonable, when both parent and child process are Go programs, that if the child process exits with os.Exit(n) then the corresponding os.Wait() in the parent should return with an ExitCode() of n. This could be arranged by parsing the Plan 9 exit string, and if it contains a non-zero numeric value and nothing else, returning that value instead of 1.

In the meantime, however, the TestFixes and TestNoEnd tests should be modified to permit an exit code of 1 in the case of Plan 9,.

gabyhelp commented 5 days ago

Related Issues

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

thanm commented 4 days ago

Thanks for the report.

gopherbot commented 4 days ago

Change https://go.dev/cl/596735 mentions this issue: go/analysis/internal/checker: allow for Plan 9 reduced exit codes in tests

golang / go