fix(workspaceapps): use fresh context in LastUsedAt assertions#22863
fix(workspaceapps): use fresh context in LastUsedAt assertions#22863
Conversation
The assertWorkspaceLastUsedAtUpdated and assertWorkspaceLastUsedAtNotUpdated helpers previously accepted a context.Context, which callers shared with preceding HTTP requests. In ProxyError tests the request targets a fake unreachable app, and the reverse-proxy timeout can consume most of the context budget — especially on Windows — leaving too little time for the Eventually polling loop and causing flakes. Replace the context parameter with a time.Duration so each assertion creates its own fresh context. This makes the timeout budget explicit at every call site and structurally prevents shared-context starvation. Fixes coder/internal#1385
There was a problem hiding this comment.
Pull request overview
Updates workspace app test helpers to avoid flakiness caused by reusing a partially-consumed context.Context (notably after reverse-proxy timeouts), by giving each LastUsedAt assertion a fresh timeout budget.
Changes:
- Change
assertWorkspaceLastUsedAtUpdated/assertWorkspaceLastUsedAtNotUpdatedto accept atime.Durationand create a fresh context internally. - Update all call sites in the workspaceapps apptest suite to pass
testutil.WaitLong. - Add explanatory comments documenting why a duration is preferred over a shared context.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Even though path-based apps are disabled, the request should indicate | ||
| // that the workspace was used. |
There was a problem hiding this comment.
The comment says the forbidden request "should indicate that the workspace was used", but the test calls assertWorkspaceLastUsedAtNotUpdated (i.e., expects LastUsedAt to not change). Please reconcile this mismatch by either updating the comment or switching to the *Updated assertion, so the test intent is unambiguous.
| // Even though path-based apps are disabled, the request should indicate | |
| // that the workspace was used. | |
| // Even though path-based apps are disabled, this forbidden request should | |
| // not indicate that the workspace was used. |
| @@ -377,7 +377,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) { | |||
| require.Equal(t, http.StatusBadGateway, resp.StatusCode) | |||
| // An valid authenticated attempt to access a workspace app | |||
There was a problem hiding this comment.
Grammar: "An valid authenticated attempt" should be "A valid authenticated attempt".
| // An valid authenticated attempt to access a workspace app | |
| // A valid authenticated attempt to access a workspace app |
Summary
The
assertWorkspaceLastUsedAtUpdatedandassertWorkspaceLastUsedAtNotUpdatedtest helpers previously accepted acontext.Context, which callers shared with preceding HTTP requests. InProxyErrortests the request targets a fake unreachable app (http://127.1.0.1:396), and the reverse-proxy connection timeout can consume most of the context budget — especially on Windows — leaving too little time for thetestutil.Eventuallypolling loop and causing flakes.Changes
Replace the
context.Contextparameter with atime.Durationso each assertion creates its own fresh context internally. This:All 34 active call sites updated to pass
testutil.WaitLong.Fixes coder/internal#1385