Skip to content

Commit 91ecf37

Browse files
johnstcnjohnstcn
committed
chore(coderd/workspaceapps/apptest): fix test flake due to concurrent usage of same deployment
1 parent 131d0bd commit 91ecf37

File tree

1 file changed

+66
-23
lines changed

1 file changed

+66
-23
lines changed

‎coderd/workspaceapps/apptest/apptest.go

Lines changed: 66 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
7070

7171
t.Run("SignedTokenQueryParameter", func(t *testing.T) {
7272
t.Parallel()
73+
7374
if appHostIsPrimary {
7475
t.Skip("Tickets are not used for terminal requests on the primary.")
7576
}
@@ -101,8 +102,6 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
101102
t.Run("WorkspaceAppsProxyPath", func(t *testing.T) {
102103
t.Parallel()
103104

104-
appDetails := setupProxyTest(t, nil)
105-
106105
t.Run("Disabled", func(t *testing.T) {
107106
t.Parallel()
108107

@@ -132,6 +131,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
132131
t.Skip("This test only applies when testing apps on the primary.")
133132
}
134133

134+
appDetails := setupProxyTest(t, nil)
135135
unauthedClient := appDetails.AppClient(t)
136136
unauthedClient.SetSessionToken("")
137137

@@ -148,7 +148,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
148148
require.NoError(t, err)
149149
require.True(t, loc.Query().Has("message"))
150150
require.True(t, loc.Query().Has("redirect"))
151-
assertWorkspaceLastUsedAtUpdated(t, appDetails)
151+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
152152
})
153153

154154
t.Run("LoginWithoutAuthOnProxy", func(t *testing.T) {
@@ -158,6 +158,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
158158
t.Skip("This test only applies when testing apps on workspace proxies.")
159159
}
160160

161+
appDetails := setupProxyTest(t, nil)
161162
unauthedClient := appDetails.AppClient(t)
162163
unauthedClient.SetSessionToken("")
163164

@@ -186,12 +187,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
186187
// request is getting stripped.
187188
require.Equal(t, u.Path, redirectURI.Path+"/")
188189
require.Equal(t, u.RawQuery, redirectURI.RawQuery)
189-
assertWorkspaceLastUsedAtUpdated(t, appDetails)
190+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
190191
})
191192

192193
t.Run("NoAccessShould404", func(t *testing.T) {
193194
t.Parallel()
194195

196+
appDetails := setupProxyTest(t, nil)
195197
userClient, _ := coderdtest.CreateAnotherUser(t, appDetails.SDKClient, appDetails.FirstUser.OrganizationID, rbac.RoleMember())
196198
userAppClient := appDetails.AppClient(t)
197199
userAppClient.SetSessionToken(userClient.SessionToken())
@@ -210,6 +212,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
210212
t.Run("RedirectsWithSlash", func(t *testing.T) {
211213
t.Parallel()
212214

215+
appDetails := setupProxyTest(t, nil)
213216
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
214217
defer cancel()
215218

@@ -226,6 +229,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
226229
t.Run("RedirectsWithQuery", func(t *testing.T) {
227230
t.Parallel()
228231

232+
appDetails := setupProxyTest(t, nil)
229233
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
230234
defer cancel()
231235

@@ -245,6 +249,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
245249
t.Run("Proxies", func(t *testing.T) {
246250
t.Parallel()
247251

252+
appDetails := setupProxyTest(t, nil)
248253
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
249254
defer cancel()
250255

@@ -333,6 +338,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
333338
t.Run("BlocksMe", func(t *testing.T) {
334339
t.Parallel()
335340

341+
appDetails := setupProxyTest(t, nil)
336342
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
337343
defer cancel()
338344

@@ -347,13 +353,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
347353
body, err := io.ReadAll(resp.Body)
348354
require.NoError(t, err)
349355
require.Contains(t, string(body), "must be accessed with the full username, not @me")
350-
// TODO(cian): A blocked request should not count as workspace usage.
351-
// assertWorkspaceLastUsedAtNotUpdated(t, appDetails.AppClient(t), appDetails)
356+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
352357
})
353358

354359
t.Run("ForwardsIP", func(t *testing.T) {
355360
t.Parallel()
356361

362+
appDetails := setupProxyTest(t, nil)
357363
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
358364
defer cancel()
359365

@@ -373,6 +379,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
373379
t.Run("ProxyError", func(t *testing.T) {
374380
t.Parallel()
375381

382+
appDetails := setupProxyTest(t, nil)
376383
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
377384
defer cancel()
378385

@@ -388,6 +395,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
388395
t.Run("NoProxyPort", func(t *testing.T) {
389396
t.Parallel()
390397

398+
appDetails := setupProxyTest(t, nil)
391399
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
392400
defer cancel()
393401

@@ -397,7 +405,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
397405
// TODO(@deansheather): This should be 400. There's a todo in the
398406
// resolve request code to fix this.
399407
require.Equal(t, http.StatusInternalServerError, resp.StatusCode)
400-
assertWorkspaceLastUsedAtUpdated(t, appDetails)
408+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
401409
})
402410
})
403411

@@ -636,6 +644,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
636644
_ = resp.Body.Close()
637645
require.Equal(t, http.StatusOK, resp.StatusCode)
638646
require.Equal(t, resp.Header.Get("X-Got-Host"), u.Host)
647+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
639648
})
640649

641650
t.Run("WorkspaceAppsProxySubdomainHostnamePrefix/Different", func(t *testing.T) {
@@ -686,6 +695,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
686695
require.NoError(t, err)
687696
_ = resp.Body.Close()
688697
require.NotEqual(t, http.StatusOK, resp.StatusCode)
698+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
689699
})
690700

691701
// This test ensures that the subdomain handler does nothing if
@@ -754,11 +764,10 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
754764
t.Run("WorkspaceAppsProxySubdomain", func(t *testing.T) {
755765
t.Parallel()
756766

757-
appDetails := setupProxyTest(t, nil)
758-
759767
t.Run("NoAccessShould401", func(t *testing.T) {
760768
t.Parallel()
761769

770+
appDetails := setupProxyTest(t, nil)
762771
userClient, _ := coderdtest.CreateAnotherUser(t, appDetails.SDKClient, appDetails.FirstUser.OrganizationID, rbac.RoleMember())
763772
userAppClient := appDetails.AppClient(t)
764773
userAppClient.SetSessionToken(userClient.SessionToken())
@@ -770,11 +779,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
770779
require.NoError(t, err)
771780
defer resp.Body.Close()
772781
require.Equal(t, http.StatusNotFound, resp.StatusCode)
782+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
773783
})
774784

775785
t.Run("RedirectsWithSlash", func(t *testing.T) {
776786
t.Parallel()
777787

788+
appDetails := setupProxyTest(t, nil)
778789
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
779790
defer cancel()
780791

@@ -789,11 +800,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
789800
loc, err := resp.Location()
790801
require.NoError(t, err)
791802
require.Equal(t, appDetails.SubdomainAppURL(appDetails.Apps.Owner).Path, loc.Path)
803+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
792804
})
793805

794806
t.Run("RedirectsWithQuery", func(t *testing.T) {
795807
t.Parallel()
796808

809+
appDetails := setupProxyTest(t, nil)
797810
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
798811
defer cancel()
799812

@@ -807,11 +820,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
807820
loc, err := resp.Location()
808821
require.NoError(t, err)
809822
require.Equal(t, appDetails.SubdomainAppURL(appDetails.Apps.Owner).RawQuery, loc.RawQuery)
823+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
810824
})
811825

812826
t.Run("Proxies", func(t *testing.T) {
813827
t.Parallel()
814828

829+
appDetails := setupProxyTest(t, nil)
815830
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
816831
defer cancel()
817832

@@ -848,6 +863,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
848863
require.NoError(t, err)
849864
require.Equal(t, proxyTestAppBody, string(body))
850865
require.Equal(t, http.StatusOK, resp.StatusCode)
866+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
851867
})
852868

853869
t.Run("ProxiesHTTPS", func(t *testing.T) {
@@ -892,11 +908,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
892908
require.NoError(t, err)
893909
require.Equal(t, proxyTestAppBody, string(body))
894910
require.Equal(t, http.StatusOK, resp.StatusCode)
911+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
895912
})
896913

897914
t.Run("ProxiesPort", func(t *testing.T) {
898915
t.Parallel()
899916

917+
appDetails := setupProxyTest(t, nil)
900918
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
901919
defer cancel()
902920

@@ -907,23 +925,27 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
907925
require.NoError(t, err)
908926
require.Equal(t, proxyTestAppBody, string(body))
909927
require.Equal(t, http.StatusOK, resp.StatusCode)
928+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
910929
})
911930

912931
t.Run("ProxyError", func(t *testing.T) {
913932
t.Parallel()
914933

934+
appDetails := setupProxyTest(t, nil)
915935
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
916936
defer cancel()
917937

918938
resp, err := appDetails.AppClient(t).Request(ctx, http.MethodGet, appDetails.SubdomainAppURL(appDetails.Apps.Fake).String(), nil)
919939
require.NoError(t, err)
920940
defer resp.Body.Close()
921941
require.Equal(t, http.StatusBadGateway, resp.StatusCode)
942+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
922943
})
923944

924945
t.Run("ProxyPortMinimumError", func(t *testing.T) {
925946
t.Parallel()
926947

948+
appDetails := setupProxyTest(t, nil)
927949
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
928950
defer cancel()
929951

@@ -939,6 +961,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
939961
err = json.NewDecoder(resp.Body).Decode(&resBody)
940962
require.NoError(t, err)
941963
require.Contains(t, resBody.Message, "Coder reserves ports less than")
964+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
942965
})
943966

944967
t.Run("SuffixWildcardOK", func(t *testing.T) {
@@ -961,6 +984,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
961984
require.NoError(t, err)
962985
require.Equal(t, proxyTestAppBody, string(body))
963986
require.Equal(t, http.StatusOK, resp.StatusCode)
987+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
964988
})
965989

966990
t.Run("WildcardPortOK", func(t *testing.T) {
@@ -993,16 +1017,19 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
9931017
require.NoError(t, err)
9941018
require.Equal(t, proxyTestAppBody, string(body))
9951019
require.Equal(t, http.StatusOK, resp.StatusCode)
1020+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
9961021
})
9971022

9981023
t.Run("SuffixWildcardNotMatch", func(t *testing.T) {
9991024
t.Parallel()
10001025

1001-
appDetails := setupProxyTest(t, &DeploymentOptions{
1002-
AppHost: "*-suffix.test.coder.com",
1003-
})
1004-
10051026
t.Run("NoSuffix", func(t *testing.T) {
1027+
t.Parallel()
1028+
1029+
appDetails := setupProxyTest(t, &DeploymentOptions{
1030+
AppHost: "*-suffix.test.coder.com",
1031+
})
1032+
10061033
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
10071034
defer cancel()
10081035

@@ -1020,11 +1047,16 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
10201047
// It's probably rendering the dashboard or a 404 page, so only
10211048
// ensure that the body doesn't match.
10221049
require.NotContains(t, string(body), proxyTestAppBody)
1050+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
10231051
})
10241052

10251053
t.Run("DifferentSuffix", func(t *testing.T) {
10261054
t.Parallel()
10271055

1056+
appDetails := setupProxyTest(t, &DeploymentOptions{
1057+
AppHost: "*-suffix.test.coder.com",
1058+
})
1059+
10281060
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
10291061
defer cancel()
10301062

@@ -1042,6 +1074,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
10421074
// It's probably rendering the dashboard, so only ensure that the body
10431075
// doesn't match.
10441076
require.NotContains(t, string(body), proxyTestAppBody)
1077+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
10451078
})
10461079
})
10471080
})
@@ -1062,6 +1095,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
10621095
require.NoError(t, err)
10631096
defer resp.Body.Close()
10641097
require.Equal(t, http.StatusNotFound, resp.StatusCode)
1098+
assertWorkspaceLastUsedAtNotUpdated(t, appDetails)
10651099
})
10661100

10671101
t.Run("AuthenticatedOK", func(t *testing.T) {
@@ -1090,6 +1124,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
10901124
require.NoError(t, err)
10911125
defer resp.Body.Close()
10921126
require.Equal(t, http.StatusOK, resp.StatusCode)
1127+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
10931128
})
10941129

10951130
t.Run("PublicOK", func(t *testing.T) {
@@ -1117,6 +1152,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
11171152
require.NoError(t, err)
11181153
defer resp.Body.Close()
11191154
require.Equal(t, http.StatusOK, resp.StatusCode)
1155+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
11201156
})
11211157

11221158
t.Run("HTTPS", func(t *testing.T) {
@@ -1145,6 +1181,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
11451181
require.NoError(t, err)
11461182
defer resp.Body.Close()
11471183
require.Equal(t, http.StatusOK, resp.StatusCode)
1184+
assertWorkspaceLastUsedAtUpdated(t, appDetails)
11481185
})
11491186
})
11501187

@@ -1719,26 +1756,32 @@ func testReconnectingPTY(ctx context.Context, t *testing.T, client *codersdk.Cli
17191756
}
17201757

17211758
// Accessing an app should update the workspace's LastUsedAt.
1722-
// NOTE: Despite our efforts with the flush channel, this is inherently racy.
1759+
// NOTE: Despite our efforts with the flush channel, this is inherently racy when used with
1760+
// parallel tests on the same workspace/app.
17231761
func assertWorkspaceLastUsedAtUpdated(t testing.TB, details *Details) {
17241762
t.Helper()
17251763

1764+
require.NotNil(t, details.Workspace, "can't assert LastUsedAt on a nil workspace!")
1765+
before, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID)
1766+
require.NoError(t, err)
17261767
// Wait for stats to fully flush.
1727-
require.Eventually(t, func() bool {
1728-
details.FlushStats()
1729-
ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID)
1730-
assert.NoError(t, err)
1731-
return ws.LastUsedAt.After(details.Workspace.LastUsedAt)
1732-
}, testutil.WaitShort, testutil.IntervalMedium, "workspace LastUsedAt not updated when it should have been")
1768+
details.FlushStats()
1769+
after, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID)
1770+
require.NoError(t, err)
1771+
require.Greater(t, after.LastUsedAt, before.LastUsedAt, "workspace LastUsedAt not updated when it should have been")
17331772
}
17341773

17351774
// Except when it sometimes shouldn't (e.g. no access)
1736-
// NOTE: Despite our efforts with the flush channel, this is inherently racy.
1775+
// NOTE: Despite our efforts with the flush channel, this is inherently racy when used with
1776+
// parallel tests on the same workspace/app.
17371777
func assertWorkspaceLastUsedAtNotUpdated(t testing.TB, details *Details) {
17381778
t.Helper()
17391779

1780+
require.NotNil(t, details.Workspace, "can't assert LastUsedAt on a nil workspace!")
1781+
before, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID)
1782+
require.NoError(t, err)
17401783
details.FlushStats()
1741-
ws, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID)
1784+
after, err := details.SDKClient.Workspace(context.Background(), details.Workspace.ID)
17421785
require.NoError(t, err)
1743-
require.Equal(t, ws.LastUsedAt, details.Workspace.LastUsedAt, "workspace LastUsedAt updated when it should not have been")
1786+
require.Equal(t, before.LastUsedAt, after.LastUsedAt, "workspace LastUsedAt updated when it should not have been")
17441787
}

0 commit comments

Comments
 (0)