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.

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

fix: ipv4 detection on node 18 #34203

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Jan 3, 2025

Closes #34187

@Skn0tt Skn0tt requested review from dgozman and Copilot January 3, 2025 13:45
@Skn0tt Skn0tt self-assigned this Jan 3, 2025

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

packages/playwright-core/src/utils/httpServer.ts:131

  • [nitpick] The variable name isIPV4 should be renamed to isIPv4 for consistency and clarity.
const isIPV4 = address.family === 'IPv4' || (address.family as any) === 4;
@@ -128,7 +128,8 @@ export class HttpServer {
this._urlPrefixHumanReadable = address;
} else {
this._port = address.port;
const resolvedHost = address.family === 'IPv4' ? address.address : `[${address.address}]`;
const isIPV4 = address.family === 'IPv4' || (address.family as any) === 4; // Node 18.0.0 returns number, >18.4.0 returns string
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we support 18.0.0. We support only the latest 18.X, 20.X, 22.X release.

I recommend the user instead update his Node.js since he misses important security updates

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know! I can't find us stating that anywhere in the docs, let's change that.

Copy link
Contributor

github-actions bot commented Jan 3, 2025

Test results for "tests 1"

3 failed
❌ [installation tests] › playwright-electron-should-work.spec.ts:21:5 › electron should work @package-installations-ubuntu-latest
❌ [installation tests] › playwright-electron-should-work.spec.ts:31:5 › electron should work with special characters in path @package-installations-ubuntu-latest
❌ [installation tests] › playwright-electron-should-work.spec.ts:44:5 › should work when wrapped inside @playwright/test and trace is enabled @package-installations-ubuntu-latest

8 flaky ⚠️ [firefox-page] › page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-setup.spec.ts:98:5 › should show errors in config @macos-latest-node18-1
⚠️ [playwright-test] › ui-mode-test-update.spec.ts:83:5 › should pick new / deleted tests @ubuntu-latest-node20-1
⚠️ [webkit-library] › library/browsercontext-reuse.spec.ts:50:1 › should reset serviceworker @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/proxy.spec.ts:93:11 › should proxy local network requests › by default › loopback address @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/trace-viewer.spec.ts:174:1 › should properly synchronize local and remote time @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/tracing.spec.ts:614:5 › should hide internal stack frames in expect @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/video.spec.ts:381:5 › screencast › should capture navigation @webkit-ubuntu-22.04-node18

37506 passed, 653 skipped
✔️✔️✔️

Merge workflow run.

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.

[Bug]: Cannot run Playwright UI after npm init playwright@latest on WSL2
2 participants