interp: test tty detection properly

[mvdan]

See the commit message.

[mvdan]

@theclapp requesting a review, if you have a few minutes :)

[theclapp]

LGTM. As a bonus, I got the darwin test to pass (... though I haven't retested under Linux or Windows).

As near as I can tell, on a Mac, once you close the tty, reading from the pty returns no data. Which kind of makes sense to me, but apparently isn't true under Linux.

And I think I fixed the weird non-EOF error you occasionally(?) get out of creack/pty; see creack/pty#21, with a little help from @mattn in antonmedv/watch/pull/1, here. (Although, since I'm not closing the pty any more, maybe you wouldn't get that error any more? I dunno.)

Here's the diff:

diff --git a/interp/terminal_test.go b/interp/terminal_test.go
index fac138c..dd6379d 100644
--- a/interp/terminal_test.go
+++ b/interp/terminal_test.go
@@ -9,11 +9,10 @@ import (
 	"bytes"
 	"context"
 	"io"
-	"io/ioutil"
 	"os"
 	"os/exec"
 	"runtime"
-	"strings"
+	"syscall"
 	"testing"
 
 	"github.com/creack/pty"
@@ -37,9 +36,6 @@ func TestRunnerTerminalStdIO(t *testing.T) {
 			return b, b
 		}, "0\n1\n2\n"},
 		{"Pseudo", func(t *testing.T) (io.ReadWriter, io.Reader) {
-			if runtime.GOOS == "darwin" {
-				t.Skip("TODO: figure out why this case won't work on Mac")
-			}
 			pty, tty, err := pty.Open()
 			if err != nil {
 				t.Fatal(err)
@@ -59,10 +55,6 @@ func TestRunnerTerminalStdIO(t *testing.T) {
 			if err := r.Run(context.Background(), file); err != nil {
 				t.Fatal(err)
 			}
-			if closer, ok := stdio.(io.Closer); ok {
-				// Otherwise the ReadAll below might get stuck.
-				closer.Close()
-			}
 			if got := testReadAll(t, out); got != test.want {
 				t.Fatalf("\nwant: %q\ngot:  %q", test.want, got)
 			}
@@ -80,11 +72,15 @@ func testReadAll(t *testing.T, r io.Reader) string {
 	if r == nil {
 		return ""
 	}
-	// the pty package can give a weird error instead of EOF
-	bs, err := ioutil.ReadAll(r)
-	if err != nil && !strings.Contains(err.Error(), "input/output error") {
+	buf := make([]byte, 1024)
+	n, err := r.Read(buf)
+	bs := buf[:n]
+	// See https://github.com/creack/pty/issues/21
+	if err != nil && err != io.EOF {
+		if pathErr, ok := err.(*os.PathError); !ok || pathErr.Err != syscall.EIO {
 			t.Fatal(err)
 		}
+	}
 	return string(bs)
 }

[mvdan]

Did you by any chance generate that patch via git diff -w? It doesn't apply properly :) It was simple enough, so I just pieced it back together by hand. In the future, I think git diff without any options is the only sure way to produce a working patch.

In any case - thanks so much for the help! I do give up on Mac affairs pretty quickly, simply because it's a nightmare to test on it. I think I was able to simplify your version a bit, and it still passes on Linux. I'll push and see if it still passes on Mac.

[mvdan]

Ah, the Exec one still fails on Darwin, but you weren't reenabling that one. Hm.

[mvdan]

I think I'm wrapping my mind around the problem. I have a potential solution I'm trying out, which should be portable and non-racy.

[theclapp]

Did you by any chance generate that patch via git diff -w?

Yes.

It doesn't apply properly

:( Sorry

It was simple enough, so I just pieced it back together by hand.

🎉

In the future, I think git diff without any options is the only sure way to produce a working patch.

Yeah, will do.

[mvdan]

OK, so here are the major takeaways:

1. The Runner and Exec tests must both use the same threading model. Before, Runner ran synchronously via Run, and Exec ran asynchronously via Start. Now Runner spawns a goroutine to mimick Cmd.Start.

2. The other major problem related to waiting. Initially, we didn't close either end of a pty/pipe. This resulted in ReadAll hanging forever. You fixed this initially with a single Read call, but I didn't find that answer very satisfactory. What if it's a partial read? How do we know we're done reading?

Then, I resorted to having the master/writer close the connection. This worked most of the time, but it was racy - if the reader hadn't had time to read all of the bytes we want, it could end up with EOF, or an even worse error like "pipe already closed".

The root of the problem here is that neither side knows when we're "done". Each test case writes a different number of bytes, so we don't know either. I fixed this by forcing every test case to expect exactly one line of output. This way, the reader can stop as soon as it's read one line, and the writer can then close.

Phew :) @theclapp if you're happy with the latest patch, I'll merge. It seems all green, and there are no skips.

[theclapp]

LGTM. Can't test it right now, myself. I'd say, ship it.

Thanks for working on this!