fix(site/e2e): close mock external-auth servers in teardown#26575
Draft
jakehwll wants to merge 2 commits into
Draft
fix(site/e2e): close mock external-auth servers in teardown#26575jakehwll wants to merge 2 commits into
jakehwll wants to merge 2 commits into
Conversation
createServer now returns a close handle so callers can release the listener after the test. The web flow closes in afterAll; the device flow uses try/finally. This prevents leaked listeners from triggering EADDRINUSE on the next run, the long-standing root cause of the externalAuth.spec.ts flake (coder/internal#356).
This comment has been minimized.
This comment has been minimized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The externalAuth e2e suite has been skipped since #17235 because
createServerinsite/e2e/helpers.tsstarted an express server but never gave callers a way to close it. On retries or repeated runs, the listener from the previous invocation was still bound to the hardcoded port and the nextbeforeAllfailed withEADDRINUSE, eventually timing out inwaitForPort.createServernow returns a{ app, close }pair. The web flow closes inafterAll; the device flow usestry/finally.closeAllConnections()is called beforeclose()so teardown stays bounded if keep-alive connections linger.The suite remains
test.describe.skipfor now. A follow-up PR will flip the skip off and watch CI to confirm A is sufficient before any further work.Refs https://linear.app/codercom/issue/DEVEX-413
Refs coder/internal#356
Decision log
Discussed the full options list with @jakehwll before drafting. Picked option A (minimal teardown) because:
Returning
closerather than the rawhttp.Serverencapsulates thecloseAllConnections+closechoreography so callers don't repeat it.closeAllConnectionsis optional-chained because it landed in Node 18.2; coder/coder runs newer, but the chain costs nothing.The device test uses
try/finallyrather than a sharedafterEachto keep per-test state local. The web flow'safterAllmirrors itsbeforeAll.