From b232717a712762575beff164087fa1c7186a7b56 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 14:07:44 +0300 Subject: [PATCH 01/15] chore: Standardize waits and intervals between tests --- agent/reaper/reaper_test.go | 4 +-- cli/cliui/prompt_test.go | 5 +-- cli/create_test.go | 3 +- cli/portforward_test.go | 11 +++--- cli/resetpassword_test.go | 4 +-- cli/server_test.go | 50 +++++++++++---------------- cli/ssh_test.go | 7 ++-- coderd/coderd_test.go | 3 +- coderd/devtunnel/tunnel_test.go | 9 ++--- coderd/httpmw/ratelimit_test.go | 4 +-- coderd/provisionerdaemons_test.go | 4 +-- coderd/templateversions_test.go | 17 ++++----- coderd/workspacebuilds_test.go | 5 +-- internal/testutil/duration.go | 20 +++++++++++ internal/testutil/duration_windows.go | 22 ++++++++++++ peer/conn_test.go | 11 +++--- provisionerd/provisionerd_test.go | 33 +++++++++--------- site/site_test.go | 5 +-- 18 files changed, 131 insertions(+), 86 deletions(-) create mode 100644 internal/testutil/duration.go create mode 100644 internal/testutil/duration_windows.go diff --git a/agent/reaper/reaper_test.go b/agent/reaper/reaper_test.go index 8e23bec198c5c..b3a8370dfaeca 100644 --- a/agent/reaper/reaper_test.go +++ b/agent/reaper/reaper_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/agent/reaper" + "github.com/coder/coder/internal/testutil" ) func TestReap(t *testing.T) { @@ -52,10 +53,9 @@ func TestReap(t *testing.T) { expectedPIDs := []int{cmd.Process.Pid, cmd2.Process.Pid} - deadline := time.NewTimer(time.Second * 5) for i := 0; i < len(expectedPIDs); i++ { select { - case <-deadline.C: + case <-time.After(testutil.WaitShort): t.Fatalf("Timed out waiting for process") case pid := <-pids: require.Contains(t, expectedPIDs, pid) diff --git a/cli/cliui/prompt_test.go b/cli/cliui/prompt_test.go index eefc6ab4c8a5a..3aafd2aeba6f2 100644 --- a/cli/cliui/prompt_test.go +++ b/cli/cliui/prompt_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/cli/cliui" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty" "github.com/coder/coder/pty/ptytest" ) @@ -193,7 +194,7 @@ func TestPasswordTerminalState(t *testing.T) { require.Eventually(t, func() bool { echo, err := ptyWithFlags.EchoEnabled() return err == nil && !echo - }, 5*time.Second, 50*time.Millisecond, "echo is on while reading password") + }, testutil.WaitShort, testutil.IntervalMedium, "echo is on while reading password") err = process.Signal(os.Interrupt) require.NoError(t, err) @@ -203,7 +204,7 @@ func TestPasswordTerminalState(t *testing.T) { require.Eventually(t, func() bool { echo, err := ptyWithFlags.EchoEnabled() return err == nil && echo - }, 5*time.Second, 50*time.Millisecond, "echo is off after reading password") + }, testutil.WaitShort, testutil.IntervalMedium, "echo is off after reading password") } // nolint:unused diff --git a/cli/create_test.go b/cli/create_test.go index 5578518f351dc..988c85113cbc0 100644 --- a/cli/create_test.go +++ b/cli/create_test.go @@ -13,6 +13,7 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" "github.com/coder/coder/pty/ptytest" @@ -88,7 +89,7 @@ func TestCreate(t *testing.T) { member := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) clitest.SetupConfig(t, member, root) - cmdCtx, done := context.WithTimeout(context.Background(), 10*time.Second) + cmdCtx, done := context.WithTimeout(context.Background(), testutil.WaitLong) go func() { defer done() err := cmd.ExecuteContext(cmdCtx) diff --git a/cli/portforward_test.go b/cli/portforward_test.go index 5f314a3658850..d8071ab52824f 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -21,6 +21,7 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" ) @@ -170,7 +171,7 @@ func TestPortForward(t *testing.T) { // Open two connections simultaneously and test them out of // sync. - d := net.Dialer{Timeout: 3 * time.Second} + d := net.Dialer{Timeout: testutil.WaitShort} c1, err := d.DialContext(ctx, c.network, localAddress) require.NoError(t, err, "open connection 1 to 'local' listener") defer c1.Close() @@ -216,7 +217,7 @@ func TestPortForward(t *testing.T) { // Open a connection to both listener 1 and 2 simultaneously and // then test them out of order. - d := net.Dialer{Timeout: 3 * time.Second} + d := net.Dialer{Timeout: testutil.WaitShort} c1, err := d.DialContext(ctx, c.network, localAddress1) require.NoError(t, err, "open connection 1 to 'local' listener 1") defer c1.Close() @@ -269,7 +270,7 @@ func TestPortForward(t *testing.T) { // Open two connections simultaneously and test them out of // sync. - d := net.Dialer{Timeout: 3 * time.Second} + d := net.Dialer{Timeout: testutil.WaitShort} c1, err := d.DialContext(ctx, tcpCase.network, localAddress) require.NoError(t, err, "open connection 1 to 'local' listener") defer c1.Close() @@ -329,7 +330,7 @@ func TestPortForward(t *testing.T) { // Open connections to all items in the "dial" array. var ( - d = net.Dialer{Timeout: 3 * time.Second} + d = net.Dialer{Timeout: testutil.WaitShort} conns = make([]net.Conn, len(dials)) ) for i, a := range dials { @@ -488,7 +489,7 @@ func assertWritePayload(t *testing.T, w io.Writer, payload []byte) { func waitForPortForwardReady(t *testing.T, output *threadSafeBuffer) { t.Helper() for i := 0; i < 100; i++ { - time.Sleep(250 * time.Millisecond) + time.Sleep(testutil.IntervalMedium) data := output.String() if strings.Contains(data, "Ready!") { diff --git a/cli/resetpassword_test.go b/cli/resetpassword_test.go index 6499e30a2481b..faf82fb2e749b 100644 --- a/cli/resetpassword_test.go +++ b/cli/resetpassword_test.go @@ -5,7 +5,6 @@ import ( "net/url" "runtime" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -13,6 +12,7 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/database/postgres" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty/ptytest" ) @@ -53,7 +53,7 @@ func TestResetPassword(t *testing.T) { require.Eventually(t, func() bool { rawURL, err = cfg.URL().Read() return err == nil && rawURL != "" - }, 15*time.Second, 25*time.Millisecond) + }, testutil.WaitLong, testutil.IntervalFast) accessURL, err := url.Parse(rawURL) require.NoError(t, err) client := codersdk.New(accessURL) diff --git a/cli/server_test.go b/cli/server_test.go index ae70324b33741..a2ab757c2cf1f 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -26,9 +26,11 @@ import ( "go.uber.org/goleak" "github.com/coder/coder/cli/clitest" + "github.com/coder/coder/cli/config" "github.com/coder/coder/coderd/database/postgres" "github.com/coder/coder/coderd/telemetry" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty/ptytest" ) @@ -56,13 +58,7 @@ func TestServer(t *testing.T) { go func() { errC <- root.ExecuteContext(ctx) }() - var rawURL string - require.Eventually(t, func() bool { - rawURL, err = cfg.URL().Read() - return err == nil && rawURL != "" - }, time.Minute, 50*time.Millisecond) - accessURL, err := url.Parse(rawURL) - require.NoError(t, err) + accessURL := waitAccessURL(t, cfg) client := codersdk.New(accessURL) _, err = client.CreateFirstUser(ctx, codersdk.CreateFirstUserRequest{ @@ -95,10 +91,7 @@ func TestServer(t *testing.T) { go func() { errC <- root.ExecuteContext(ctx) }() - require.Eventually(t, func() bool { - accessURLRaw, err := cfg.URL().Read() - return accessURLRaw != "" && err == nil - }, 3*time.Minute, 250*time.Millisecond) + _ = waitAccessURL(t, cfg) cancelFunc() require.ErrorIs(t, <-errC, context.Canceled) }) @@ -133,11 +126,7 @@ func TestServer(t *testing.T) { }() // Just wait for startup - require.Eventually(t, func() bool { - var err error - _, err = cfg.URL().Read() - return err == nil - }, 15*time.Second, 25*time.Millisecond) + _ = waitAccessURL(t, cfg) cancelFunc() require.ErrorIs(t, <-errC, context.Canceled) @@ -213,14 +202,7 @@ func TestServer(t *testing.T) { }() // Verify HTTPS - var accessURLRaw string - require.Eventually(t, func() bool { - var err error - accessURLRaw, err = cfg.URL().Read() - return accessURLRaw != "" && err == nil - }, 15*time.Second, 25*time.Millisecond) - accessURL, err := url.Parse(accessURLRaw) - require.NoError(t, err) + accessURL := waitAccessURL(t, cfg) require.Equal(t, "https", accessURL.Scheme) client := codersdk.New(accessURL) client.HTTPClient = &http.Client{ @@ -258,11 +240,7 @@ func TestServer(t *testing.T) { go func() { serverErr <- root.ExecuteContext(ctx) }() - require.Eventually(t, func() bool { - var err error - _, err = cfg.URL().Read() - return err == nil - }, 15*time.Second, 25*time.Millisecond) + _ = waitAccessURL(t, cfg) currentProcess, err := os.FindProcess(os.Getpid()) require.NoError(t, err) err = currentProcess.Signal(os.Interrupt) @@ -368,3 +346,17 @@ func generateTLSCertificate(t testing.TB) (certPath, keyPath string) { require.NoError(t, err) return certFile.Name(), keyFile.Name() } + +func waitAccessURL(t *testing.T, cfg config.Root) *url.URL { + var err error + var rawURL string + require.Eventually(t, func() bool { + rawURL, err = cfg.URL().Read() + return err == nil && rawURL != "" + }, testutil.WaitLong, testutil.IntervalFast) + + accessURL, err := url.Parse(rawURL) + require.NoError(t, err) + + return accessURL +} diff --git a/cli/ssh_test.go b/cli/ssh_test.go index f88339c103fd9..d0d5f581f77f8 100644 --- a/cli/ssh_test.go +++ b/cli/ssh_test.go @@ -26,6 +26,7 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" "github.com/coder/coder/pty/ptytest" @@ -77,7 +78,7 @@ func TestSSH(t *testing.T) { cmd.SetErr(pty.Output()) cmd.SetOut(pty.Output()) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() cmdDone := tGo(t, func() { @@ -124,7 +125,7 @@ func TestSSH(t *testing.T) { } }() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() cmd, root := clitest.New(t, "ssh", "--stdio", workspace.Name) @@ -215,7 +216,7 @@ func TestSSH(t *testing.T) { } }) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() cmd, root := clitest.New(t, diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 7eb9167caa05a..28df87ce1ac08 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -38,6 +38,7 @@ import ( "github.com/coder/coder/coderd/telemetry" "github.com/coder/coder/coderd/turnconn" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" ) @@ -157,7 +158,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { require.Eventually(t, func() bool { provisionerds, err := client.ProvisionerDaemons(ctx) return assert.NoError(t, err) && len(provisionerds) > 0 - }, time.Second*10, time.Second) + }, testutil.WaitLong, testutil.IntervalSlow) provisionerds, err := client.ProvisionerDaemons(ctx) require.NoError(t, err, "fetch provisioners") diff --git a/coderd/devtunnel/tunnel_test.go b/coderd/devtunnel/tunnel_test.go index 70007cc257191..c91bc18cfe09c 100644 --- a/coderd/devtunnel/tunnel_test.go +++ b/coderd/devtunnel/tunnel_test.go @@ -25,6 +25,7 @@ import ( "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/coderd/devtunnel" + "github.com/coder/coder/internal/testutil" ) const ( @@ -85,15 +86,15 @@ func TestTunnel(t *testing.T) { _, _ = io.Copy(io.Discard, res.Body) return res.StatusCode == http.StatusAccepted - }, time.Minute, time.Second) + }, testutil.WaitShort, testutil.IntervalSlow) assert.NoError(t, server.Close()) cancelTun() select { case <-errCh: - case <-time.After(10 * time.Second): - t.Error("tunnel did not close after 10 seconds") + case <-time.After(testutil.WaitLong): + t.Errorf("tunnel did not close after %s", testutil.WaitLong) } } @@ -226,7 +227,7 @@ func (f *fakeTunnelServer) requestHTTP() (*http.Response, error) { } client := &http.Client{ Transport: transport, - Timeout: 10 * time.Second, + Timeout: testutil.WaitLong, } return client.Get(fmt.Sprintf("http://[%s]:8090", clientIP)) } diff --git a/coderd/httpmw/ratelimit_test.go b/coderd/httpmw/ratelimit_test.go index c2e4ebd41fd78..b0034d0784c00 100644 --- a/coderd/httpmw/ratelimit_test.go +++ b/coderd/httpmw/ratelimit_test.go @@ -4,12 +4,12 @@ import ( "net/http" "net/http/httptest" "testing" - "time" "github.com/go-chi/chi/v5" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/internal/testutil" ) func TestRateLimit(t *testing.T) { @@ -27,6 +27,6 @@ func TestRateLimit(t *testing.T) { rec := httptest.NewRecorder() rtr.ServeHTTP(rec, req) return rec.Result().StatusCode == http.StatusTooManyRequests - }, 5*time.Second, time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) }) } diff --git a/coderd/provisionerdaemons_test.go b/coderd/provisionerdaemons_test.go index 1a89bb4e701a8..bd1667737baac 100644 --- a/coderd/provisionerdaemons_test.go +++ b/coderd/provisionerdaemons_test.go @@ -5,13 +5,13 @@ import ( "crypto/rand" "runtime" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisionersdk" ) @@ -41,7 +41,7 @@ func TestProvisionerDaemons(t *testing.T) { var err error version, err = client.TemplateVersion(context.Background(), version.ID) return assert.NoError(t, err) && version.Job.Error != "" - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) }) } diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 8706c77d25019..a39312accf729 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -12,6 +12,7 @@ import ( "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" ) @@ -120,7 +121,7 @@ func TestPatchCancelTemplateVersion(t *testing.T) { } t.Logf("Status: %s", version.Job.Status) return version.Job.Status == codersdk.ProvisionerJobRunning - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) err := client.CancelTemplateVersion(context.Background(), version.ID) require.NoError(t, err) err = client.CancelTemplateVersion(context.Background(), version.ID) @@ -131,7 +132,7 @@ func TestPatchCancelTemplateVersion(t *testing.T) { var err error version, err = client.TemplateVersion(context.Background(), version.ID) return assert.NoError(t, err) && version.Job.Status == codersdk.ProvisionerJobFailed - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) }) // TODO(Cian): until we are able to test cancellation properly, validating // Running -> Canceling is the best we can do for now. @@ -155,7 +156,7 @@ func TestPatchCancelTemplateVersion(t *testing.T) { } t.Logf("Status: %s", version.Job.Status) return version.Job.Status == codersdk.ProvisionerJobRunning - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) err := client.CancelTemplateVersion(context.Background(), version.ID) require.NoError(t, err) require.Eventually(t, func() bool { @@ -166,7 +167,7 @@ func TestPatchCancelTemplateVersion(t *testing.T) { // provision complete response. assert.Empty(t, version.Job.Error) && version.Job.Status == codersdk.ProvisionerJobCanceling - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) }) } @@ -541,7 +542,7 @@ func TestTemplateVersionDryRun(t *testing.T) { require.Eventually(t, func() bool { job, err := client.TemplateVersionDryRun(ctx, version.ID, job.ID) return assert.NoError(t, err) && job.Status == codersdk.ProvisionerJobSucceeded - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) <-logsDone @@ -618,7 +619,7 @@ func TestTemplateVersionDryRun(t *testing.T) { t.Logf("Status: %s", job.Status) return job.Status == codersdk.ProvisionerJobPending - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) err = client.CancelTemplateVersionDryRun(context.Background(), version.ID, job.ID) require.NoError(t, err) @@ -631,7 +632,7 @@ func TestTemplateVersionDryRun(t *testing.T) { t.Logf("Status: %s", job.Status) return job.Status == codersdk.ProvisionerJobCanceling - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) }) t.Run("AlreadyCompleted", func(t *testing.T) { @@ -655,7 +656,7 @@ func TestTemplateVersionDryRun(t *testing.T) { t.Logf("Status: %s", job.Status) return job.Status == codersdk.ProvisionerJobSucceeded - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) err = client.CancelTemplateVersionDryRun(context.Background(), version.ID, job.ID) var apiErr *codersdk.Error diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 56f705940a72b..958b2a4bd609c 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -15,6 +15,7 @@ import ( "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" ) @@ -222,7 +223,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { var err error build, err = client.WorkspaceBuild(context.Background(), workspace.LatestBuild.ID) return assert.NoError(t, err) && build.Job.Status == codersdk.ProvisionerJobRunning - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) err := client.CancelWorkspaceBuild(context.Background(), build.ID) require.NoError(t, err) require.Eventually(t, func() bool { @@ -233,7 +234,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { // provision complete response. assert.Empty(t, build.Job.Error) && build.Job.Status == codersdk.ProvisionerJobCanceling - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) } func TestWorkspaceBuildResources(t *testing.T) { diff --git a/internal/testutil/duration.go b/internal/testutil/duration.go new file mode 100644 index 0000000000000..382679a3d823e --- /dev/null +++ b/internal/testutil/duration.go @@ -0,0 +1,20 @@ +//go:build !windows + +package testutil + +import "time" + +// Constants for timing out operations, usable for creating contexts +// that timeout or in require.Eventually. +const ( + WaitShort = 5 * time.Second + WaitLong = 15 * time.Second +) + +// Constants for delaying repeated operations, e.g. in +// require.Eventually. +const ( + IntervalFast = 25 * time.Millisecond + IntervalMedium = 250 * time.Millisecond + IntervalSlow = time.Second +) diff --git a/internal/testutil/duration_windows.go b/internal/testutil/duration_windows.go new file mode 100644 index 0000000000000..1eacba1965893 --- /dev/null +++ b/internal/testutil/duration_windows.go @@ -0,0 +1,22 @@ +package testutil + +import "time" + +// Constants for timing out operations, usable for creating contexts +// that timeout or in require.Eventually. +// +// Windows durations are adjusted for slow CI workers. +const ( + WaitShort = 10 * time.Second + WaitLong = 30 * time.Second +) + +// Constants for delaying repeated operations, e.g. in +// require.Eventually. +// +// Windows durations are adjusted for slow CI workers. +const ( + IntervalFast = 50 * time.Millisecond + IntervalMedium = 500 * time.Millisecond + IntervalSlow = 2 * time.Second +) diff --git a/peer/conn_test.go b/peer/conn_test.go index d1fbf63d15ab6..91663a621a161 100644 --- a/peer/conn_test.go +++ b/peer/conn_test.go @@ -20,6 +20,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/peer" ) @@ -109,7 +110,7 @@ func TestConn(t *testing.T) { t.Parallel() client, server, _ := createPair(t) exchange(t, client, server) - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() cch, err := client.CreateChannel(ctx, "hello", &peer.ChannelOptions{}) require.NoError(t, err) @@ -128,7 +129,7 @@ func TestConn(t *testing.T) { t.Parallel() client, server, wan := createPair(t) exchange(t, client, server) - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() cch, err := client.CreateChannel(ctx, "hello", &peer.ChannelOptions{}) require.NoError(t, err) @@ -148,7 +149,7 @@ func TestConn(t *testing.T) { t.Parallel() client, server, _ := createPair(t) exchange(t, client, server) - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() cch, err := client.CreateChannel(ctx, "hello", &peer.ChannelOptions{}) require.NoError(t, err) @@ -196,7 +197,7 @@ func TestConn(t *testing.T) { srv, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) defer srv.Close() - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() go func() { sch, err := server.Accept(ctx) @@ -314,7 +315,7 @@ func TestConn(t *testing.T) { t.Parallel() client, server, _ := createPair(t) exchange(t, client, server) - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() go func() { channel, err := client.CreateChannel(ctx, "test", nil) diff --git a/provisionerd/provisionerd_test.go b/provisionerd/provisionerd_test.go index fd724babea853..e7865d85699c1 100644 --- a/provisionerd/provisionerd_test.go +++ b/provisionerd/provisionerd_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisionerd/runner" "github.com/hashicorp/yamux" @@ -69,7 +70,7 @@ func TestProvisionerd(t *testing.T) { defer close(completeChan) return nil, xerrors.New("an error") }, provisionerd.Provisioners{}) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.NoError(t, closer.Close()) }) @@ -92,7 +93,7 @@ func TestProvisionerd(t *testing.T) { updateJob: noopUpdateJob, }), nil }, provisionerd.Provisioners{}) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.NoError(t, closer.Close()) }) @@ -137,7 +138,7 @@ func TestProvisionerd(t *testing.T) { }), }) closerMutex.Unlock() - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.NoError(t, closer.Close()) }) @@ -175,7 +176,7 @@ func TestProvisionerd(t *testing.T) { }, provisionerd.Provisioners{ "someprovisioner": createProvisionerClient(t, provisionerTestServer{}), }) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.NoError(t, closer.Close()) }) @@ -218,7 +219,7 @@ func TestProvisionerd(t *testing.T) { }, }), }) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.NoError(t, closer.Close()) }) @@ -323,7 +324,7 @@ func TestProvisionerd(t *testing.T) { }, }), }) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.True(t, didLog.Load()) require.True(t, didComplete.Load()) require.True(t, didDryRun.Load()) @@ -403,7 +404,7 @@ func TestProvisionerd(t *testing.T) { }), }) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.True(t, didLog.Load()) require.True(t, didComplete.Load()) require.NoError(t, closer.Close()) @@ -474,7 +475,7 @@ func TestProvisionerd(t *testing.T) { }, }), }) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.True(t, didLog.Load()) require.True(t, didComplete.Load()) require.NoError(t, closer.Close()) @@ -529,7 +530,7 @@ func TestProvisionerd(t *testing.T) { }, }), }) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.True(t, didFail.Load()) require.NoError(t, closer.Close()) }) @@ -602,10 +603,10 @@ func TestProvisionerd(t *testing.T) { }, }), }) - require.Condition(t, closedWithin(updateChan, 5*time.Second)) + require.Condition(t, closedWithin(updateChan, testutil.WaitShort)) err := server.Shutdown(context.Background()) require.NoError(t, err) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.NoError(t, server.Close()) }) @@ -685,8 +686,8 @@ func TestProvisionerd(t *testing.T) { }, }), }) - require.Condition(t, closedWithin(updateChan, 5*time.Second)) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(updateChan, testutil.WaitShort)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.NoError(t, server.Close()) }) @@ -758,7 +759,7 @@ func TestProvisionerd(t *testing.T) { }, }), }) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.NoError(t, server.Close()) }) @@ -830,7 +831,7 @@ func TestProvisionerd(t *testing.T) { }, }), }) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) require.NoError(t, server.Close()) }) @@ -915,7 +916,7 @@ func TestProvisionerd(t *testing.T) { }, }), }) - require.Condition(t, closedWithin(completeChan, 5*time.Second)) + require.Condition(t, closedWithin(completeChan, testutil.WaitShort)) m.Lock() defer m.Unlock() require.Equal(t, ops[len(ops)-1], "CompleteJob") diff --git a/site/site_test.go b/site/site_test.go index 27b6edcbbf527..4afd51ed9498d 100644 --- a/site/site_test.go +++ b/site/site_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/site" ) @@ -48,7 +49,7 @@ func TestCaching(t *testing.T) { defer srv.Close() // Create a context - ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancelFunc := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancelFunc() testCases := []struct { @@ -337,7 +338,7 @@ func TestServingBin(t *testing.T) { defer srv.Close() // Create a context - ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancelFunc := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancelFunc() for _, tr := range tt.reqs { From f1fb0af795833bb91beb1b419c9a58a5f92d718f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 15:16:04 +0300 Subject: [PATCH 02/15] Fix cases caught by linter --- cli/list_test.go | 4 ++-- cli/server_test.go | 2 +- coderd/provisionerjobs_internal_test.go | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/cli/list_test.go b/cli/list_test.go index 8343f1fead1ab..21cf6eae09b84 100644 --- a/cli/list_test.go +++ b/cli/list_test.go @@ -3,12 +3,12 @@ package cli_test import ( "context" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty/ptytest" ) @@ -29,7 +29,7 @@ func TestList(t *testing.T) { cmd.SetIn(pty.Input()) cmd.SetOut(pty.Output()) - ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancelFunc := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancelFunc() done := make(chan any) go func() { diff --git a/cli/server_test.go b/cli/server_test.go index a2ab757c2cf1f..f6f245e5bdd53 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -213,7 +213,7 @@ func TestServer(t *testing.T) { }, }, } - _, err = client.HasFirstUser(ctx) + _, err := client.HasFirstUser(ctx) require.NoError(t, err) cancelFunc() diff --git a/coderd/provisionerjobs_internal_test.go b/coderd/provisionerjobs_internal_test.go index 6a5cbcccfe5cd..696c328cfc044 100644 --- a/coderd/provisionerjobs_internal_test.go +++ b/coderd/provisionerjobs_internal_test.go @@ -21,6 +21,7 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/database/databasefake" "github.com/coder/coder/codersdk" + "github.com/coder/coder/internal/testutil" ) func TestProvisionerJobLogs_Unit(t *testing.T) { @@ -63,7 +64,7 @@ func TestProvisionerJobLogs_Unit(t *testing.T) { {ID: uuid.New(), JobID: jobID, Stage: "Stage3"}, } - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() // wow there are a lot of DB rows we touch... From 579a96b97fe66b6d512251d0fda79aa239bd493d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 15:16:44 +0300 Subject: [PATCH 03/15] chore: Add linter for timeouts --- scripts/rules.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/scripts/rules.go b/scripts/rules.go index 16917101d16e9..b3a7da5c44eeb 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -81,6 +81,45 @@ func doNotCallTFailNowInsideGoroutine(m dsl.Matcher) { Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)") } +// useStandardTimeoutsAndDelaysInTests ensures all tests use common +// constants for timeouts and delays in usual scenarios, this allows us +// to tweak them based on platform (important to avoid CI flakes). +//nolint:unused,deadcode,varnamelen +func useStandardTimeoutsAndDelaysInTests(m dsl.Matcher) { + // m.Import("testing") + // m.Import("context") + m.Import("github.com/stretchr/testify/require") + m.Import("github.com/stretchr/testify/assert") + m.Import("github.com/coder/coder/internal/testutil") + + m.Match(`context.WithTimeout($ctx, $duration)`). + Where(m.File().Imports("testing") && !m["duration"].Text.Matches("^testutil\\.")). + At(m["duration"]). + Report("Do not use magic numbers in test timeouts and delays. Use the standard testutil.Wait* or testutil.Interval* constants instead.") + + m.Match(` + $testify.$Eventually($t, func() bool { + $*_ + }, $timeout, $interval, $*_) + `). + Where((m["testify"].Text == "require" || m["testify"].Text == "assert") && + (m["Eventually"].Text == "Eventually" || m["Eventually"].Text == "Eventuallyf") && + !m["timeout"].Text.Matches("^testutil\\.")). + At(m["timeout"]). + Report("Do not use magic numbers in test timeouts and delays. Use the standard testutil.Wait* or testutil.Interval* constants instead.") + + m.Match(` + $testify.$Eventually($t, func() bool { + $*_ + }, $timeout, $interval, $*_) + `). + Where((m["testify"].Text == "require" || m["testify"].Text == "assert") && + (m["Eventually"].Text == "Eventually" || m["Eventually"].Text == "Eventuallyf") && + !m["interval"].Text.Matches("^testutil\\.")). + At(m["interval"]). + Report("Do not use magic numbers in test timeouts and delays. Use the standard testutil.Wait* or testutil.Interval* constants instead.") +} + // InTx checks to ensure the database used inside the transaction closure is the transaction // database, and not the original database that creates the tx. func InTx(m dsl.Matcher) { From c17a577af343e9fd92676c489129ae3ae6990b83 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 15:28:33 +0300 Subject: [PATCH 04/15] fix: Commit rest of the modified files --- coderd/coderdtest/coderdtest.go | 9 +++++---- pty/ptytest/ptytest.go | 3 ++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index b392aafffd5b4..8b2cc2346f5f9 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -52,6 +52,7 @@ import ( "github.com/coder/coder/coderd/util/ptr" "github.com/coder/coder/codersdk" "github.com/coder/coder/cryptorand" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionerd" "github.com/coder/coder/provisionersdk" @@ -179,7 +180,7 @@ func newWithCloser(t *testing.T, options *Options) (*codersdk.Client, io.Closer) AgentConnectionUpdateFrequency: 150 * time.Millisecond, // Force a long disconnection timeout to ensure // agents are not marked as disconnected during slow tests. - AgentInactiveDisconnectTimeout: 5 * time.Second, + AgentInactiveDisconnectTimeout: testutil.WaitShort, AccessURL: serverURL, Logger: slogtest.Make(t, nil).Leveled(slog.LevelDebug), CacheDir: t.TempDir(), @@ -412,7 +413,7 @@ func AwaitTemplateVersionJob(t *testing.T, client *codersdk.Client, version uuid var err error templateVersion, err = client.TemplateVersion(context.Background(), version) return assert.NoError(t, err) && templateVersion.Job.CompletedAt != nil - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) return templateVersion } @@ -426,7 +427,7 @@ func AwaitWorkspaceBuildJob(t *testing.T, client *codersdk.Client, build uuid.UU var err error workspaceBuild, err = client.WorkspaceBuild(context.Background(), build) return assert.NoError(t, err) && workspaceBuild.Job.CompletedAt != nil - }, 5*time.Second, 25*time.Millisecond) + }, testutil.WaitShort, testutil.IntervalFast) return workspaceBuild } @@ -450,7 +451,7 @@ func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, build uuid.UUID } } return true - }, 15*time.Second, 50*time.Millisecond) + }, testutil.WaitLong, testutil.IntervalMedium) return resources } diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index dc22351416c50..cb52908ff6ffd 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/xerrors" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty" ) @@ -85,7 +86,7 @@ type PTY struct { func (p *PTY) ExpectMatch(str string) string { p.t.Helper() - timeout, cancel := context.WithTimeout(context.Background(), 10*time.Second) + timeout, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() var buffer bytes.Buffer From cd2930f19e3dee5eaeafc8ac1beff5e03988fc5a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 15:29:36 +0300 Subject: [PATCH 05/15] fix: Caught by lint --- cli/cliui/prompt_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cli/cliui/prompt_test.go b/cli/cliui/prompt_test.go index 3aafd2aeba6f2..701ed18de1033 100644 --- a/cli/cliui/prompt_test.go +++ b/cli/cliui/prompt_test.go @@ -7,7 +7,6 @@ import ( "os" "os/exec" "testing" - "time" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -62,7 +61,7 @@ func TestPrompt(t *testing.T) { // Copy all data written out to a buffer. When we close the ptty, we can // no longer read from the ptty.Output(), but we can read what was // written to the buffer. - dataRead, doneReading := context.WithTimeout(context.Background(), time.Second*2) + dataRead, doneReading := context.WithTimeout(context.Background(), testutil.WaitShort) go func() { // This will throw an error sometimes. The underlying ptty // has its own cleanup routines in t.Cleanup. Instead of From 7fd6449e4b18668dc35534d5588eeaecc8ddd05d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 16:10:46 +0300 Subject: [PATCH 06/15] chore: Add WaitMedium --- internal/testutil/duration.go | 5 +++-- internal/testutil/duration_windows.go | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/testutil/duration.go b/internal/testutil/duration.go index 382679a3d823e..8e3210f155178 100644 --- a/internal/testutil/duration.go +++ b/internal/testutil/duration.go @@ -7,8 +7,9 @@ import "time" // Constants for timing out operations, usable for creating contexts // that timeout or in require.Eventually. const ( - WaitShort = 5 * time.Second - WaitLong = 15 * time.Second + WaitShort = 5 * time.Second + WaitMedium = 10 * time.Second + WaitLong = 15 * time.Second ) // Constants for delaying repeated operations, e.g. in diff --git a/internal/testutil/duration_windows.go b/internal/testutil/duration_windows.go index 1eacba1965893..5707824800bbf 100644 --- a/internal/testutil/duration_windows.go +++ b/internal/testutil/duration_windows.go @@ -7,8 +7,9 @@ import "time" // // Windows durations are adjusted for slow CI workers. const ( - WaitShort = 10 * time.Second - WaitLong = 30 * time.Second + WaitShort = 10 * time.Second + WaitMedium = 20 * time.Second + WaitLong = 30 * time.Second ) // Constants for delaying repeated operations, e.g. in From 8b84b09e9f95be63f58cf3c481cac7f094d0b079 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 16:13:06 +0300 Subject: [PATCH 07/15] fix: Additional lints --- agent/agent_test.go | 3 ++- site/site_test.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index cefa54d037f31..7c253af75893d 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -33,6 +33,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/agent" + "github.com/coder/coder/internal/testutil" "github.com/coder/coder/peer" "github.com/coder/coder/peerbroker" "github.com/coder/coder/peerbroker/proto" @@ -257,7 +258,7 @@ func TestAgent(t *testing.T) { } gotContent = string(content) return true - }, 15*time.Second, 100*time.Millisecond) + }, testutil.WaitMedium, testutil.IntervalMedium) require.Equal(t, content, strings.TrimSpace(gotContent)) }) diff --git a/site/site_test.go b/site/site_test.go index 4afd51ed9498d..bac5a81dc8d7c 100644 --- a/site/site_test.go +++ b/site/site_test.go @@ -109,7 +109,7 @@ func TestServingFiles(t *testing.T) { defer srv.Close() // Create a context - ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancelFunc := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancelFunc() testCases := []struct { From 1e9e5c3f111a2e3576599b113c658809f3a767f2 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 16:32:00 +0300 Subject: [PATCH 08/15] chore: Make waitAccessURL a helper --- cli/server_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cli/server_test.go b/cli/server_test.go index f6f245e5bdd53..61cb5f6bc7a76 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -348,15 +348,17 @@ func generateTLSCertificate(t testing.TB) (certPath, keyPath string) { } func waitAccessURL(t *testing.T, cfg config.Root) *url.URL { + t.Helper() + var err error var rawURL string require.Eventually(t, func() bool { rawURL, err = cfg.URL().Read() return err == nil && rawURL != "" - }, testutil.WaitLong, testutil.IntervalFast) + }, testutil.WaitLong, testutil.IntervalFast, "failed to get access URL") accessURL, err := url.Parse(rawURL) - require.NoError(t, err) + require.NoError(t, err, "failed to parse access URL") return accessURL } From 4047b4d35e9b72b684f824cbf509dc568428aea4 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 16:32:57 +0300 Subject: [PATCH 09/15] chore: Use WaitMedium in waitAccessURL --- cli/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/server_test.go b/cli/server_test.go index 61cb5f6bc7a76..ded2b028a1e20 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -355,7 +355,7 @@ func waitAccessURL(t *testing.T, cfg config.Root) *url.URL { require.Eventually(t, func() bool { rawURL, err = cfg.URL().Read() return err == nil && rawURL != "" - }, testutil.WaitLong, testutil.IntervalFast, "failed to get access URL") + }, testutil.WaitMedium, testutil.IntervalFast, "failed to get access URL") accessURL, err := url.Parse(rawURL) require.NoError(t, err, "failed to parse access URL") From 49d3cb052e437579cec474ea4534fe0d027acc15 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 17:36:10 +0300 Subject: [PATCH 10/15] chore: Revert embedded postgres timeout change --- cli/server_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cli/server_test.go b/cli/server_test.go index ded2b028a1e20..5084cfb26eefa 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -91,7 +91,11 @@ func TestServer(t *testing.T) { go func() { errC <- root.ExecuteContext(ctx) }() - _ = waitAccessURL(t, cfg) + //nolint:gocritic // Embedded postgres take a while to fire up. + require.Eventually(t, func() bool { + rawURL, err := cfg.URL().Read() + return err == nil && rawURL != "" + }, 3*time.Minute, testutil.IntervalFast, "failed to get access URL") cancelFunc() require.ErrorIs(t, <-errC, context.Canceled) }) @@ -355,7 +359,7 @@ func waitAccessURL(t *testing.T, cfg config.Root) *url.URL { require.Eventually(t, func() bool { rawURL, err = cfg.URL().Read() return err == nil && rawURL != "" - }, testutil.WaitMedium, testutil.IntervalFast, "failed to get access URL") + }, testutil.WaitLong, testutil.IntervalFast, "failed to get access URL") accessURL, err := url.Parse(rawURL) require.NoError(t, err, "failed to parse access URL") From 87432e6fefac1dac3a991af7ed82991466a138dd Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 17:54:23 +0300 Subject: [PATCH 11/15] chore: Increase windows ci test timeout from 5m to 10m --- .github/workflows/coder.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/coder.yaml b/.github/workflows/coder.yaml index 9bce6a892d276..879038d97e267 100644 --- a/.github/workflows/coder.yaml +++ b/.github/workflows/coder.yaml @@ -303,7 +303,11 @@ jobs: echo ::set-output name=cover::false fi set -x - gotestsum --junitfile="gotests.xml" --packages="./..." -- -parallel=8 -timeout=5m -short -failfast $COVERAGE_FLAGS + test_timeout=5m + if [[ "${{ matrix.os }}" == windows* ]]; then + test_timeout=10m + fi + gotestsum --junitfile="gotests.xml" --packages="./..." -- -parallel=8 -timeout=$test_timeout -short -failfast $COVERAGE_FLAGS - name: Upload DataDog Trace if: github.actor != 'dependabot[bot]' && !github.event.pull_request.head.repo.fork From a167f37299f813e67c63bb549f5d2451719f81a9 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 17:58:00 +0300 Subject: [PATCH 12/15] chore: Remove commented code --- scripts/rules.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/scripts/rules.go b/scripts/rules.go index b3a7da5c44eeb..83e292663e1cf 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -86,8 +86,6 @@ func doNotCallTFailNowInsideGoroutine(m dsl.Matcher) { // to tweak them based on platform (important to avoid CI flakes). //nolint:unused,deadcode,varnamelen func useStandardTimeoutsAndDelaysInTests(m dsl.Matcher) { - // m.Import("testing") - // m.Import("context") m.Import("github.com/stretchr/testify/require") m.Import("github.com/stretchr/testify/assert") m.Import("github.com/coder/coder/internal/testutil") From 917e4ae73adbc2daad9475ffd029257154a76a93 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 29 Jul 2022 18:20:00 +0300 Subject: [PATCH 13/15] chore: Use WaitMedium for ptytest --- pty/ptytest/ptytest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index cb52908ff6ffd..dcb514e301f78 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -86,7 +86,7 @@ type PTY struct { func (p *PTY) ExpectMatch(str string) string { p.t.Helper() - timeout, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + timeout, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) defer cancel() var buffer bytes.Buffer From 422e72b23072078c17e1f8423c47900b4cdf3500 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 1 Aug 2022 12:56:16 +0300 Subject: [PATCH 14/15] chore: Move testutil from internal to root --- agent/agent_test.go | 2 +- agent/reaper/reaper_test.go | 2 +- cli/cliui/prompt_test.go | 2 +- cli/create_test.go | 2 +- cli/list_test.go | 2 +- cli/portforward_test.go | 2 +- cli/resetpassword_test.go | 2 +- cli/server_test.go | 2 +- cli/ssh_test.go | 2 +- coderd/coderd_test.go | 2 +- coderd/coderdtest/coderdtest.go | 2 +- coderd/devtunnel/tunnel_test.go | 2 +- coderd/httpmw/ratelimit_test.go | 2 +- coderd/provisionerdaemons_test.go | 2 +- coderd/provisionerjobs_internal_test.go | 2 +- coderd/templateversions_test.go | 2 +- coderd/workspacebuilds_test.go | 2 +- peer/conn_test.go | 2 +- provisionerd/provisionerd_test.go | 2 +- pty/ptytest/ptytest.go | 2 +- scripts/rules.go | 2 +- site/site_test.go | 2 +- {internal/testutil => testutil}/duration.go | 0 {internal/testutil => testutil}/duration_windows.go | 0 24 files changed, 22 insertions(+), 22 deletions(-) rename {internal/testutil => testutil}/duration.go (100%) rename {internal/testutil => testutil}/duration_windows.go (100%) diff --git a/agent/agent_test.go b/agent/agent_test.go index 7c253af75893d..e3d3a80886c02 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -33,12 +33,12 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/agent" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/peer" "github.com/coder/coder/peerbroker" "github.com/coder/coder/peerbroker/proto" "github.com/coder/coder/provisionersdk" "github.com/coder/coder/pty/ptytest" + "github.com/coder/coder/testutil" ) func TestMain(m *testing.M) { diff --git a/agent/reaper/reaper_test.go b/agent/reaper/reaper_test.go index b3a8370dfaeca..88573636ee39c 100644 --- a/agent/reaper/reaper_test.go +++ b/agent/reaper/reaper_test.go @@ -12,7 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/agent/reaper" - "github.com/coder/coder/internal/testutil" + "github.com/coder/coder/testutil" ) func TestReap(t *testing.T) { diff --git a/cli/cliui/prompt_test.go b/cli/cliui/prompt_test.go index 701ed18de1033..6c7f233c872e6 100644 --- a/cli/cliui/prompt_test.go +++ b/cli/cliui/prompt_test.go @@ -13,9 +13,9 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/cli/cliui" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty" "github.com/coder/coder/pty/ptytest" + "github.com/coder/coder/testutil" ) func TestPrompt(t *testing.T) { diff --git a/cli/create_test.go b/cli/create_test.go index 988c85113cbc0..cdb8f5e5e8019 100644 --- a/cli/create_test.go +++ b/cli/create_test.go @@ -13,10 +13,10 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" "github.com/coder/coder/pty/ptytest" + "github.com/coder/coder/testutil" ) func TestCreate(t *testing.T) { diff --git a/cli/list_test.go b/cli/list_test.go index 21cf6eae09b84..a5aaeb5f4da30 100644 --- a/cli/list_test.go +++ b/cli/list_test.go @@ -8,8 +8,8 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty/ptytest" + "github.com/coder/coder/testutil" ) func TestList(t *testing.T) { diff --git a/cli/portforward_test.go b/cli/portforward_test.go index d8071ab52824f..a3547ba4dface 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -21,9 +21,9 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" + "github.com/coder/coder/testutil" ) func TestPortForward(t *testing.T) { diff --git a/cli/resetpassword_test.go b/cli/resetpassword_test.go index faf82fb2e749b..420f3793b2f09 100644 --- a/cli/resetpassword_test.go +++ b/cli/resetpassword_test.go @@ -12,8 +12,8 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/database/postgres" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty/ptytest" + "github.com/coder/coder/testutil" ) // nolint:paralleltest diff --git a/cli/server_test.go b/cli/server_test.go index 5084cfb26eefa..8ad34024872a4 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -30,8 +30,8 @@ import ( "github.com/coder/coder/coderd/database/postgres" "github.com/coder/coder/coderd/telemetry" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty/ptytest" + "github.com/coder/coder/testutil" ) // This cannot be ran in parallel because it uses a signal. diff --git a/cli/ssh_test.go b/cli/ssh_test.go index d0d5f581f77f8..6978e5da298ba 100644 --- a/cli/ssh_test.go +++ b/cli/ssh_test.go @@ -26,10 +26,10 @@ import ( "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" "github.com/coder/coder/pty/ptytest" + "github.com/coder/coder/testutil" ) func setupWorkspaceForSSH(t *testing.T) (*codersdk.Client, codersdk.Workspace, string) { diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 28df87ce1ac08..4eab98170f55d 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -38,9 +38,9 @@ import ( "github.com/coder/coder/coderd/telemetry" "github.com/coder/coder/coderd/turnconn" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" + "github.com/coder/coder/testutil" ) func TestMain(m *testing.M) { diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 8b2cc2346f5f9..4e96f112469f1 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -52,11 +52,11 @@ import ( "github.com/coder/coder/coderd/util/ptr" "github.com/coder/coder/codersdk" "github.com/coder/coder/cryptorand" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionerd" "github.com/coder/coder/provisionersdk" "github.com/coder/coder/provisionersdk/proto" + "github.com/coder/coder/testutil" ) type Options struct { diff --git a/coderd/devtunnel/tunnel_test.go b/coderd/devtunnel/tunnel_test.go index c91bc18cfe09c..23dd92d966d54 100644 --- a/coderd/devtunnel/tunnel_test.go +++ b/coderd/devtunnel/tunnel_test.go @@ -25,7 +25,7 @@ import ( "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/coderd/devtunnel" - "github.com/coder/coder/internal/testutil" + "github.com/coder/coder/testutil" ) const ( diff --git a/coderd/httpmw/ratelimit_test.go b/coderd/httpmw/ratelimit_test.go index b0034d0784c00..132615d669ab7 100644 --- a/coderd/httpmw/ratelimit_test.go +++ b/coderd/httpmw/ratelimit_test.go @@ -9,7 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/httpmw" - "github.com/coder/coder/internal/testutil" + "github.com/coder/coder/testutil" ) func TestRateLimit(t *testing.T) { diff --git a/coderd/provisionerdaemons_test.go b/coderd/provisionerdaemons_test.go index bd1667737baac..709dc2eb0c092 100644 --- a/coderd/provisionerdaemons_test.go +++ b/coderd/provisionerdaemons_test.go @@ -11,8 +11,8 @@ import ( "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisionersdk" + "github.com/coder/coder/testutil" ) func TestProvisionerDaemons(t *testing.T) { diff --git a/coderd/provisionerjobs_internal_test.go b/coderd/provisionerjobs_internal_test.go index 696c328cfc044..4d215f6bb2a92 100644 --- a/coderd/provisionerjobs_internal_test.go +++ b/coderd/provisionerjobs_internal_test.go @@ -21,7 +21,7 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/database/databasefake" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" + "github.com/coder/coder/testutil" ) func TestProvisionerJobLogs_Unit(t *testing.T) { diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index a39312accf729..9c6c76a927fdd 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -12,9 +12,9 @@ import ( "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" + "github.com/coder/coder/testutil" ) func TestTemplateVersion(t *testing.T) { diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 958b2a4bd609c..8b68cff559c94 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -15,9 +15,9 @@ import ( "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" "github.com/coder/coder/codersdk" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" + "github.com/coder/coder/testutil" ) func TestWorkspaceBuild(t *testing.T) { diff --git a/peer/conn_test.go b/peer/conn_test.go index 91663a621a161..6e2de345f0d9e 100644 --- a/peer/conn_test.go +++ b/peer/conn_test.go @@ -20,8 +20,8 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/peer" + "github.com/coder/coder/testutil" ) var ( diff --git a/provisionerd/provisionerd_test.go b/provisionerd/provisionerd_test.go index e7865d85699c1..05becb863ebd7 100644 --- a/provisionerd/provisionerd_test.go +++ b/provisionerd/provisionerd_test.go @@ -12,8 +12,8 @@ import ( "testing" "time" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/provisionerd/runner" + "github.com/coder/coder/testutil" "github.com/hashicorp/yamux" "github.com/stretchr/testify/assert" diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index dcb514e301f78..2c79ae1991d1d 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -16,8 +16,8 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/xerrors" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/pty" + "github.com/coder/coder/testutil" ) func New(t *testing.T) *PTY { diff --git a/scripts/rules.go b/scripts/rules.go index 83e292663e1cf..d77289388ba1c 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -88,7 +88,7 @@ func doNotCallTFailNowInsideGoroutine(m dsl.Matcher) { func useStandardTimeoutsAndDelaysInTests(m dsl.Matcher) { m.Import("github.com/stretchr/testify/require") m.Import("github.com/stretchr/testify/assert") - m.Import("github.com/coder/coder/internal/testutil") + m.Import("github.com/coder/coder/testutil") m.Match(`context.WithTimeout($ctx, $duration)`). Where(m.File().Imports("testing") && !m["duration"].Text.Matches("^testutil\\.")). diff --git a/site/site_test.go b/site/site_test.go index bac5a81dc8d7c..7b28ac91b3a6a 100644 --- a/site/site_test.go +++ b/site/site_test.go @@ -19,8 +19,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/coder/coder/internal/testutil" "github.com/coder/coder/site" + "github.com/coder/coder/testutil" ) func TestCaching(t *testing.T) { diff --git a/internal/testutil/duration.go b/testutil/duration.go similarity index 100% rename from internal/testutil/duration.go rename to testutil/duration.go diff --git a/internal/testutil/duration_windows.go b/testutil/duration_windows.go similarity index 100% rename from internal/testutil/duration_windows.go rename to testutil/duration_windows.go From 43629d3cf6aeb331f5d9741e89b172ad6bdf8c2e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 1 Aug 2022 14:59:45 +0300 Subject: [PATCH 15/15] chore: Fix lint errors caught in main merge --- cli/server_test.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/cli/server_test.go b/cli/server_test.go index 6cb8cfab665a8..ed9676f98f391 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -162,11 +162,7 @@ func TestServer(t *testing.T) { }() // Just wait for startup - require.Eventually(t, func() bool { - var err error - _, err = cfg.URL().Read() - return err == nil - }, 15*time.Second, 25*time.Millisecond) + _ = waitAccessURL(t, cfg) cancelFunc() require.ErrorIs(t, <-errC, context.Canceled) @@ -194,11 +190,7 @@ func TestServer(t *testing.T) { }() // Just wait for startup - require.Eventually(t, func() bool { - var err error - _, err = cfg.URL().Read() - return err == nil - }, 15*time.Second, 25*time.Millisecond) + _ = waitAccessURL(t, cfg) cancelFunc() require.ErrorIs(t, <-errC, context.Canceled)