From 02ee724d9fa1cd2ca7d3e860ddf68de8f966d4b4 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 16 Aug 2023 13:02:03 -0800 Subject: [PATCH] fix: do terminal emulation in reconnecting pty tests (#9114) It looks like it is possible for screen to use control sequences instead of literal newlines which fails the tests. This reuses the existing readUntil function used in other pty tests. --- agent/agent_test.go | 58 ++++++-------------- coderd/workspaceapps/apptest/apptest.go | 41 ++++----------- pty/start_test.go | 37 +------------ testutil/pty.go | 70 +++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 109 deletions(-) create mode 100644 testutil/pty.go diff --git a/agent/agent_test.go b/agent/agent_test.go index e9f8bc772b..1621b0075b 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -1,7 +1,6 @@ package agent_test import ( - "bufio" "bytes" "context" "encoding/json" @@ -1588,10 +1587,6 @@ func TestAgent_Startup(t *testing.T) { }) } -const ansi = "[\u001B\u009B][[\\]()#;?]*(?:(?:(?:[a-zA-Z\\d]*(?:;[a-zA-Z\\d]*)*)?\u0007)|(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PRZcf-ntqry=><~]))" - -var re = regexp.MustCompile(ansi) - //nolint:paralleltest // This test sets an environment variable. func TestAgent_ReconnectingPTY(t *testing.T) { if runtime.GOOS == "windows" { @@ -1635,17 +1630,14 @@ func TestAgent_ReconnectingPTY(t *testing.T) { //nolint:dogsled conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0) id := uuid.New() - netConn1, err := conn.ReconnectingPTY(ctx, id, 100, 100, "bash") + netConn1, err := conn.ReconnectingPTY(ctx, id, 80, 80, "bash") require.NoError(t, err) defer netConn1.Close() - scanner1 := bufio.NewScanner(netConn1) - // A second simultaneous connection. - netConn2, err := conn.ReconnectingPTY(ctx, id, 100, 100, "bash") + netConn2, err := conn.ReconnectingPTY(ctx, id, 80, 80, "bash") require.NoError(t, err) defer netConn2.Close() - scanner2 := bufio.NewScanner(netConn2) // Brief pause to reduce the likelihood that we send keystrokes while // the shell is simultaneously sending a prompt. @@ -1658,17 +1650,6 @@ func TestAgent_ReconnectingPTY(t *testing.T) { _, err = netConn1.Write(data) require.NoError(t, err) - hasLine := func(scanner *bufio.Scanner, matcher func(string) bool) bool { - for scanner.Scan() { - line := scanner.Text() - t.Logf("bash tty stdout = %s", re.ReplaceAllString(line, "")) - if matcher(line) { - return true - } - } - return false - } - matchEchoCommand := func(line string) bool { return strings.Contains(line, "echo test") } @@ -1683,25 +1664,23 @@ func TestAgent_ReconnectingPTY(t *testing.T) { } // Once for typing the command... - require.True(t, hasLine(scanner1, matchEchoCommand), "find echo command") + require.NoError(t, testutil.ReadUntil(ctx, t, netConn1, matchEchoCommand), "find echo command") // And another time for the actual output. - require.True(t, hasLine(scanner1, matchEchoOutput), "find echo output") + require.NoError(t, testutil.ReadUntil(ctx, t, netConn1, matchEchoOutput), "find echo output") // Same for the other connection. - require.True(t, hasLine(scanner2, matchEchoCommand), "find echo command") - require.True(t, hasLine(scanner2, matchEchoOutput), "find echo output") + require.NoError(t, testutil.ReadUntil(ctx, t, netConn2, matchEchoCommand), "find echo command") + require.NoError(t, testutil.ReadUntil(ctx, t, netConn2, matchEchoOutput), "find echo output") _ = netConn1.Close() _ = netConn2.Close() - netConn3, err := conn.ReconnectingPTY(ctx, id, 100, 100, "bash") + netConn3, err := conn.ReconnectingPTY(ctx, id, 80, 80, "bash") require.NoError(t, err) defer netConn3.Close() - scanner3 := bufio.NewScanner(netConn3) - // Same output again! - require.True(t, hasLine(scanner3, matchEchoCommand), "find echo command") - require.True(t, hasLine(scanner3, matchEchoOutput), "find echo output") + require.NoError(t, testutil.ReadUntil(ctx, t, netConn3, matchEchoCommand), "find echo command") + require.NoError(t, testutil.ReadUntil(ctx, t, netConn3, matchEchoOutput), "find echo output") // Exit should cause the connection to close. data, err = json.Marshal(codersdk.ReconnectingPTYRequest{ @@ -1712,26 +1691,19 @@ func TestAgent_ReconnectingPTY(t *testing.T) { require.NoError(t, err) // Once for the input and again for the output. - require.True(t, hasLine(scanner3, matchExitCommand), "find exit command") - require.True(t, hasLine(scanner3, matchExitOutput), "find exit output") + require.NoError(t, testutil.ReadUntil(ctx, t, netConn3, matchExitCommand), "find exit command") + require.NoError(t, testutil.ReadUntil(ctx, t, netConn3, matchExitOutput), "find exit output") // Wait for the connection to close. - for scanner3.Scan() { - line := scanner3.Text() - t.Logf("bash tty stdout = %s", re.ReplaceAllString(line, "")) - } + require.ErrorIs(t, testutil.ReadUntil(ctx, t, netConn3, nil), io.EOF) // Try a non-shell command. It should output then immediately exit. - netConn4, err := conn.ReconnectingPTY(ctx, uuid.New(), 100, 100, "echo test") + netConn4, err := conn.ReconnectingPTY(ctx, uuid.New(), 80, 80, "echo test") require.NoError(t, err) defer netConn4.Close() - scanner4 := bufio.NewScanner(netConn4) - require.True(t, hasLine(scanner4, matchEchoOutput), "find echo output") - for scanner4.Scan() { - line := scanner4.Text() - t.Logf("bash tty stdout = %s", re.ReplaceAllString(line, "")) - } + require.NoError(t, testutil.ReadUntil(ctx, t, netConn4, matchEchoOutput), "find echo output") + require.ErrorIs(t, testutil.ReadUntil(ctx, t, netConn3, nil), io.EOF) }) } } diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 228ebceee9..6d192735b4 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -12,7 +12,6 @@ import ( "net/http/httputil" "net/url" "path" - "regexp" "runtime" "strconv" "strings" @@ -32,10 +31,6 @@ import ( "github.com/coder/coder/testutil" ) -const ansi = "[\u001B\u009B][[\\]()#;?]*(?:(?:(?:[a-zA-Z\\d]*(?:;[a-zA-Z\\d]*)*)?\u0007)|(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PRZcf-ntqry=><~]))" - -var re = regexp.MustCompile(ansi) - // Run runs the entire workspace app test suite against deployments minted // by the provided factory. // @@ -70,8 +65,8 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { testReconnectingPTY(ctx, t, client, codersdk.WorkspaceAgentReconnectingPTYOpts{ AgentID: appDetails.Agent.ID, Reconnect: uuid.New(), - Height: 80, - Width: 80, + Height: 100, + Width: 100, Command: "bash", }) }) @@ -104,8 +99,8 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { testReconnectingPTY(ctx, t, unauthedAppClient, codersdk.WorkspaceAgentReconnectingPTYOpts{ AgentID: appDetails.Agent.ID, Reconnect: uuid.New(), - Height: 80, - Width: 80, + Height: 100, + Width: 100, Command: "bash", SignedToken: issueRes.SignedToken, }) @@ -1407,16 +1402,6 @@ func (r *fakeStatsReporter) Report(_ context.Context, stats []workspaceapps.Stat } func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Client, opts codersdk.WorkspaceAgentReconnectingPTYOpts) { - hasLine := func(scanner *bufio.Scanner, matcher func(string) bool) bool { - for scanner.Scan() { - line := scanner.Text() - t.Logf("bash tty stdout = %s", re.ReplaceAllString(line, "")) - if matcher(line) { - return true - } - } - return false - } matchEchoCommand := func(line string) bool { return strings.Contains(line, "echo test") } @@ -1437,13 +1422,12 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli // First attempt to resize the TTY. // The websocket will close if it fails! data, err := json.Marshal(codersdk.ReconnectingPTYRequest{ - Height: 250, - Width: 250, + Height: 80, + Width: 80, }) require.NoError(t, err) _, err = conn.Write(data) require.NoError(t, err) - scanner := bufio.NewScanner(conn) // Brief pause to reduce the likelihood that we send keystrokes while // the shell is simultaneously sending a prompt. @@ -1456,8 +1440,8 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli _, err = conn.Write(data) require.NoError(t, err) - require.True(t, hasLine(scanner, matchEchoCommand), "find echo command") - require.True(t, hasLine(scanner, matchEchoOutput), "find echo output") + require.NoError(t, testutil.ReadUntil(ctx, t, conn, matchEchoCommand), "find echo command") + require.NoError(t, testutil.ReadUntil(ctx, t, conn, matchEchoOutput), "find echo output") // Exit should cause the connection to close. data, err = json.Marshal(codersdk.ReconnectingPTYRequest{ @@ -1468,12 +1452,9 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli require.NoError(t, err) // Once for the input and again for the output. - require.True(t, hasLine(scanner, matchExitCommand), "find exit command") - require.True(t, hasLine(scanner, matchExitOutput), "find exit output") + require.NoError(t, testutil.ReadUntil(ctx, t, conn, matchExitCommand), "find exit command") + require.NoError(t, testutil.ReadUntil(ctx, t, conn, matchExitOutput), "find exit output") // Ensure the connection closes. - for scanner.Scan() { - line := scanner.Text() - t.Logf("bash tty stdout = %s", re.ReplaceAllString(line, "")) - } + require.ErrorIs(t, testutil.ReadUntil(ctx, t, conn, nil), io.EOF) } diff --git a/pty/start_test.go b/pty/start_test.go index 5f273428d2..a3141e8268 100644 --- a/pty/start_test.go +++ b/pty/start_test.go @@ -5,11 +5,9 @@ import ( "context" "fmt" "io" - "strings" "testing" "time" - "github.com/hinshun/vt10x" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -73,7 +71,7 @@ func Test_Start_truncation(t *testing.T) { n := 1 for n <= countEnd { want := fmt.Sprintf("%d", n) - err := readUntil(ctx, t, want, pc.OutputReader()) + err := testutil.ReadUntilString(ctx, t, want, pc.OutputReader()) assert.NoError(t, err, "want: %s", want) if err != nil { return @@ -141,36 +139,3 @@ func Test_Start_cancel_context(t *testing.T) { t.Error("cmd.Wait() timed out") } } - -// readUntil reads one byte at a time until we either see the string we want, or the context expires -func readUntil(ctx context.Context, t *testing.T, want string, r io.Reader) error { - // output can contain virtual terminal sequences, so we need to parse these - // to correctly interpret getting what we want. - term := vt10x.New(vt10x.WithSize(80, 80)) - readErrs := make(chan error, 1) - for { - b := make([]byte, 1) - go func() { - _, err := r.Read(b) - readErrs <- err - }() - select { - case err := <-readErrs: - if err != nil { - t.Logf("err: %v\ngot: %v", err, term) - return err - } - term.Write(b) - case <-ctx.Done(): - return ctx.Err() - } - got := term.String() - lines := strings.Split(got, "\n") - for _, line := range lines { - if strings.TrimSpace(line) == want { - t.Logf("want: %v\n got:%v", want, line) - return nil - } - } - } -} diff --git a/testutil/pty.go b/testutil/pty.go new file mode 100644 index 0000000000..956a77813a --- /dev/null +++ b/testutil/pty.go @@ -0,0 +1,70 @@ +package testutil + +import ( + "context" + "io" + "strings" + "testing" + + "github.com/hinshun/vt10x" +) + +// ReadUntilString emulates a terminal and reads one byte at a time until we +// either see the string we want, or the context expires. The PTY must be sized +// to 80x80 or there could be unexpected results. +func ReadUntilString(ctx context.Context, t *testing.T, want string, r io.Reader) error { + return ReadUntil(ctx, t, r, func(line string) bool { + return strings.TrimSpace(line) == want + }) +} + +// ReadUntil emulates a terminal and reads one byte at a time until the matcher +// returns true or the context expires. If the matcher is nil, read until EOF. +// The PTY must be sized to 80x80 or there could be unexpected results. +func ReadUntil(ctx context.Context, t *testing.T, r io.Reader, matcher func(line string) bool) error { + // output can contain virtual terminal sequences, so we need to parse these + // to correctly interpret getting what we want. + term := vt10x.New(vt10x.WithSize(80, 80)) + readErrs := make(chan error, 1) + defer func() { + // Dump the terminal contents since they can be helpful for debugging, but + // skip empty lines since much of the terminal will usually be blank. + got := term.String() + lines := strings.Split(got, "\n") + for _, line := range lines { + if strings.TrimSpace(line) != "" { + t.Logf("got: %v", line) + } + } + }() + for { + b := make([]byte, 1) + go func() { + _, err := r.Read(b) + readErrs <- err + }() + select { + case err := <-readErrs: + if err != nil { + return err + } + _, err = term.Write(b) + if err != nil { + return err + } + case <-ctx.Done(): + return ctx.Err() + } + if matcher == nil { + // A nil matcher means to read until EOF. + continue + } + got := term.String() + lines := strings.Split(got, "\n") + for _, line := range lines { + if matcher(line) { + return nil + } + } + } +}