Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(fixture): improve finding ports to reduce flaky #55151

Merged
merged 2 commits into from
Sep 11, 2023
Merged

test(fixture): improve finding ports to reduce flaky #55151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ae594f
test(fixture): improve finding ports to reduce flaky
kwonoj Sep 8, 2023
2fef0c0
Merge branch 'canary' into better-get-port
kodiakhq[bot] Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@
"form-data": "4.0.0",
"fs-extra": "9.0.0",
"get-port": "5.1.1",
"get-port-please": "3.1.1",
"glob": "7.1.6",
"gzip-size": "5.1.1",
"html-validator": "5.1.18",
Expand Down
7 changes: 7 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 13 additions & 1 deletion test/lib/next-test-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import path from 'path'
import spawn from 'cross-spawn'
import { writeFile } from 'fs-extra'
import getPort from 'get-port'
import { getRandomPort } from 'get-port-please'
import fetch from 'node-fetch'
import qs from 'querystring'
import treeKill from 'tree-kill'
Expand Down Expand Up @@ -171,7 +172,18 @@ export function renderViaHTTP(
}

export function findPort() {
return getPort()
Copy link
Member

Choose a reason for hiding this comment

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

return import('get-port').then(mod => mod.default())

This should work with the latest get-port version

Copy link
Contributor Author

@kwonoj kwonoj Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Out of the box I mean, using our next.js setup. How did you make it work with existing test setup? You can see our test setup doesn't like native esm like
https://github.com/vercel/next.js/actions/runs/6126906741/job/16631841436?pr=55153#step:29:1292

Course could dig further and trying to bend / adjust our setup, but honestly don't feel worth investment unless we want to switch full native esm module support. Probably need to update jest config, and other module resolution including typescript to make it work fully.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm guessing jest needs to be configured https://jestjs.io/docs/ecmascript-modules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, done it once in other codebases. overall, jest & others + native esm wasn't very pleasant experiences yet, and get-port is native esm only (doesn't provide cjs compat at all) unfortunately.

// [NOTE] What are we doing here?
// There are some flaky tests failures caused by `No available ports found` from 'get-port'.
// This may be related / fixed by upstream https://github.com/sindresorhus/get-port/pull/56,
// however it happened after get-port switched to pure esm which is not easy to adapt by bump.
// get-port-please seems to offer the feature parity so we'll try to use it, and leave get-port as fallback
// for a while until we are certain to switch to get-port-please entirely.
try {
return getRandomPort()
} catch (e) {
require('console').warn('get-port-please failed, falling back to get-port')
return getPort()
Copy link
Contributor

@SukkaW SukkaW Sep 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see any reason why we need to fallback to get-port. If get-port-please does fail, IMHO we would assume there is no available port and the test should fail.

Furthermore, IMHO we should drop get-port in Next.js completely (our setup just don't work with pure ESM package, so why should we use a pure ESM package when a CJS/ESM dual package, that does the almost exactly the same thing, is available?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fallback is not meant to stay permanent, we will observe stability and eventually get rid of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kwonoj That makes sense. IMHO you should add a TODO comment here if it is not meant to be permanent.

}
}

export interface NextOptions {
Expand Down
Loading