]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: make testterminal18153 a normal test
authorAustin Clements <austin@google.com>
Fri, 11 Nov 2022 16:51:28 +0000 (11:51 -0500)
committerAustin Clements <austin@google.com>
Wed, 16 Nov 2022 19:00:20 +0000 (19:00 +0000)
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 <austin@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

src/cmd/dist/test.go
src/cmd/dist/test_linux.go [deleted file]
src/cmd/go/terminal_test.go [new file with mode: 0644]
src/cmd/go/testdata/testterminal18153/terminal_test.go [deleted file]
src/internal/testpty/pty.go
src/internal/testpty/pty_none.go [new file with mode: 0644]

index 1d1f325bc77abbd48f94a08f5fd0014f97207f7b..3e30bccd5acae3ed2af6712b73454364b5ed8ec8 100644 (file)
@@ -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 (file)
index 43d28dc..0000000
+++ /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 (file)
index 0000000..03ca772
--- /dev/null
@@ -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 (file)
index 34ee580..0000000
+++ /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")
-       }
-}
index 88a47cf85ff5c9e69e739afa663dbecf13dc77e0..f0b2a331b8d801516306c4fc499237dfd1fd2a4f 100644 (file)
@@ -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 (file)
index 0000000..4f9e2b7
--- /dev/null
@@ -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
+}