From cd9d26f0da769c5644ab7956433991385259ee0a Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Fri, 11 Nov 2022 11:51:28 -0500 Subject: [PATCH] cmd/go: make testterminal18153 a normal test Currently, cmd/go's testterminal18153 is implemented as a special test that doesn't run as part of cmd/go's regular tests. Because the test requires stdout and stderr to be a terminal, it is currently run directly by "dist test" so it can inherit the terminal of all.bash. This has a few problems. It's yet another special case in dist test. dist test also has to be careful to not apply its own buffering to this test, so it can't run in parallel and it limits dist test's own scheduler. It doesn't run as part of regular "go test", which means it usually only gets coverage from running all.bash. And since we have to skip it if all.bash wasn't run at a terminal, I'm sure it often gets skipped even when running all.bash. Fix all of this by rewriting this test to create its own PTY and re-exec "go test" to check that PTY passes through go test. This makes the test self-contained, so it can be a regular cmd/go test, we can drop it from dist test, and it's not sensitive to the environment of all.bash. Preparation for #37486. Updates #18153. Change-Id: I6493dbb0143348e299718f6e311ac8a63f5d69c5 Reviewed-on: https://go-review.googlesource.com/c/go/+/449503 Run-TryBot: Austin Clements Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot --- src/cmd/dist/test.go | 27 ---- src/cmd/dist/test_linux.go | 28 ----- src/cmd/go/terminal_test.go | 119 ++++++++++++++++++ .../testterminal18153/terminal_test.go | 40 ------ src/internal/testpty/pty.go | 12 +- src/internal/testpty/pty_none.go | 13 ++ 6 files changed, 139 insertions(+), 100 deletions(-) delete mode 100644 src/cmd/dist/test_linux.go create mode 100644 src/cmd/go/terminal_test.go delete mode 100644 src/cmd/go/testdata/testterminal18153/terminal_test.go create mode 100644 src/internal/testpty/pty_none.go diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go index 1d1f325bc7..3e30bccd5a 100644 --- a/src/cmd/dist/test.go +++ b/src/cmd/dist/test.go @@ -453,10 +453,6 @@ func (t *tester) registerRaceBenchTest(pkg string) { }) } -// stdOutErrAreTerminals is defined in test_linux.go, to report -// whether stdout & stderr are terminals. -var stdOutErrAreTerminals func() bool - func (t *tester) registerTests() { // Fast path to avoid the ~1 second of `go list std cmd` when // the caller lists specific tests to run. (as the continuous @@ -591,29 +587,6 @@ func (t *tester) registerTests() { } } - // This test needs its stdout/stderr to be terminals, so we don't run it from cmd/go's tests. - // See issue 18153. - if goos == "linux" { - t.tests = append(t.tests, distTest{ - name: "cmd_go_test_terminal", - heading: "cmd/go terminal test", - fn: func(dt *distTest) error { - t.runPending(dt) - timelog("start", dt.name) - defer timelog("end", dt.name) - if !stdOutErrAreTerminals() { - fmt.Println("skipping terminal test; stdout/stderr not terminals") - return nil - } - cmd := exec.Command(gorootBinGo, "test") - setDir(cmd, filepath.Join(os.Getenv("GOROOT"), "src/cmd/go/testdata/testterminal18153")) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - return cmd.Run() - }, - }) - } - // On the builders only, test that a moved GOROOT still works. // Fails on iOS because CC_FOR_TARGET refers to clangwrap.sh // in the unmoved GOROOT. diff --git a/src/cmd/dist/test_linux.go b/src/cmd/dist/test_linux.go deleted file mode 100644 index 43d28dc661..0000000000 --- a/src/cmd/dist/test_linux.go +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright 2016 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -//go:build linux -// +build linux - -package main - -import ( - "syscall" - "unsafe" -) - -const ioctlReadTermios = syscall.TCGETS - -// isTerminal reports whether fd is a terminal. -func isTerminal(fd uintptr) bool { - var termios syscall.Termios - _, _, err := syscall.Syscall6(syscall.SYS_IOCTL, fd, ioctlReadTermios, uintptr(unsafe.Pointer(&termios)), 0, 0, 0) - return err == 0 -} - -func init() { - stdOutErrAreTerminals = func() bool { - return isTerminal(1) && isTerminal(2) - } -} diff --git a/src/cmd/go/terminal_test.go b/src/cmd/go/terminal_test.go new file mode 100644 index 0000000000..03ca772700 --- /dev/null +++ b/src/cmd/go/terminal_test.go @@ -0,0 +1,119 @@ +// Copyright 2016 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main_test + +import ( + "errors" + "internal/testenv" + "internal/testpty" + "io" + "os" + "testing" + + "golang.org/x/term" +) + +func TestTerminalPassthrough(t *testing.T) { + // Check that if 'go test' is run with a terminal connected to stdin/stdout, + // then the go command passes that terminal down to the test binary + // invocation (rather than, e.g., putting a pipe in the way). + // + // See issue 18153. + testenv.MustHaveGoBuild(t) + + // Start with a "self test" to make sure that if we *don't* pass in a + // terminal, the test can correctly detect that. (cmd/go doesn't guarantee + // that it won't add a terminal in the middle, but that would be pretty weird.) + t.Run("pipe", func(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe failed: %s", err) + } + defer r.Close() + defer w.Close() + stdout, stderr := runTerminalPassthrough(t, r, w) + if stdout { + t.Errorf("stdout is unexpectedly a terminal") + } + if stderr { + t.Errorf("stderr is unexpectedly a terminal") + } + }) + + // Now test with a read PTY. + t.Run("pty", func(t *testing.T) { + r, processTTY, err := testpty.Open() + if errors.Is(err, testpty.ErrNotSupported) { + t.Skipf("%s", err) + } else if err != nil { + t.Fatalf("failed to open test PTY: %s", err) + } + defer r.Close() + w, err := os.OpenFile(processTTY, os.O_RDWR, 0) + if err != nil { + t.Fatal(err) + } + defer w.Close() + stdout, stderr := runTerminalPassthrough(t, r, w) + if !stdout { + t.Errorf("stdout is not a terminal") + } + if !stderr { + t.Errorf("stderr is not a terminal") + } + }) +} + +func runTerminalPassthrough(t *testing.T, r, w *os.File) (stdout, stderr bool) { + cmd := testenv.Command(t, testGo, "test", "-run=^$") + cmd.Env = append(cmd.Environ(), "GO_TEST_TERMINAL_PASSTHROUGH=1") + cmd.Stdout = w + cmd.Stderr = w + t.Logf("running %s", cmd) + err := cmd.Start() + if err != nil { + t.Fatalf("starting subprocess: %s", err) + } + w.Close() + // Read the subprocess output. The behavior of reading from a PTY after the + // child closes it is very strange (e.g., on Linux, read returns EIO), so we + // ignore errors as long as we get everything we need. We still try to read + // all of the output so we can report it in case of failure. + buf, err := io.ReadAll(r) + if len(buf) != 2 || !(buf[0] == '1' || buf[0] == 'X') || !(buf[1] == '2' || buf[1] == 'X') { + t.Errorf("expected exactly 2 bytes matching [1X][2X]") + if err != nil { + // An EIO here might be expected depending on OS. + t.Errorf("error reading from subprocess: %s", err) + } + } + err = cmd.Wait() + if err != nil { + t.Errorf("suprocess failed with: %s", err) + } + if t.Failed() { + t.Logf("subprocess output:\n%s", string(buf)) + t.FailNow() + } + return buf[0] == '1', buf[1] == '2' +} + +func init() { + if os.Getenv("GO_TEST_TERMINAL_PASSTHROUGH") == "" { + return + } + + if term.IsTerminal(1) { + print("1") + } else { + print("X") + } + if term.IsTerminal(2) { + print("2") + } else { + print("X") + } + os.Exit(0) +} diff --git a/src/cmd/go/testdata/testterminal18153/terminal_test.go b/src/cmd/go/testdata/testterminal18153/terminal_test.go deleted file mode 100644 index 34ee580c0e..0000000000 --- a/src/cmd/go/testdata/testterminal18153/terminal_test.go +++ /dev/null @@ -1,40 +0,0 @@ -// Copyright 2016 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -//go:build linux -// +build linux - -// This test is run by src/cmd/dist/test.go (cmd_go_test_terminal), -// and not by cmd/go's tests. This is because this test requires -// that it be called with its stdout and stderr being a terminal. -// dist doesn't run `cmd/go test` against this test directory if -// dist's stdout/stderr aren't terminals. -// -// See issue 18153. - -package p - -import ( - "syscall" - "testing" - "unsafe" -) - -const ioctlReadTermios = syscall.TCGETS - -// isTerminal reports whether fd is a terminal. -func isTerminal(fd uintptr) bool { - var termios syscall.Termios - _, _, err := syscall.Syscall6(syscall.SYS_IOCTL, fd, ioctlReadTermios, uintptr(unsafe.Pointer(&termios)), 0, 0, 0) - return err == 0 -} - -func TestIsTerminal(t *testing.T) { - if !isTerminal(1) { - t.Errorf("stdout is not a terminal") - } - if !isTerminal(2) { - t.Errorf("stderr is not a terminal") - } -} diff --git a/src/internal/testpty/pty.go b/src/internal/testpty/pty.go index 88a47cf85f..f0b2a331b8 100644 --- a/src/internal/testpty/pty.go +++ b/src/internal/testpty/pty.go @@ -2,26 +2,24 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build ((aix || dragonfly || freebsd || (linux && !android) || netbsd || openbsd) && cgo) || darwin - // Package testpty is a simple pseudo-terminal package for Unix systems, // implemented by calling C functions via cgo. package testpty import ( + "errors" "fmt" "os" - "syscall" ) type PtyError struct { FuncName string ErrorString string - Errno syscall.Errno + Errno error } func ptyError(name string, err error) *PtyError { - return &PtyError{name, err.Error(), err.(syscall.Errno)} + return &PtyError{name, err.Error(), err} } func (e *PtyError) Error() string { @@ -30,7 +28,11 @@ func (e *PtyError) Error() string { func (e *PtyError) Unwrap() error { return e.Errno } +var ErrNotSupported = errors.New("testpty.Open not implemented on this platform") + // Open returns a control pty and the name of the linked process tty. +// +// If Open is not implemented on this platform, it returns ErrNotSupported. func Open() (pty *os.File, processTTY string, err error) { return open() } diff --git a/src/internal/testpty/pty_none.go b/src/internal/testpty/pty_none.go new file mode 100644 index 0000000000..4f9e2b7c17 --- /dev/null +++ b/src/internal/testpty/pty_none.go @@ -0,0 +1,13 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build !(cgo && (aix || dragonfly || freebsd || (linux && !android) || netbsd || openbsd)) && !darwin + +package testpty + +import "os" + +func open() (pty *os.File, processTTY string, err error) { + return nil, "", ErrNotSupported +} -- 2.50.0