Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

fix(site): wait until port is available in e2e - #15537

#15537
Merged
dannykopping merged 5 commits into
maincoder/coder:mainfrom
fix-e2e-212-wait-for-servercoder/coder:fix-e2e-212-wait-for-serverCopy head branch name to clipboard
Nov 18, 2024
Merged

fix(site): wait until port is available in e2e#15537
dannykopping merged 5 commits into
maincoder/coder:mainfrom
fix-e2e-212-wait-for-servercoder/coder:fix-e2e-212-wait-for-serverCopy head branch name to clipboard

Conversation

@mtojek

@mtojek mtojek commented Nov 15, 2024

Copy link
Copy Markdown
Member

Related: coder/internal#212

This PR modifies the logic responsible for creating a server in E2E tests to check if the port is free. Alternatively, we could refactor the framework to dynamically create server instances, but this solution might be a cheaper quick win.

Note:

I'll leave it as is now, it might be worth asking somebody with a frontend skillset to double-check this contribution.

@mtojek mtojek self-assigned this Nov 15, 2024
@mtojek mtojek changed the title fix(site): e2e: wait until port is available) fix(site): wait until port is available in e2e Nov 15, 2024
@mtojek
mtojek requested a review from dannykopping November 15, 2024 16:47
@mtojek
mtojek marked this pull request as ready for review November 15, 2024 16:47
Signed-off-by: Danny Kopping <danny@coder.com>

@dannykopping dannykopping left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the assist on this one @mtojek!

Tested this locally and it works, let's try it out in CI.

Signed-off-by: Danny Kopping <danny@coder.com>
@dannykopping
dannykopping enabled auto-merge (squash) November 18, 2024 10:20
@dannykopping
dannykopping merged commit 8ca8e01 into main Nov 18, 2024
@dannykopping
dannykopping deleted the fix-e2e-212-wait-for-server branch November 18, 2024 10:28
jakehwll added a commit that referenced this pull request Jul 1, 2026
> 🤖 This PR was written by Coder Agents on behalf of Jake Howell.

Stack:
1. #26575 `fix(site/e2e): close mock external-auth servers in teardown`
← this PR
2. #26793 `fix(site/e2e): accept 404 from external auth reset hook`
3. #26795 `fix(site/src): refresh provider state after device-flow
exchange`
4. #26798 `fix(site/e2e): reset both providers in external auth hook`
5. #26648 `chore(site/e2e): re-enable externalAuth suite`

The externalAuth e2e suite has been skipped since #17235 because
`createServer` in `site/e2e/helpers.ts` started 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 next `beforeAll` failed with `EADDRINUSE`, eventually
timing out in `waitForPort`.

`createServer` now returns a `{ app, close }` pair. The web flow closes
in `afterAll`; the device flow uses `try/finally`.
`closeAllConnections()` is called before `close()` so teardown stays
bounded if keep-alive connections linger.

The suite remains `test.describe.skip` here; #26648 flips the skip off
once the rest of the stack is in.

Refs https://linear.app/codercom/issue/DEVEX-413
Refs coder/internal#356

<details>
<summary>Decision log</summary>

Discussed the full options list with @jakehwll before drafting. Picked
option A (minimal teardown) because:

- Two prior PRs (#15537, #16528) attacked symptoms (port probing, longer
timeout) without addressing the leaked listener.
- Kayla's diagnosis on coder/internal#356 pointed at exactly this case:
nothing else in CI is grabbing the port, the listener from the previous
run is still bound.
- A is mechanical and orthogonal: it adds a real teardown without
changing port allocation, fixture wiring, or what gets mocked. If the
flake persists after A, we know to escalate to a worker-scoped fixture
or dynamically allocated ports.

Returning `close` rather than the raw `http.Server` encapsulates the
`closeAllConnections` + `close` choreography so callers don't repeat it.
`closeAllConnections` is optional-chained because it landed in Node
18.2; coder/coder runs newer, but the chain costs nothing.

The device test uses `try/finally` rather than a shared `afterEach` to
keep per-test state local. The web flow's `afterAll` mirrors its
`beforeAll`.

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.