From 8ea5fb7115dfdd478f01770d38e3e8267150a59a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 21 Mar 2024 15:38:38 +0000 Subject: [PATCH] chore(coderd/workspaceapps/apptest): fix test flake due to concurrent usage of same deployment (#12700) - Updates existing tests under workspaceapps/apptest to not reuse existing appDetails as assertWorkspaceLastUsed(Not)?Updated calls FlushStats() which was causing racy test behaviour and incorrect test assertions. - Expands scope of assertWorkspaceLastUsedAtUpdated and its counterpart to ProxySubdomain tests. --- coderd/workspaceapps/apptest/apptest.go | 90 ++++++++++++++++++------- 1 file changed, 67 insertions(+), 23 deletions(-) diff --git a/coderd/workspaceapps/apptest/apptest.go b/coderd/workspaceapps/apptest/apptest.go index 6a17df402f..c532713894 100644 --- a/coderd/workspaceapps/apptest/apptest.go +++ b/coderd/workspaceapps/apptest/apptest.go @@ -70,6 +70,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("SignedTokenQueryParameter", func(t *testing.T) { t.Parallel() + if appHostIsPrimary { t.Skip("Tickets are not used for terminal requests on the primary.") } @@ -101,8 +102,6 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("WorkspaceAppsProxyPath", func(t *testing.T) { t.Parallel() - appDetails := setupProxyTest(t, nil) - t.Run("Disabled", func(t *testing.T) { t.Parallel() @@ -132,6 +131,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Skip("This test only applies when testing apps on the primary.") } + appDetails := setupProxyTest(t, nil) unauthedClient := appDetails.AppClient(t) unauthedClient.SetSessionToken("") @@ -148,7 +148,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.True(t, loc.Query().Has("message")) require.True(t, loc.Query().Has("redirect")) - assertWorkspaceLastUsedAtUpdated(t, appDetails) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("LoginWithoutAuthOnProxy", func(t *testing.T) { @@ -158,6 +158,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Skip("This test only applies when testing apps on workspace proxies.") } + appDetails := setupProxyTest(t, nil) unauthedClient := appDetails.AppClient(t) unauthedClient.SetSessionToken("") @@ -186,12 +187,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // request is getting stripped. require.Equal(t, u.Path, redirectURI.Path+"/") require.Equal(t, u.RawQuery, redirectURI.RawQuery) - assertWorkspaceLastUsedAtUpdated(t, appDetails) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("NoAccessShould404", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) userClient, _ := coderdtest.CreateAnotherUser(t, appDetails.SDKClient, appDetails.FirstUser.OrganizationID, rbac.RoleMember()) userAppClient := appDetails.AppClient(t) userAppClient.SetSessionToken(userClient.SessionToken()) @@ -210,6 +212,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("RedirectsWithSlash", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -226,6 +229,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("RedirectsWithQuery", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -245,6 +249,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("Proxies", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -333,6 +338,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("BlocksMe", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -347,13 +353,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.Contains(t, string(body), "must be accessed with the full username, not @me") - // TODO(cian): A blocked request should not count as workspace usage. - // assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("ForwardsIP", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -373,6 +379,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("ProxyError", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -388,6 +395,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("NoProxyPort", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -397,7 +405,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // TODO(@deansheather): This should be 400. There's a todo in the // resolve request code to fix this. require.Equal(t, http.StatusInternalServerError, resp.StatusCode) - assertWorkspaceLastUsedAtUpdated(t, appDetails) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) }) @@ -636,6 +644,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { _ = resp.Body.Close() require.Equal(t, http.StatusOK, resp.StatusCode) require.Equal(t, resp.Header.Get("X-Got-Host"), u.Host) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("WorkspaceAppsProxySubdomainHostnamePrefix/Different", func(t *testing.T) { @@ -686,6 +695,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) _ = resp.Body.Close() require.NotEqual(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) // This test ensures that the subdomain handler does nothing if @@ -754,11 +764,10 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { t.Run("WorkspaceAppsProxySubdomain", func(t *testing.T) { t.Parallel() - appDetails := setupProxyTest(t, nil) - t.Run("NoAccessShould401", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) userClient, _ := coderdtest.CreateAnotherUser(t, appDetails.SDKClient, appDetails.FirstUser.OrganizationID, rbac.RoleMember()) userAppClient := appDetails.AppClient(t) userAppClient.SetSessionToken(userClient.SessionToken()) @@ -770,11 +779,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusNotFound, resp.StatusCode) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("RedirectsWithSlash", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -789,11 +800,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { loc, err := resp.Location() require.NoError(t, err) require.Equal(t, appDetails.SubdomainAppURL(appDetails.Apps.Owner).Path, loc.Path) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("RedirectsWithQuery", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -807,11 +820,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { loc, err := resp.Location() require.NoError(t, err) require.Equal(t, appDetails.SubdomainAppURL(appDetails.Apps.Owner).RawQuery, loc.RawQuery) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("Proxies", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -848,6 +863,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("ProxiesHTTPS", func(t *testing.T) { @@ -892,11 +908,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("ProxiesPort", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -907,11 +925,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("ProxyError", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -919,11 +939,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusBadGateway, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("ProxyPortMinimumError", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, nil) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -939,6 +961,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { err = json.NewDecoder(resp.Body).Decode(&resBody) require.NoError(t, err) require.Contains(t, resBody.Message, "Coder reserves ports less than") + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("SuffixWildcardOK", func(t *testing.T) { @@ -961,6 +984,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("WildcardPortOK", func(t *testing.T) { @@ -993,16 +1017,19 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) require.Equal(t, proxyTestAppBody, string(body)) require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("SuffixWildcardNotMatch", func(t *testing.T) { t.Parallel() - appDetails := setupProxyTest(t, &DeploymentOptions{ - AppHost: "*-suffix.test.coder.com", - }) - t.Run("NoSuffix", func(t *testing.T) { + t.Parallel() + + appDetails := setupProxyTest(t, &DeploymentOptions{ + AppHost: "*-suffix.test.coder.com", + }) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -1020,11 +1047,16 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // It's probably rendering the dashboard or a 404 page, so only // ensure that the body doesn't match. require.NotContains(t, string(body), proxyTestAppBody) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("DifferentSuffix", func(t *testing.T) { t.Parallel() + appDetails := setupProxyTest(t, &DeploymentOptions{ + AppHost: "*-suffix.test.coder.com", + }) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -1042,6 +1074,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { // It's probably rendering the dashboard, so only ensure that the body // doesn't match. require.NotContains(t, string(body), proxyTestAppBody) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) }) }) @@ -1062,6 +1095,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusNotFound, resp.StatusCode) + assertWorkspaceLastUsedAtNotUpdated(t, appDetails) }) t.Run("AuthenticatedOK", func(t *testing.T) { @@ -1090,6 +1124,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("PublicOK", func(t *testing.T) { @@ -1117,6 +1152,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) t.Run("HTTPS", func(t *testing.T) { @@ -1145,6 +1181,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { require.NoError(t, err) defer resp.Body.Close() require.Equal(t, http.StatusOK, resp.StatusCode) + assertWorkspaceLastUsedAtUpdated(t, appDetails) }) }) @@ -1719,26 +1756,33 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli } // Accessing an app should update the workspace's LastUsedAt. -// NOTE: Despite our efforts with the flush channel, this is inherently racy. +// NOTE: Despite our efforts with the flush channel, this is inherently racy when used with +// parallel tests on the same workspace/app. func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) { t.Helper() + require.NotNil(t, details.Workspace, "can't assert LastUsedAt on a nil workspace!") + before, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) + require.NoError(t, err) // Wait for stats to fully flush. + details.FlushStats() require.Eventually(t, func() bool { - details.FlushStats() - ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) - assert.NoError(t, err) - return ws.LastUsedAt.After(details.Workspace.LastUsedAt) - }, testutil.WaitShort, testutil.IntervalMedium, "workspace LastUsedAt not updated when it should have been") + after, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) + return assert.NoError(t, err) && after.LastUsedAt.After(before.LastUsedAt) + }, testutil.WaitShort, testutil.IntervalMedium) } // Except when it sometimes shouldn't (e.g. no access) -// NOTE: Despite our efforts with the flush channel, this is inherently racy. +// NOTE: Despite our efforts with the flush channel, this is inherently racy when used with +// parallel tests on the same workspace/app. func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, details *Details) { t.Helper() - details.FlushStats() - ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) + require.NotNil(t, details.Workspace, "can't assert LastUsedAt on a nil workspace!") + before, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) require.NoError(t, err) - require.Equal(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt updated when it should not have been") + details.FlushStats() + after, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID) + require.NoError(t, err) + require.Equal(t, before.LastUsedAt, after.LastUsedAt, "workspace LastUsedAt updated when it should not have been") }