From bc68e99fbce5ae8aad4806768b6f06ea0c743731 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Tue, 31 Oct 2023 09:07:26 -0400 Subject: [PATCH] Minor cosmetic cleanups in the tests. Add missing error checks. Use Fatal instead of Error to avoid panics upon failure. --- doc_test.go | 193 +++++++++++++++++++++++----------------------------- io_test.go | 55 ++++++++------- 2 files changed, 116 insertions(+), 132 deletions(-) diff --git a/doc_test.go b/doc_test.go index a1d915a..3e6a3fa 100644 --- a/doc_test.go +++ b/doc_test.go @@ -7,17 +7,10 @@ import ( "testing" ) -// Will fill p from reader r +// Will fill p from reader r. func readBytes(r io.Reader, p []byte) error { - bytesRead := 0 - for bytesRead < len(p) { - n, err := r.Read(p[bytesRead:]) - if err != nil { - return err - } - bytesRead = bytesRead + n - } - return nil + _, err := io.ReadFull(r, p) + return err } func TestOpen(t *testing.T) { @@ -25,17 +18,15 @@ func TestOpen(t *testing.T) { pty, tty, err := Open() if err != nil { - t.Errorf("Unexpected error from Open: %s", err) + t.Fatalf("Unexpected error from Open: %s.", err) } - err = tty.Close() - if err != nil { - t.Errorf("Unexpected error from tty Close: %s", err) + if err := tty.Close(); err != nil { + t.Errorf("Unexpected error from tty Close: %s.", err) } - err = pty.Close() - if err != nil { - t.Errorf("Unexpected error from pty Close: %s", err) + if err := pty.Close(); err != nil { + t.Errorf("Unexpected error from pty Close: %s.", err) } } @@ -44,24 +35,22 @@ func TestName(t *testing.T) { pty, tty, err := Open() if err != nil { - t.Errorf("Unexpected error from Open: %s", err) + t.Fatalf("Unexpected error from Open: %s.", err) } // Check name isn't empty. There's variation on what exactly the OS calls these files. if pty.Name() == "" { - t.Error("pty name was empty") + t.Error("Pty name was empty.") } if tty.Name() == "" { - t.Error("tty name was empty") + t.Error("Tty name was empty.") } - err = tty.Close() - if err != nil { + if err := tty.Close(); err != nil { t.Errorf("Unexpected error from tty Close: %s", err) } - err = pty.Close() - if err != nil { + if err := pty.Close(); err != nil { t.Errorf("Unexpected error from pty Close: %s", err) } } @@ -74,30 +63,30 @@ func TestOpenByName(t *testing.T) { pty, tty, err := Open() if err != nil { - t.Fatal(err) + t.Fatalf("Error: open: %s.", err) } - defer pty.Close() - defer tty.Close() + defer func() { _ = pty.Close() }() + defer func() { _ = tty.Close() }() - ttyFile, err := os.OpenFile(tty.Name(), os.O_RDWR, 0600) + ttyFile, err := os.OpenFile(tty.Name(), os.O_RDWR, 0o600) if err != nil { - t.Fatalf("Failed to open tty file: %v", err) + t.Fatalf("Failed to open tty file: %s.", err) } - defer ttyFile.Close() + defer func() { _ = ttyFile.Close() }() // Ensure we can write to the newly opened tty file and read on the pty. text := []byte("ping") n, err := ttyFile.Write(text) if err != nil { - t.Errorf("Unexpected error from Write: %s", err) + t.Fatalf("Unexpected error from Write: %s.", err) } if n != len(text) { - t.Errorf("Unexpected count returned from Write, got %d expected %d", n, len(text)) + t.Errorf("Unexpected count returned from Write, got %d expected %d.", n, len(text)) } buffer := make([]byte, len(text)) if err := readBytes(pty, buffer); err != nil { - t.Errorf("Unexpected error from readBytes: %s", err) + t.Fatalf("Unexpected error from readBytes: %s.", err) } if !bytes.Equal(text, buffer) { t.Errorf("Unexpected result returned from Read, got %v expected %v", buffer, text) @@ -109,34 +98,32 @@ func TestGetsize(t *testing.T) { pty, tty, err := Open() if err != nil { - t.Errorf("Unexpected error from Open: %s", err) + t.Fatalf("Unexpected error from Open: %s.", err) } prows, pcols, err := Getsize(pty) if err != nil { - t.Errorf("Unexpected error from Getsize: %s", err) + t.Errorf("Unexpected error from Getsize: %s.", err) } trows, tcols, err := Getsize(tty) if err != nil { - t.Errorf("Unexpected error from Getsize: %s", err) + t.Errorf("Unexpected error from Getsize: %s.", err) } if prows != trows { - t.Errorf("pty rows != tty rows: %d != %d", prows, trows) + t.Errorf("pty rows != tty rows: %d != %d.", prows, trows) } if prows != trows { - t.Errorf("pty cols != tty cols: %d != %d", pcols, tcols) + t.Errorf("pty cols != tty cols: %d != %d.", pcols, tcols) } - err = tty.Close() - if err != nil { - t.Errorf("Unexpected error from tty Close: %s", err) + if err := tty.Close(); err != nil { + t.Errorf("Unexpected error from tty Close: %s.", err) } - err = pty.Close() - if err != nil { - t.Errorf("Unexpected error from pty Close: %s", err) + if err := pty.Close(); err != nil { + t.Errorf("Unexpected error from pty Close: %s.", err) } } @@ -145,40 +132,38 @@ func TestGetsizefull(t *testing.T) { pty, tty, err := Open() if err != nil { - t.Errorf("Unexpected error from Open: %s", err) + t.Fatalf("Unexpected error from Open: %s.", err) } psize, err := GetsizeFull(pty) if err != nil { - t.Errorf("Unexpected error from GetsizeFull: %s", err) + t.Fatalf("Unexpected error from GetsizeFull: %s.", err) } tsize, err := GetsizeFull(tty) if err != nil { - t.Errorf("Unexpected error from GetsizeFull: %s", err) + t.Fatalf("Unexpected error from GetsizeFull: %s.", err) } if psize.X != tsize.X { - t.Errorf("pty x != tty x: %d != %d", psize.X, tsize.X) + t.Errorf("pty x != tty x: %d != %d.", psize.X, tsize.X) } if psize.Y != tsize.Y { - t.Errorf("pty y != tty y: %d != %d", psize.Y, tsize.Y) + t.Errorf("pty y != tty y: %d != %d.", psize.Y, tsize.Y) } if psize.Rows != tsize.Rows { - t.Errorf("pty rows != tty rows: %d != %d", psize.Rows, tsize.Rows) + t.Errorf("pty rows != tty rows: %d != %d.", psize.Rows, tsize.Rows) } if psize.Cols != tsize.Cols { - t.Errorf("pty cols != tty cols: %d != %d", psize.Cols, tsize.Cols) + t.Errorf("pty cols != tty cols: %d != %d.", psize.Cols, tsize.Cols) } - err = tty.Close() - if err != nil { - t.Errorf("Unexpected error from tty Close: %s", err) + if err := tty.Close(); err != nil { + t.Errorf("Unexpected error from tty Close: %s.", err) } - err = pty.Close() - if err != nil { - t.Errorf("Unexpected error from pty Close: %s", err) + if err := pty.Close(); err != nil { + t.Errorf("Unexpected error from pty Close: %s.", err) } } @@ -187,12 +172,12 @@ func TestSetsize(t *testing.T) { pty, tty, err := Open() if err != nil { - t.Errorf("Unexpected error from Open: %s", err) + t.Fatalf("Unexpected error from Open: %s.", err) } psize, err := GetsizeFull(pty) if err != nil { - t.Errorf("Unexpected error from GetsizeFull: %s", err) + t.Fatalf("Unexpected error from GetsizeFull: %s.", err) } psize.X = psize.X + 1 @@ -200,37 +185,34 @@ func TestSetsize(t *testing.T) { psize.Rows = psize.Rows + 1 psize.Cols = psize.Cols + 1 - err = Setsize(tty, psize) - if err != nil { - t.Errorf("Unexpected error from Setsize: %s", err) + if err := Setsize(tty, psize); err != nil { + t.Fatalf("Unexpected error from Setsize: %s", err) } tsize, err := GetsizeFull(tty) if err != nil { - t.Errorf("Unexpected error from GetsizeFull: %s", err) + t.Fatalf("Unexpected error from GetsizeFull: %s.", err) } if psize.X != tsize.X { - t.Errorf("pty x != tty x: %d != %d", psize.X, tsize.X) + t.Errorf("pty x != tty x: %d != %d.", psize.X, tsize.X) } if psize.Y != tsize.Y { - t.Errorf("pty y != tty y: %d != %d", psize.Y, tsize.Y) + t.Errorf("pty y != tty y: %d != %d.", psize.Y, tsize.Y) } if psize.Rows != tsize.Rows { - t.Errorf("pty rows != tty rows: %d != %d", psize.Rows, tsize.Rows) + t.Errorf("pty rows != tty rows: %d != %d.", psize.Rows, tsize.Rows) } if psize.Cols != tsize.Cols { - t.Errorf("pty cols != tty cols: %d != %d", psize.Cols, tsize.Cols) + t.Errorf("pty cols != tty cols: %d != %d.", psize.Cols, tsize.Cols) } - err = tty.Close() - if err != nil { - t.Errorf("Unexpected error from tty Close: %s", err) + if err := tty.Close(); err != nil { + t.Errorf("Unexpected error from tty Close: %s.", err) } - err = pty.Close() - if err != nil { - t.Errorf("Unexpected error from pty Close: %s", err) + if err := pty.Close(); err != nil { + t.Errorf("Unexpected error from pty Close: %s.", err) } } @@ -239,7 +221,7 @@ func TestReadWriteText(t *testing.T) { pty, tty, err := Open() if err != nil { - t.Errorf("Unexpected error from Open: %s", err) + t.Fatalf("Unexpected error from Open: %s.", err) } // Write to tty, read from pty @@ -249,16 +231,15 @@ func TestReadWriteText(t *testing.T) { t.Errorf("Unexpected error from Write: %s", err) } if n != len(text) { - t.Errorf("Unexpected count returned from Write, got %d expected %d", n, len(text)) + t.Errorf("Unexpected count returned from Write, got %d expected %d.", n, len(text)) } buffer := make([]byte, 4) - err = readBytes(pty, buffer) - if err != nil { - t.Errorf("Unexpected error from readBytes: %s", err) + if err := readBytes(pty, buffer); err != nil { + t.Fatalf("Unexpected error from readBytes: %s.", err) } if !bytes.Equal(text, buffer) { - t.Errorf("Unexpected result returned from Read, got %v expected %v", buffer, text) + t.Errorf("Unexpected result returned from Read, got %v expected %v.", buffer, text) } // Write to pty, read from tty. @@ -266,41 +247,41 @@ func TestReadWriteText(t *testing.T) { text = []byte("pong\n") n, err = pty.Write(text) if err != nil { - t.Errorf("Unexpected error from Write: %s", err) + t.Fatalf("Unexpected error from Write: %s.", err) } if n != len(text) { - t.Errorf("Unexpected count returned from Write, got %d expected %d", n, len(text)) + t.Errorf("Unexpected count returned from Write, got %d expected %d.", n, len(text)) } buffer = make([]byte, 5) err = readBytes(tty, buffer) if err != nil { - t.Errorf("Unexpected error from readBytes: %s", err) + t.Errorf("Unexpected error from readBytes: %s.", err) } expect := []byte("pong\n") if !bytes.Equal(expect, buffer) { - t.Errorf("Unexpected result returned from Read, got %v expected %v", buffer, expect) + t.Errorf("Unexpected result returned from Read, got %v expected %v.", buffer, expect) } // Read the echo back from pty buffer = make([]byte, 5) err = readBytes(pty, buffer) if err != nil { - t.Errorf("Unexpected error from readBytes: %s", err) + t.Fatalf("Unexpected error from readBytes: %s.", err) } expect = []byte("pong\r") if !bytes.Equal(expect, buffer) { - t.Errorf("Unexpected result returned from Read, got %v expected %v", buffer, expect) + t.Errorf("Unexpected result returned from Read, got %v expected %v.", buffer, expect) } err = tty.Close() if err != nil { - t.Errorf("Unexpected error from tty Close: %s", err) + t.Errorf("Unexpected error from tty Close: %s.", err) } err = pty.Close() if err != nil { - t.Errorf("Unexpected error from pty Close: %s", err) + t.Errorf("Unexpected error from pty Close: %s.", err) } } @@ -309,66 +290,64 @@ func TestReadWriteControls(t *testing.T) { pty, tty, err := Open() if err != nil { - t.Errorf("Unexpected error from Open: %s", err) + t.Fatalf("Unexpected error from Open: %s.", err) } - // Write the start of a line to pty + // Write the start of a line to pty. text := []byte("pind") n, err := pty.Write(text) if err != nil { - t.Errorf("Unexpected error from Write: %s", err) + t.Fatalf("Unexpected error from Write: %s.", err) } if n != len(text) { - t.Errorf("Unexpected count returned from Write, got %d expected %d", n, len(text)) + t.Errorf("Unexpected count returned from Write, got %d expected %d.", n, len(text)) } - // Backspace that last char + // Backspace that last char. n, err = pty.Write([]byte("\b")) if err != nil { - t.Errorf("Unexpected error from Write: %s", err) + t.Fatalf("Unexpected error from Write: %s.", err) } if n != 1 { - t.Errorf("Unexpected count returned from Write, got %d expected %d", n, 1) + t.Errorf("Unexpected count returned from Write, got %d expected %d.", n, 1) } - // Write the correct char and a CR + // Write the correct char and a CR. n, err = pty.Write([]byte("g\n")) if err != nil { - t.Errorf("Unexpected error from Write: %s", err) + t.Fatalf("Unexpected error from Write: %s.", err) } if n != 2 { - t.Errorf("Unexpected count returned from Write, got %d expected %d", n, 2) + t.Errorf("Unexpected count returned from Write, got %d expected %d.", n, 2) } // Read the line buffer := make([]byte, 7) err = readBytes(tty, buffer) if err != nil { - t.Errorf("Unexpected error from readBytes: %s", err) + t.Errorf("Unexpected error from readBytes: %s.", err) } expect := []byte("pind\bg\n") if !bytes.Equal(expect, buffer) { - t.Errorf("Unexpected result returned from Read, got %v expected %v", buffer, expect) + t.Errorf("Unexpected result returned from Read, got %v expected %v.", buffer, expect) } // Read the echo back from pty buffer = make([]byte, 7) err = readBytes(pty, buffer) if err != nil { - t.Errorf("Unexpected error from readBytes: %s", err) + t.Errorf("Unexpected error from readBytes: %s.", err) } expect = []byte("pind^Hg") if !bytes.Equal(expect, buffer) { - t.Errorf("Unexpected result returned from Read, got %v expected %v", buffer, expect) + t.Errorf("Unexpected result returned from Read, got %v expected %v.", buffer, expect) } - err = tty.Close() - if err != nil { - t.Errorf("Unexpected error from tty Close: %s", err) + if err := tty.Close(); err != nil { + t.Errorf("Unexpected error from tty Close: %s.", err) } - err = pty.Close() - if err != nil { - t.Errorf("Unexpected error from pty Close: %s", err) + if err := pty.Close(); err != nil { + t.Errorf("Unexpected error from pty Close: %s.", err) } } diff --git a/io_test.go b/io_test.go index 0837d9e..661559e 100644 --- a/io_test.go +++ b/io_test.go @@ -4,13 +4,12 @@ package pty import ( - "testing" - "context" "errors" "os" "runtime" "sync" + "testing" "time" ) @@ -19,7 +18,7 @@ const ( timeout = time.Second ) -var mu sync.Mutex +var glTestFdLock sync.Mutex // Check that SetDeadline() works for ptmx. // Outstanding Read() calls must be interrupted by deadline. @@ -28,25 +27,27 @@ var mu sync.Mutex func TestReadDeadline(t *testing.T) { ptmx, success := prepare(t) - err := ptmx.SetDeadline(time.Now().Add(timeout / 10)) - if err != nil { + if err := ptmx.SetDeadline(time.Now().Add(timeout / 10)); err != nil { if errors.Is(err, os.ErrNoDeadline) { - t.Skipf("deadline is not supported on %s/%s", runtime.GOOS, runtime.GOARCH) + t.Skipf("Deadline is not supported on %s/%s.", runtime.GOOS, runtime.GOARCH) } else { - t.Fatalf("error: set deadline: %v\n", err) + t.Fatalf("Error: set deadline: %s.", err) } } - var buf = make([]byte, 1) + buf := make([]byte, 1) n, err := ptmx.Read(buf) success() + if err != nil && !errors.Is(err, os.ErrDeadlineExceeded) { + t.Fatalf("Unexpected read error: %s.", err) + } if n != 0 && buf[0] != errMarker { - t.Errorf("received unexpected data from pmtx (%d bytes): 0x%X; err=%v", n, buf, err) + t.Errorf("Received unexpected data from pmtx (%d bytes): 0x%X; err=%v.", n, buf, err) } } -// Check that ptmx.Close() interrupts outstanding ptmx.Read() calls +// Check that ptmx.Close() interrupts outstanding ptmx.Read() calls. // // https://github.com/creack/pty/issues/114 // https://github.com/creack/pty/issues/88 @@ -55,23 +56,27 @@ func TestReadClose(t *testing.T) { go func() { time.Sleep(timeout / 10) - err := ptmx.Close() - if err != nil { - t.Errorf("failed to close ptmx: %v", err) + if err := ptmx.Close(); err != nil { + t.Errorf("Failed to close ptmx: %s.", err) } }() - var buf = make([]byte, 1) + buf := make([]byte, 1) n, err := ptmx.Read(buf) success() + if err != nil && !errors.Is(err, os.ErrClosed) { + t.Fatalf("Unexpected read error: %s.", err) + } if n != 0 && buf[0] != errMarker { - t.Errorf("received unexpected data from pmtx (%d bytes): 0x%X; err=%v", n, buf, err) + t.Errorf("Received unexpected data from pmtx (%d bytes): 0x%X; err=%v.", n, buf, err) } } // Open pty and setup watchdogs for graceful and not so graceful failure modes func prepare(t *testing.T) (ptmx *os.File, done func()) { + t.Helper() + if runtime.GOOS == "darwin" { t.Log("creack/pty uses blocking i/o on darwin intentionally:") t.Log("> https://github.com/creack/pty/issues/52") @@ -81,13 +86,13 @@ func prepare(t *testing.T) (ptmx *os.File, done func()) { } // Due to data race potential in (*os.File).Fd() - // we should never run these two tests in parallel - mu.Lock() - t.Cleanup(mu.Unlock) + // we should never run these two tests in parallel. + glTestFdLock.Lock() + t.Cleanup(glTestFdLock.Unlock) ptmx, pts, err := Open() if err != nil { - t.Fatalf("error: open: %v\n", err) + t.Fatalf("Error: open: %s.\n", err) } t.Cleanup(func() { _ = ptmx.Close() }) t.Cleanup(func() { _ = pts.Close() }) @@ -99,21 +104,21 @@ func prepare(t *testing.T) (ptmx *os.File, done func()) { case <-ctx.Done(): // ptmx.Read() did not block forever, yay! case <-time.After(timeout): - _, err := pts.Write([]byte{errMarker}) // unblock ptmx.Read() - if err != nil { - t.Errorf("failed to write to pts: %v", err) + if _, err := pts.Write([]byte{errMarker}); err != nil { // unblock ptmx.Read() + t.Errorf("Failed to write to pts: %s.", err) } - t.Error("ptmx.Read() was not unblocked") + t.Error("ptmx.Read() was not unblocked.") done() // cancel panic() } }() go func() { select { case <-ctx.Done(): - // Test has either failed or succeeded; it definitely did not hang + // Test has either failed or succeeded; it definitely did not hang. case <-time.After(timeout * 10 / 9): // timeout +11% - panic("ptmx.Read() was not unblocked; avoid hanging forever") // just in case + panic("ptmx.Read() was not unblocked; avoid hanging forever.") // Just in case. } }() + return ptmx, done }