Skip to content

Fix flaky http-reverse-proxy test by using OS-assigned ports#7200

Merged
gonzaloriestra merged 1 commit intomainfrom
gonzalo/fix-flaky-http-reverse-proxy-test
Apr 7, 2026
Merged

Fix flaky http-reverse-proxy test by using OS-assigned ports#7200
gonzaloriestra merged 1 commit intomainfrom
gonzalo/fix-flaky-http-reverse-proxy-test

Conversation

@gonzaloriestra
Copy link
Copy Markdown
Contributor

@gonzaloriestra gonzaloriestra commented Apr 6, 2026

WHY are these changes introduced?

There's a flaky test:

FAIL   @shopify/app  src/cli/utilities/app/http-reverse-proxy.test.ts > http-reverse-proxy for http > routes requests to the default target when no matching path is found
Error: Test timed out in 5000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".
 ❯ src/cli/utilities/app/http-reverse-proxy.test.ts:28:3
     26|   })
     27| 
     28|   test('routes requests to the default target when no matching path is…
       |   ^
     29|     const response = await fetch(`${protocol}://localhost:${ports.prox…
     30|     await expect(response.text()).resolves.toBe('Response from target

Example: https://github.com/Shopify/cli/actions/runs/24026356767/job/70065586128

WHAT is this pull request doing?

Replace getAvailableTCPPort() with port 0 in listen() calls to eliminate the TOCTOU race condition where a port could be claimed by another process before the server binds to it.

When listen() fails silently (unhandled error), the promise never resolves and the test hangs until the 5000ms timeout fires.

Also simplifies the test fixtures by merging the ports and servers fixtures into a single setup fixture, since ports are now derived from the actual server addresses.

How to test your changes?

CI

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Replace getAvailableTCPPort() with port 0 in listen() calls to eliminate
the TOCTOU race condition where a port returned by getAvailableTCPPort()
could be claimed by another process before the server binds to it.

When listen() fails silently (unhandled error), the promise never resolves
and the test hangs until the 5000ms timeout fires.

Also simplifies the test fixtures by merging the ports and servers fixtures
into a single setup fixture, since ports are now derived from the actual
server addresses.
@gonzaloriestra gonzaloriestra marked this pull request as ready for review April 6, 2026 12:36
@gonzaloriestra gonzaloriestra requested a review from a team as a code owner April 6, 2026 12:36
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Apr 7, 2026
Merged via the queue into main with commit 1d478b7 Apr 7, 2026
26 of 27 checks passed
@gonzaloriestra gonzaloriestra deleted the gonzalo/fix-flaky-http-reverse-proxy-test branch April 7, 2026 09:25
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