From bc68e99fbce5ae8aad4806768b6f06ea0c743731 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Tue, 31 Oct 2023 09:07:26 -0400 Subject: [PATCH 1/5] 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 } From ccf8e7e6b689fcf5142a9a1b39f2a5b173fba9e0 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Tue, 31 Oct 2023 09:20:53 -0400 Subject: [PATCH 2/5] Add openClose helper in tests to remove duplication. --- doc_test.go | 117 ++++++++++++---------------------------------------- 1 file changed, 27 insertions(+), 90 deletions(-) diff --git a/doc_test.go b/doc_test.go index 3e6a3fa..6e5916e 100644 --- a/doc_test.go +++ b/doc_test.go @@ -13,30 +13,37 @@ func readBytes(r io.Reader, p []byte) error { return err } -func TestOpen(t *testing.T) { - t.Parallel() +// openCloses opens a pty/tty pair and stages the closing as part of the cleanup. +func openClose(t *testing.T) (pty, tty *os.File) { + t.Helper() pty, tty, err := Open() if err != nil { t.Fatalf("Unexpected error from Open: %s.", err) } + t.Cleanup(func() { + if err := tty.Close(); 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) - } + if err := pty.Close(); 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) - } + return pty, tty +} + +func TestOpen(t *testing.T) { + t.Parallel() + + openClose(t) } func TestName(t *testing.T) { t.Parallel() - pty, tty, err := Open() - if err != nil { - t.Fatalf("Unexpected error from Open: %s.", err) - } + pty, tty := openClose(t) // Check name isn't empty. There's variation on what exactly the OS calls these files. if pty.Name() == "" { @@ -45,14 +52,6 @@ func TestName(t *testing.T) { if tty.Name() == "" { t.Error("Tty name was empty.") } - - if err := tty.Close(); err != nil { - t.Errorf("Unexpected error from tty Close: %s", err) - } - - if err := pty.Close(); err != nil { - t.Errorf("Unexpected error from pty Close: %s", err) - } } // TestOpenByName ensures that the name associated with the tty is valid @@ -61,12 +60,7 @@ func TestName(t *testing.T) { func TestOpenByName(t *testing.T) { t.Parallel() - pty, tty, err := Open() - if err != nil { - t.Fatalf("Error: open: %s.", err) - } - defer func() { _ = pty.Close() }() - defer func() { _ = tty.Close() }() + pty, tty := openClose(t) ttyFile, err := os.OpenFile(tty.Name(), os.O_RDWR, 0o600) if err != nil { @@ -96,10 +90,7 @@ func TestOpenByName(t *testing.T) { func TestGetsize(t *testing.T) { t.Parallel() - pty, tty, err := Open() - if err != nil { - t.Fatalf("Unexpected error from Open: %s.", err) - } + pty, tty := openClose(t) prows, pcols, err := Getsize(pty) if err != nil { @@ -117,23 +108,12 @@ func TestGetsize(t *testing.T) { if prows != trows { t.Errorf("pty cols != tty cols: %d != %d.", pcols, tcols) } - - if err := tty.Close(); err != nil { - t.Errorf("Unexpected error from tty Close: %s.", err) - } - - if err := pty.Close(); err != nil { - t.Errorf("Unexpected error from pty Close: %s.", err) - } } func TestGetsizefull(t *testing.T) { t.Parallel() - pty, tty, err := Open() - if err != nil { - t.Fatalf("Unexpected error from Open: %s.", err) - } + pty, tty := openClose(t) psize, err := GetsizeFull(pty) if err != nil { @@ -157,23 +137,12 @@ func TestGetsizefull(t *testing.T) { if psize.Cols != tsize.Cols { t.Errorf("pty cols != tty cols: %d != %d.", psize.Cols, tsize.Cols) } - - if err := tty.Close(); err != nil { - t.Errorf("Unexpected error from tty Close: %s.", err) - } - - if err := pty.Close(); err != nil { - t.Errorf("Unexpected error from pty Close: %s.", err) - } } func TestSetsize(t *testing.T) { t.Parallel() - pty, tty, err := Open() - if err != nil { - t.Fatalf("Unexpected error from Open: %s.", err) - } + pty, tty := openClose(t) psize, err := GetsizeFull(pty) if err != nil { @@ -206,29 +175,18 @@ func TestSetsize(t *testing.T) { if psize.Cols != tsize.Cols { t.Errorf("pty cols != tty cols: %d != %d.", psize.Cols, tsize.Cols) } - - if err := tty.Close(); err != nil { - t.Errorf("Unexpected error from tty Close: %s.", err) - } - - if err := pty.Close(); err != nil { - t.Errorf("Unexpected error from pty Close: %s.", err) - } } func TestReadWriteText(t *testing.T) { t.Parallel() - pty, tty, err := Open() - if err != nil { - t.Fatalf("Unexpected error from Open: %s.", err) - } + pty, tty := openClose(t) // Write to tty, read from pty text := []byte("ping") n, err := tty.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)) @@ -256,7 +214,7 @@ func TestReadWriteText(t *testing.T) { buffer = make([]byte, 5) err = readBytes(tty, buffer) if err != nil { - t.Errorf("Unexpected error from readBytes: %s.", err) + t.Fatalf("Unexpected error from readBytes: %s.", err) } expect := []byte("pong\n") if !bytes.Equal(expect, buffer) { @@ -273,25 +231,12 @@ func TestReadWriteText(t *testing.T) { if !bytes.Equal(expect, buffer) { 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) - } - - err = pty.Close() - if err != nil { - t.Errorf("Unexpected error from pty Close: %s.", err) - } } func TestReadWriteControls(t *testing.T) { t.Parallel() - pty, tty, err := Open() - if err != nil { - t.Fatalf("Unexpected error from Open: %s.", err) - } + pty, tty := openClose(t) // Write the start of a line to pty. text := []byte("pind") @@ -342,12 +287,4 @@ func TestReadWriteControls(t *testing.T) { if !bytes.Equal(expect, buffer) { t.Errorf("Unexpected result returned from Read, got %v expected %v.", buffer, expect) } - - if err := tty.Close(); err != nil { - t.Errorf("Unexpected error from tty Close: %s.", err) - } - - if err := pty.Close(); err != nil { - t.Errorf("Unexpected error from pty Close: %s.", err) - } } From 219f7e0db96efd51edb48aae71bf77a427209710 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Tue, 31 Oct 2023 10:46:02 -0400 Subject: [PATCH 3/5] Add more test helpers, cleanup existing tests. Add editorconfig and golangci config, comply with golangci rules. --- .editorconfig | 54 ++++++++ .golangci.yml | 324 +++++++++++++++++++++++++++++++++++++++++++ doc_test.go | 262 ++++++++++------------------------ helpers_test.go | 62 +++++++++ io_test.go | 13 +- ioctl.go | 4 +- ioctl_inner.go | 3 +- ioctl_legacy.go | 2 +- ioctl_solaris.go | 2 +- ioctl_unsupported.go | 2 +- pty_linux.go | 2 +- winsize.go | 5 +- winsize_unix.go | 8 +- 13 files changed, 534 insertions(+), 209 deletions(-) create mode 100644 .editorconfig create mode 100644 .golangci.yml create mode 100644 helpers_test.go diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000..349f67a --- /dev/null +++ b/.editorconfig @@ -0,0 +1,54 @@ +root = true + +# Sane defaults. +[*] +# Always use unix end of line. +end_of_line = lf +# Always insert a new line at the end of files. +insert_final_newline = true +# Don't leave trailing whitespaces. +trim_trailing_whitespace = true +# Default to utf8 encoding. +charset = utf-8 +# Space > tab for consistent aligns. +indent_style = space +# Default to 2 spaces for indent/tabs. +indent_size = 2 +# Flag long lines. +max_line_length = 140 + +# Explicitly define settings for commonly used files. + +[*.go] +indent_style = tab +indent_size = 8 + +[*.feature] +indent_style = space +indent_size = 2 + +[*.json] +indent_style = space +indent_size = 2 + +[*.{yml,yaml}] +indent_style = space +indent_size = 2 + +[*.tf] +indent_style = space +indent_size = 2 + +[*.md] +# Don't check line lenghts in files. +max_line_length = 0 + +[{Makefile,*.mk}] +indent_style = tab +indent_size = 8 + +[{Dockerfile,Dockerfile.*}] +indent_size = 4 + +[*.sql] +indent_size = 2 diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..f023e0f --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,324 @@ +--- +# Reference: https://golangci-lint.run/usage/configuration/ +run: + timeout: 5m + # modules-download-mode: vendor + + # Include test files. + tests: true + + skip-dirs: [] + + skip-files: [] + +output: + # colored-line-number|line-number|json|tab|checkstyle|code-climate, default is "colored-line-number". + format: colored-line-number + print-issued-lines: true + print-linter-name: true + +# Linter specific settings. See below in the `linter.enable` section for details on what each linter is doing. +linters-settings: + dogsled: + # Checks assignments with too many blank identifiers. Default is 2. + max-blank-identifiers: 2 + + dupl: + # Tokens count to trigger issue. + threshold: 150 + + errcheck: + # Report about not checking of errors in type assertions: `a := b.(MyStruct)`. + # Enabled as this is often overlooked by developers. + check-type-assertions: true + # Report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`. + # Disabled as we consider that if the developer did type `_`, it was on purpose. + # Note that while this isn't enforced by the linter, each and every case of ignored error should + # be accompanied with a comment explaining why that error is being discarded. + check-blank: false + + exhaustive: + # Indicates that switch statements are to be considered exhaustive if a + # 'default' case is present, even if all enum members aren't listed in the + # switch. + default-signifies-exhaustive: false + + funlen: + # funlen checks the number of lines/statements in a function. + # While is is always best to keep functions short for readability, maintainability and testing, + # the default are a bit too strict (60 lines / 40 statements), increase it to be more flexible. + lines: 160 + statements: 70 + + # NOTE: We don't set `gci` for import order as it supports only one prefix. Use `goimports.local-prefixes` instead. + + gocognit: + # Minimal code complexity to report, defaults to 30 in gocognit, defaults 10 in golangci. + # Use 15 as it allows for some flexibility while preventing too much complexity. + # NOTE: Similar to gocyclo. + min-complexity: 35 + + nestif: + # Minimal complexity of if statements to report. + min-complexity: 8 + + goconst: + # Minimal length of string constant. + min-len: 4 + # Minimal occurrences count to trigger. + # Increase the default from 3 to 5 as small number of const usage can reduce readability instead of improving it. + min-occurrences: 5 + + gocritic: + # Which checks should be disabled; can't be combined with 'enabled-checks'. + # See https://go-critic.github.io/overview#checks-overview + # To check which checks are enabled run `GL_DEBUG=gocritic golangci-lint run` + disabled-checks: + - hugeParam # Very strict check on the size of variables being copied. Too strict for most developer. + # Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint run` to see all tags and checks. + # Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags". + enabled-tags: + - diagnostic + - style + - opinionated + - performance + settings: + rangeValCopy: + sizeThreshold: 1024 # Increase the allowed copied bytes in range. + + cyclop: + max-complexity: 35 + + gocyclo: + # Similar check as gocognit. + # NOTE: We might be able to remove this linter as it is redundant with gocyclo. It is in golangci-lint, so we keep it for now. + min-complexity: 35 + + godot: + # Check all top-level comments, not only declarations. + check-all: true + + gofmt: + # simplify code: gofmt with `-s` option. + simplify: true + + # NOTE: the goheader settings are set per-project. + + goimports: + # Put imports beginning with prefix after 3rd-party packages. + # It's a comma-separated list of prefixes. + local-prefixes: "github.com/creack/pty" + + golint: + # Minimal confidence for issues, default is 0.8. + min-confidence: 0.8 + + gosimple: + # Select the Go version to target. The default is '1.13'. + go: "1.18" + # https://staticcheck.io/docs/options#checks + checks: ["all"] + + gosec: + + govet: + # Enable all available checks from go vet. + enable-all: false + # Report about shadowed variables. + check-shadowing: true + + # NOTE: depguard is disabled as it is very slow and made redundant by gomodguard. + + lll: + # Make sure everyone is on the same level, fix the tab width to go's default. + tab-width: 8 + # Increase the default max line length to give more flexibility. Forcing newlines can reduce readability instead of improving it. + line-length: 180 + + misspell: + locale: US + ignore-words: + + nakedret: + # Make an issue if func has more lines of code than this setting and it has naked returns; default is 30. + # NOTE: Consider setting this to 1 to prevent naked returns. + max-func-lines: 30 + + nolintlint: + # Prevent ununsed directive to avoid stale comments. + allow-unused: false + # Require an explanation of nonzero length after each nolint directive. + require-explanation: true + # Exclude following linters from requiring an explanation. + # NOTE: It is strongly discouraged to put anything in there. + allow-no-explanation: [] + # Enable to require nolint directives to mention the specific linter being suppressed. This ensurce the developer understand the reason being the error. + require-specific: true + + prealloc: + # NOTE: For most programs usage of prealloc will be a premature optimization. + # Keep thing simple, pre-alloc what is obvious and profile the program for more complex scenarios. + # + simple: true # Checkonly on simple loops that have no returns/breaks/continues/gotos in them. + range-loops: true # Check range loops, true by default + for-loops: false # Check suggestions on for loops, false by default + + rowserrcheck: + packages: [] + + staticcheck: + # Select the Go version to target. The default is '1.13'. + go: "1.18" + # https://staticcheck.io/docs/options#checks + checks: ["all"] + + stylecheck: + # Select the Go version to target. The default is '1.13'. + go: "1.18" + # https://staticcheck.io/docs/options#checks + checks: ["all"] # "-ST1000", "-ST1003", "-ST1016", "-ST1020", "-ST1021", "-ST1022"] + + tagliatelle: + # Check the struck tag name case. + case: + # Use the struct field name to check the name of the struct tag. + use-field-name: false + rules: + # Any struct tag type can be used. + # support string case: `camel`, `pascal`, `kebab`, `snake`, `goCamel`, `goPascal`, `goKebab`, `goSnake`, `upper`, `lower` + json: snake + firestore: camel + yaml: camel + xml: camel + bson: camel + avro: snake + mapstructure: kebab + envconfig: upper + + unparam: + # Don't create an error if an exported code have static params being used. It is often expected in libraries. + # NOTE: It would be nice if this linter would differentiate between a main package and a lib. + check-exported: true + + unused: {} + + whitespace: + multi-if: false # Enforces newlines (or comments) after every multi-line if statement + multi-func: false # Enforces newlines (or comments) after every multi-line function signature + +# Run `golangci-lint help linters` to get the full list of linter with their description. +linters: + disable-all: true + # NOTE: enable-all is deprecated because too many people don't pin versions... + # We still require explicit documentation on why some linters are disabled. + # disable: + # - depguard # Go linter that checks if package imports are in a list of acceptable packages [fast: true, auto-fix: false] + # - exhaustivestruct # Checks if all struct's fields are initialized [fast: true, auto-fix: false] + # - forbidigo # Forbids identifiers [fast: true, auto-fix: false] + # - gci # Gci control golang package import order and make it always deterministic. [fast: true, auto-fix: true] + # - godox # Tool for detection of FIXME, TODO and other comment keywords [fast: true, auto-fix: false] + # - goerr113 # Golang linter to check the errors handling expressions [fast: true, auto-fix: false] + # - golint # Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes [fast: false, auto-fix: false] + # - gomnd # An analyzer to detect magic numbers. [fast: true, auto-fix: false] + # - gomoddirectives # Manage the use of 'replace', 'retract', and 'excludes' directives in go.mod. [fast: true, auto-fix: false] + # - interfacer # Linter that suggests narrower interface types [fast: false, auto-fix: false] + # - maligned # Tool to detect Go structs that would take less memory if their fields were sorted [fast: false, auto-fix: false] + # - nlreturn # nlreturn checks for a new line before return and branch statements to increase code clarity [fast: true, auto-fix: false] + # - scopelint # Scopelint checks for unpinned variables in go programs [fast: true, auto-fix: false] + # - wrapcheck # Checks that errors returned from external packages are wrapped [fast: false, auto-fix: false] + # - wsl # Whitespace Linter - Forces you to use empty lines! [fast: true, auto-fix: false] + + # disable-reasons: + # - depguard # Checks whitelisted/blacklisted import path, but runs way too slow. Not that useful. + # - exhaustivestruct # Good concept, but not mature enough (errors on not assignable fields like locks) and too noisy when using AWS SDK as most fields are unused. + # - forbidigo # Great idea, but too strict out of the box. Probably will re-enable soon. + # - gci # Conflicts with goimports/gofumpt. + # - godox # Don't fail when finding TODO, FIXME, etc. + # - goerr113 # Too many false positives. + # - golint # Deprecated (since v1.41.0) due to: The repository of the linter has been archived by the owner. Replaced by revive. + # - gomnd # Checks for magic numbers. Disabled due to too many false positives not configurable (03/01/2020 v1.23.7). + # - gomoddirectives # Doesn't support //nolint to whitelist. + # - interfacer # Deprecated (since v1.38.0) due to: The repository of the linter has been archived by the owner. + # - maligned # Deprecated (since v1.38.0) due to: The repository of the linter has been archived by the owner. Replaced by govet 'fieldalignment'. + # - nlreturn # Actually reduces readability in most cases. + # - scopelint # Deprecated (since v1.39.0) due to: The repository of the linter has been deprecated by the owner. Replaced by exportloopref. + # - wrapcheck # Good concept, but always warns for http coded errors. Need to re-enable and whitelist our error package. + # - wsl # Forces to add newlines around blocks. Lots of false positives, not that useful. + + enable: + - asciicheck # Simple linter to check that your code does not contain non-ASCII identifiers [fast: true, auto-fix: false] + - bodyclose # checks whether HTTP response body is closed successfully [fast: false, auto-fix: false] + - cyclop # checks function and package cyclomatic complexity [fast: false, auto-fix: false] + - dogsled # Checks assignments with too many blank identifiers (e.g. x, _, _, _, := f()) [fast: true, auto-fix: false] + - dupl # Tool for code clone detection [fast: true, auto-fix: false] + - durationcheck # check for two durations multiplied together [fast: false, auto-fix: false] + - errcheck # Errcheck is a program for checking for unchecked errors in go programs. These unchecked errors can be critical bugs in some cases [fast: false, auto-fix: false] + - errname # Checks that sentinel errors are prefixed with the `Err` and error types are suffixed with the `Error`. [fast: false, auto-fix: false] + - errorlint # go-errorlint is a source code linter for Go software that can be used to find code that will cause problems with the error wrapping scheme introduced in Go 1.13. [fast: false, auto-fix: false] + - exhaustive # check exhaustiveness of enum switch statements [fast: false, auto-fix: false] + - exportloopref # checks for pointers to enclosing loop variables [fast: false, auto-fix: false] + - forcetypeassert # finds forced type assertions [fast: true, auto-fix: false] + - funlen # Tool for detection of long functions [fast: true, auto-fix: false] + - gochecknoglobals # check that no global variables exist [fast: true, auto-fix: false] + - gochecknoinits # Checks that no init functions are present in Go code [fast: true, auto-fix: false] + - gocognit # Computes and checks the cognitive complexity of functions [fast: true, auto-fix: false] + - goconst # Finds repeated strings that could be replaced by a constant [fast: true, auto-fix: false] + - gocritic # Provides many diagnostics that check for bugs, performance and style issues. [fast: false, auto-fix: false] + - gocyclo # Computes and checks the cyclomatic complexity of functions [fast: true, auto-fix: false] + - godot # Check if comments end in a period [fast: true, auto-fix: true] + - gofmt # Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true, auto-fix: true] + - gofumpt # Gofumpt checks whether code was gofumpt-ed. [fast: true, auto-fix: true] + - goheader # Checks is file header matches to pattern [fast: true, auto-fix: false] + - goimports # Goimports does everything that gofmt does. Additionally it checks unused imports [fast: true, auto-fix: true] + - gomodguard # Allow and block list linter for direct Go module dependencies. This is different from depguard where there are different block types for example version constraints and module recommendations. [fast: true, auto-fix: false] + - goprintffuncname # Checks that printf-like functions are named with `f` at the end [fast: true, auto-fix: false] + - gosec # (gas): Inspects source code for security problems [fast: false, auto-fix: false] + - gosimple # (megacheck): Linter for Go source code that specializes in simplifying a code [fast: false, auto-fix: false] + - govet # (vet, vetshadow): Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string [fast: false, auto-fix: false] + - importas # Enforces consistent import aliases [fast: false, auto-fix: false] + - ineffassign # Detects when assignments to existing variables are not used [fast: true, auto-fix: false] + - lll # Reports long lines [fast: true, auto-fix: false] + - makezero # Finds slice declarations with non-zero initial length [fast: false, auto-fix: false] + - misspell # Finds commonly misspelled English words in comments [fast: true, auto-fix: true] + - nakedret # Finds naked returns in functions greater than a specified function length [fast: true, auto-fix: false] + - nestif # Reports deeply nested if statements [fast: true, auto-fix: false] + - nilerr # Finds the code that returns nil even if it checks that the error is not nil. [fast: false, auto-fix: false] + - noctx # noctx finds sending http request without context.Context [fast: false, auto-fix: false] + - nolintlint # Reports ill-formed or insufficient nolint directives [fast: true, auto-fix: false] + - paralleltest # paralleltest detects missing usage of t.Parallel() method in your Go test [fast: true, auto-fix: false] + - prealloc # Finds slice declarations that could potentially be preallocated [fast: true, auto-fix: false] + - predeclared # find code that shadows one of Go's predeclared identifiers [fast: true, auto-fix: false] + - promlinter # Check Prometheus metrics naming via promlint [fast: true, auto-fix: false] + - revive # Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint. [fast: false, auto-fix: false] + # Disabled due to generic. Work in progress upstream. + # - rowserrcheck # checks whether Err of rows is checked successfully [fast: false, auto-fix: false] + # Disabled due to generic. Work in progress upstream. + # - sqlclosecheck # Checks that sql.Rows and sql.Stmt are closed. [fast: false, auto-fix: false] + - staticcheck # (megacheck): Staticcheck is a go vet on steroids, applying a ton of static analysis checks [fast: false, auto-fix: false] + - stylecheck # Stylecheck is a replacement for golint [fast: false, auto-fix: false] + # Disabled due to generic. Work in progress upstream. + # - tagliatelle # Checks the struct tags. [fast: true, auto-fix: false] + # - testpackage # linter that makes you use a separate _test package [fast: true, auto-fix: false] + - thelper # thelper detects golang test helpers without t.Helper() call and checks the consistency of test helpers [fast: false, auto-fix: false] + - tparallel # tparallel detects inappropriate usage of t.Parallel() method in your Go test codes [fast: false, auto-fix: false] + - typecheck # Like the front-end of a Go compiler, parses and type-checks Go code [fast: false, auto-fix: false] + - unconvert # Remove unnecessary type conversions [fast: false, auto-fix: false] + - unparam # Reports unused function parameters [fast: false, auto-fix: false] + # Disabled due to way too many false positive in go1.20. + # - unused # (megacheck): Checks Go code for unused constants, variables, functions and types [fast: false, auto-fix: false] + # Disabled due to generic. Work in progress upstream. + # - wastedassign # wastedassign finds wasted assignment statements. [fast: false, auto-fix: false] + - whitespace # Tool for detection of leading and trailing whitespace [fast: true, auto-fix: true] + +issues: + exclude: + # Allow shadowing of 'err'. + - 'shadow: declaration of "err" shadows declaration' + # Allow shadowing of `ctx`. + - 'shadow: declaration of "ctx" shadows declaration' + # Maximum issues count per one linter. Set to 0 to disable. Default is 50. + max-per-linter: 10 + # Disable default excludes. Always be explicit on what we exclude. + exclude-use-default: false + # Exclude some linters from running on tests files. + exclude-rules: [] diff --git a/doc_test.go b/doc_test.go index 6e5916e..ac67e82 100644 --- a/doc_test.go +++ b/doc_test.go @@ -1,39 +1,10 @@ package pty import ( - "bytes" - "io" "os" "testing" ) -// Will fill p from reader r. -func readBytes(r io.Reader, p []byte) error { - _, err := io.ReadFull(r, p) - return err -} - -// openCloses opens a pty/tty pair and stages the closing as part of the cleanup. -func openClose(t *testing.T) (pty, tty *os.File) { - t.Helper() - - pty, tty, err := Open() - if err != nil { - t.Fatalf("Unexpected error from Open: %s.", err) - } - t.Cleanup(func() { - if err := tty.Close(); err != nil { - t.Errorf("Unexpected error from tty Close: %s.", err) - } - - if err := pty.Close(); err != nil { - t.Errorf("Unexpected error from pty Close: %s.", err) - } - }) - - return pty, tty -} - func TestOpen(t *testing.T) { t.Parallel() @@ -60,31 +31,22 @@ func TestName(t *testing.T) { func TestOpenByName(t *testing.T) { t.Parallel() - pty, tty := openClose(t) + pty, tty := openClose(t) // Get the pty/tty pair. + // Manually open the tty from the exiting name. ttyFile, err := os.OpenFile(tty.Name(), os.O_RDWR, 0o600) - if err != nil { - t.Fatalf("Failed to open tty file: %s.", err) - } + noError(t, err, "Failed to open tty file") 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.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)) - } - buffer := make([]byte, len(text)) - 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) - } + n, err := ttyFile.Write(text) // Write the text to the manually open tty. + noError(t, err, "Unexpected error from Write") // Make sure it didn't fail. + assert(t, len(text), n, "Unexpected count returned from Write") // Assert the number of bytes written. + + buffer := readN(t, pty, len(text), "Unexpected error from pty Read") + assertBytes(t, text, buffer, "Unexpected result result returned from pty Read") } func TestGetsize(t *testing.T) { @@ -93,50 +55,30 @@ func TestGetsize(t *testing.T) { pty, tty := openClose(t) prows, pcols, err := Getsize(pty) - if err != nil { - t.Errorf("Unexpected error from Getsize: %s.", err) - } + noError(t, err, "Unexpected error from pty Getsize") trows, tcols, err := Getsize(tty) - if err != nil { - t.Errorf("Unexpected error from Getsize: %s.", err) - } + noError(t, err, "Unexpected error from tty Getsize") - if 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) - } + assert(t, prows, trows, "rows from Getsize on pty and tty should match") + assert(t, pcols, tcols, "cols from Getsize on pty and tty should match") } -func TestGetsizefull(t *testing.T) { +func TestGetsizeFull(t *testing.T) { t.Parallel() pty, tty := openClose(t) psize, err := GetsizeFull(pty) - if err != nil { - t.Fatalf("Unexpected error from GetsizeFull: %s.", err) - } + noError(t, err, "Unexpected error from pty GetsizeFull") tsize, err := GetsizeFull(tty) - if err != nil { - t.Fatalf("Unexpected error from GetsizeFull: %s.", err) - } + noError(t, err, "Unexpected error from tty GetsizeFull") - if 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) - } - if 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) - } + assert(t, psize.X, tsize.X, "X from GetsizeFull on pty and tty should match") + assert(t, psize.Y, tsize.Y, "Y from GetsizeFull on pty and tty should match") + assert(t, psize.Rows, tsize.Rows, "rows from GetsizeFull on pty and tty should match") + assert(t, psize.Cols, tsize.Cols, "cols from GetsizeFull on pty and tty should match") } func TestSetsize(t *testing.T) { @@ -145,36 +87,22 @@ func TestSetsize(t *testing.T) { pty, tty := openClose(t) psize, err := GetsizeFull(pty) - if err != nil { - t.Fatalf("Unexpected error from GetsizeFull: %s.", err) - } + noError(t, err, "Unexpected error from pty GetsizeFull") - psize.X = psize.X + 1 - psize.Y = psize.Y + 1 - psize.Rows = psize.Rows + 1 - psize.Cols = psize.Cols + 1 + psize.X++ + psize.Y++ + psize.Rows++ + psize.Cols++ - if err := Setsize(tty, psize); err != nil { - t.Fatalf("Unexpected error from Setsize: %s", err) - } + noError(t, Setsize(tty, psize), "Unexpected error from Setsize") tsize, err := GetsizeFull(tty) - if err != nil { - t.Fatalf("Unexpected error from GetsizeFull: %s.", err) - } + noError(t, err, "Unexpected error from tty GetsizeFull") - if 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) - } - if 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) - } + assert(t, psize.X, tsize.X, "Unexpected Getsize X result after Setsize") + assert(t, psize.Y, tsize.Y, "Unexpected Getsize Y result after Setsize") + assert(t, psize.Rows, tsize.Rows, "Unexpected Getsize Rows result after Setsize") + assert(t, psize.Cols, tsize.Cols, "Unexpected Getsize Cols result after Setsize") } func TestReadWriteText(t *testing.T) { @@ -182,54 +110,35 @@ func TestReadWriteText(t *testing.T) { pty, tty := openClose(t) - // Write to tty, read from pty - text := []byte("ping") - n, err := tty.Write(text) - if err != nil { - 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)) - } + // Write to tty, read from pty. + { + text := []byte("ping") - buffer := make([]byte, 4) - 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) + n, err := tty.Write(text) + noError(t, err, "Unexpected error from tty Write") + assert(t, n, len(text), "Unexpected count returned from tty Write") + + buffer := readN(t, pty, len(text), "Unexpected error from pty Read") + assertBytes(t, text, buffer, "Unexpected result returned from pty Read") } // Write to pty, read from tty. // We need to send a \n otherwise this will block in the terminal driver. - text = []byte("pong\n") - n, err = pty.Write(text) - if err != nil { - 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)) - } + { + text := []byte("pong\n") - buffer = make([]byte, 5) - err = readBytes(tty, buffer) - if err != nil { - t.Fatalf("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) - } + n, err := pty.Write(text) + noError(t, err, "Unexpected error from pty Write") + assert(t, n, len(text), "Unexpected count returned from pty Write") - // Read the echo back from pty - buffer = make([]byte, 5) - err = readBytes(pty, buffer) - if err != nil { - 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) + // Expect the raw text back when reading from tty. + buffer := readN(t, tty, len(text), "Unexpected error from tty Read") + assertBytes(t, text, buffer, "Unexpected result returned from tty Read") + + // Read the echo back from pty. Expect LF to be CRLF. + expect := []byte("pong\r\n") + buffer = readN(t, pty, len(expect), "Unexpected error from pty Read") + assertBytes(t, expect, buffer, "Unexpected result returned from pty Read") } } @@ -239,52 +148,25 @@ func TestReadWriteControls(t *testing.T) { pty, tty := openClose(t) // Write the start of a line to pty. - text := []byte("pind") - n, err := pty.Write(text) - if err != nil { - 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)) - } + n, err := pty.WriteString("pind") // Intentional typo. + noError(t, err, "Unexpected error from Write initial text") // Make sure it didn't fail. + assert(t, 4, n, "Unexpected count returned from Write initial text") // Assert the number of bytes written. // Backspace that last char. - n, err = pty.Write([]byte("\b")) - if err != nil { - t.Fatalf("Unexpected error from Write: %s.", err) - } - if n != 1 { - t.Errorf("Unexpected count returned from Write, got %d expected %d.", n, 1) - } - - // Write the correct char and a CR. - n, err = pty.Write([]byte("g\n")) - if err != nil { - t.Fatalf("Unexpected error from Write: %s.", err) - } - if 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) - } - expect := []byte("pind\bg\n") - if !bytes.Equal(expect, buffer) { - 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) - } - expect = []byte("pind^Hg") - if !bytes.Equal(expect, buffer) { - t.Errorf("Unexpected result returned from Read, got %v expected %v.", buffer, expect) - } + n, err = pty.WriteString("\b") // "Remove" the typo. + noError(t, err, "Unexpected error from Write backspace") // Make sure it didn't fail. + assert(t, 1, n, "Unexpected count returned from Write backspace") // Assert the number of bytes written. + + // Write the correct char and a LF. + n, err = pty.WriteString("g\n") // Fix the typo. + noError(t, err, "Unexpected error from Write fixed text") // Make sure it didn't fail. + assert(t, 2, n, "Unexpected count returned from Write fixed text") // Assert the number of bytes written. + + // Read the line. + buffer := readN(t, tty, 7, "Unexpected error from tty Read") + assertBytes(t, []byte("pind\bg\n"), buffer, "Unexpected result returned from tty Read") + + // Read the echo back from pty. + buffer = readN(t, pty, 9, "Unexpected error from pty Read") + assertBytes(t, []byte("pind^Hg\r\n"), buffer, "Unexpected result returned from pty Read") } diff --git a/helpers_test.go b/helpers_test.go new file mode 100644 index 0000000..fbc98b7 --- /dev/null +++ b/helpers_test.go @@ -0,0 +1,62 @@ +package pty + +import ( + "bytes" + "fmt" + "io" + "os" + "testing" +) + +// openCloses opens a pty/tty pair and stages the closing as part of the cleanup. +func openClose(t *testing.T) (pty, tty *os.File) { + t.Helper() + + pty, tty, err := Open() + if err != nil { + t.Fatalf("Unexpected error from Open: %s.", err) + } + t.Cleanup(func() { + if err := tty.Close(); err != nil { + t.Errorf("Unexpected error from tty Close: %s.", err) + } + + if err := pty.Close(); err != nil { + t.Errorf("Unexpected error from pty Close: %s.", err) + } + }) + + return pty, tty +} + +func noError(t *testing.T, err error, msg string) { + t.Helper() + if err != nil { + t.Fatalf("%s: %s.", msg, err) + } +} + +func assert[T comparable](t *testing.T, a, b T, msg string) { + t.Helper() + if a != b { + t.Errorf("%s: %v != %v.", msg, a, b) + } +} + +// When asserting bytes, we want to display the diff, which may contain control characters. +func assertBytes(t *testing.T, a, b []byte, msg string) { + t.Helper() + if !bytes.Equal(a, b) { + t.Errorf("%s: %v != %v.", msg, a, b) + } +} + +// Read a specific number of bytes from r, assert it didn't fail and return the bytes. +func readN(t *testing.T, r io.Reader, n int, msg string) []byte { + t.Helper() + + buf := make([]byte, n) + _, err := io.ReadFull(r, buf) + noError(t, err, fmt.Sprintf("%s: %s", msg, err)) + return buf +} diff --git a/io_test.go b/io_test.go index 661559e..d448eb7 100644 --- a/io_test.go +++ b/io_test.go @@ -18,12 +18,15 @@ const ( timeout = time.Second ) +//nolint:gochecknoglobals // Expected global lock to avodi potential race on (*os.File).Fd(). var glTestFdLock sync.Mutex // Check that SetDeadline() works for ptmx. // Outstanding Read() calls must be interrupted by deadline. // // https://github.com/creack/pty/issues/162 +// +//nolint:paralleltest // Potential in (*os.File).Fd(). func TestReadDeadline(t *testing.T) { ptmx, success := prepare(t) @@ -51,6 +54,8 @@ func TestReadDeadline(t *testing.T) { // // https://github.com/creack/pty/issues/114 // https://github.com/creack/pty/issues/88 +// +//nolint:paralleltest // Potential in (*os.File).Fd(). func TestReadClose(t *testing.T) { ptmx, success := prepare(t) @@ -73,7 +78,7 @@ func TestReadClose(t *testing.T) { } } -// Open pty and setup watchdogs for graceful and not so graceful failure modes +// 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() @@ -104,18 +109,18 @@ func prepare(t *testing.T) (ptmx *os.File, done func()) { case <-ctx.Done(): // ptmx.Read() did not block forever, yay! case <-time.After(timeout): - if _, err := pts.Write([]byte{errMarker}); err != nil { // unblock ptmx.Read() + 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.") - done() // cancel panic() + done() // Cancel panic(). } }() go func() { select { case <-ctx.Done(): // Test has either failed or succeeded; it definitely did not hang. - case <-time.After(timeout * 10 / 9): // timeout +11% + case <-time.After(timeout * 10 / 9): // Timeout + 11%. panic("ptmx.Read() was not unblocked; avoid hanging forever.") // Just in case. } }() diff --git a/ioctl.go b/ioctl.go index 60ac9b8..021e4c7 100644 --- a/ioctl.go +++ b/ioctl.go @@ -8,13 +8,13 @@ import "os" func ioctl(f *os.File, cmd, ptr uintptr) error { sc, e := f.SyscallConn() if e != nil { - return ioctl_inner(f.Fd(), cmd, ptr) // fall back to blocking io (old behavior) + return ioctlInner(f.Fd(), cmd, ptr) // Fall back to blocking io (old behavior). } ch := make(chan error, 1) defer close(ch) - e = sc.Control(func(fd uintptr) { ch <- ioctl_inner(fd, cmd, ptr) }) + e = sc.Control(func(fd uintptr) { ch <- ioctlInner(fd, cmd, ptr) }) if e != nil { return e } diff --git a/ioctl_inner.go b/ioctl_inner.go index fd5dbef..272b50b 100644 --- a/ioctl_inner.go +++ b/ioctl_inner.go @@ -5,12 +5,13 @@ package pty import "syscall" +// Local syscall const values. const ( TIOCGWINSZ = syscall.TIOCGWINSZ TIOCSWINSZ = syscall.TIOCSWINSZ ) -func ioctl_inner(fd, cmd, ptr uintptr) error { +func ioctlInner(fd, cmd, ptr uintptr) error { _, _, e := syscall.Syscall(syscall.SYS_IOCTL, fd, cmd, ptr) if e != 0 { return e diff --git a/ioctl_legacy.go b/ioctl_legacy.go index f00b1a1..f7e923c 100644 --- a/ioctl_legacy.go +++ b/ioctl_legacy.go @@ -6,5 +6,5 @@ package pty import "os" func ioctl(f *os.File, cmd, ptr uintptr) error { - return ioctl_inner(f.Fd(), cmd, ptr) // fall back to blocking io (old behavior) + return ioctlInner(f.Fd(), cmd, ptr) // fall back to blocking io (old behavior) } diff --git a/ioctl_solaris.go b/ioctl_solaris.go index c0231df..6fd8bfe 100644 --- a/ioctl_solaris.go +++ b/ioctl_solaris.go @@ -40,7 +40,7 @@ type strioctl struct { // Defined in asm_solaris_amd64.s. func sysvicall6(trap, nargs, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2 uintptr, err syscall.Errno) -func ioctl_inner(fd, cmd, ptr uintptr) error { +func ioctlInner(fd, cmd, ptr uintptr) error { if _, _, errno := sysvicall6(uintptr(unsafe.Pointer(&procioctl)), 3, fd, cmd, ptr, 0, 0, 0); errno != 0 { return errno } diff --git a/ioctl_unsupported.go b/ioctl_unsupported.go index 79ad4c6..e17908d 100644 --- a/ioctl_unsupported.go +++ b/ioctl_unsupported.go @@ -8,6 +8,6 @@ const ( TIOCSWINSZ = 0 ) -func ioctl_inner(fd, cmd, ptr uintptr) error { +func ioctlInner(fd, cmd, ptr uintptr) error { return ErrUnsupported } diff --git a/pty_linux.go b/pty_linux.go index e12e2c8..e7e01c0 100644 --- a/pty_linux.go +++ b/pty_linux.go @@ -49,6 +49,6 @@ func ptsname(f *os.File) (string, error) { func unlockpt(f *os.File) error { var u _C_int - // use TIOCSPTLCK with a pointer to zero to clear the lock + // use TIOCSPTLCK with a pointer to zero to clear the lock. return ioctl(f, syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&u))) //nolint:gosec // Expected unsafe pointer for Syscall call. } diff --git a/winsize.go b/winsize.go index 57323f4..cfa3e5f 100644 --- a/winsize.go +++ b/winsize.go @@ -10,10 +10,7 @@ func InheritSize(pty, tty *os.File) error { if err != nil { return err } - if err := Setsize(tty, size); err != nil { - return err - } - return nil + return Setsize(tty, size) } // Getsize returns the number of rows (lines) and cols (positions diff --git a/winsize_unix.go b/winsize_unix.go index ad98c8a..8dbbcda 100644 --- a/winsize_unix.go +++ b/winsize_unix.go @@ -11,10 +11,10 @@ import ( // Winsize describes the terminal size. type Winsize struct { - Rows uint16 // ws_row: Number of rows (in cells) - Cols uint16 // ws_col: Number of columns (in cells) - X uint16 // ws_xpixel: Width in pixels - Y uint16 // ws_ypixel: Height in pixels + Rows uint16 // ws_row: Number of rows (in cells). + Cols uint16 // ws_col: Number of columns (in cells). + X uint16 // ws_xpixel: Width in pixels. + Y uint16 // ws_ypixel: Height in pixels. } // Setsize resizes t to s. From 0a15590bf0148e86be03cc00016045cf42e6ee0e Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Tue, 31 Oct 2023 10:55:31 -0400 Subject: [PATCH 4/5] Skip tests for go1.6. --- .github/workflows/test.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dab2c73..4a949d3 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -48,5 +48,8 @@ jobs: - name: Build run: go build -v - - name: Test + # Skip tests for 1.6 as we use modern Go. + # If the main lib builds and tests pass on other versions, we are good. + - if: ${{ matrix.go_version != '1.6.x' }} + name: Test run: go test -v From cafb3b47d008d5dd887a8f266969e15f3fe6a439 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Tue, 31 Oct 2023 14:32:20 -0400 Subject: [PATCH 5/5] Fix typo. --- io_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io_test.go b/io_test.go index d448eb7..a429edb 100644 --- a/io_test.go +++ b/io_test.go @@ -18,7 +18,7 @@ const ( timeout = time.Second ) -//nolint:gochecknoglobals // Expected global lock to avodi potential race on (*os.File).Fd(). +//nolint:gochecknoglobals // Expected global lock to avoid potential race on (*os.File).Fd(). var glTestFdLock sync.Mutex // Check that SetDeadline() works for ptmx.