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: ct testing support for node 17+ #21430

Merged
merged 9 commits into from
May 13, 2022
Merged

fix: ct testing support for node 17+ #21430

merged 9 commits into from
May 13, 2022

Conversation

ZachJW34
Copy link
Contributor

User facing changelog

Fix ct apps using webpack-dev-server v4 not working for Node >= 17

Additional details

Node 17+ changed how they resolve DNS names. Changing localhost to 127.0.0.1 is the fix for this issue

nodejs/node#40537
Automattic/mongoose#10917
chimurai/http-proxy-middleware#705

To test this (using Node 17+), I had to make a few changes to the wherever we were building Webpack. Since we are using an older version of Webpack (< 5.6.1), we have to set NODE_OPTIONS=--openssl-legacy-provider for all Webpack build scripts. Without these changes, yarn and yarn dev will not work as it will fail with digital envelope routines::unsupported

How has the user experience changed?

CT works for Node 17+

Screen.Recording.2022-05-10.at.5.58.32.PM.mov

PR Tasks

  • [na] Have tests been added/updated?
  • [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?
  • [na] Have new configuration options been added to the cypress.schema.json?

@ZachJW34 ZachJW34 requested review from a team as code owners May 10, 2022 23:00
@ZachJW34 ZachJW34 requested review from jennifer-shehane and removed request for a team May 10, 2022 23:00
@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 10, 2022

Thanks for taking the time to open a PR!

@ZachJW34 ZachJW34 requested review from emilyrohrbough, a team and lmiller1990 May 10, 2022 23:00
Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Some general comments 👍

scripts/gulp/tasks/gulpWebpack.ts Outdated Show resolved Hide resolved
scripts/use-legacy-openssl.js Outdated Show resolved Hide resolved
scripts/use-legacy-openssl.js Outdated Show resolved Hide resolved
@cypress
Copy link

cypress bot commented May 10, 2022



Test summary

37671 3 464 0Flakiness 7


Run details

Project cypress
Status Failed
Commit de551f7
Started May 13, 2022 2:38 PM
Ended May 13, 2022 3:00 PM
Duration 22:13 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Failures

Run group: 5x-driver-electron (Linux, Electron )
cypress/proxy-logging.cy.ts Failed
1 Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged first
commands/files.cy.js Failed
1 ... > has implicit existence assertion, retries and throws a specific error when file does not exist for null encoding
Run group: 5x-driver-electron-experimentalSessionAndOrigin (Linux, Electron )
cypress/proxy-logging.cy.ts Failed
1 Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged first

Flakiness

commands/net_stubbing.cy.ts Flakiness
1 network stubbing > waiting and aliasing > can timeout waiting on a single response using "alias"
react.cy.ts Flakiness
1 Working with webpack5_wds3-react > MissingReactInSpec: should fail, rerun, succeed
e2e/origin/commands/screenshot.cy.ts Flakiness
1 cy.origin screenshot > #consoleProps > .screenshot()
commands/window.cy.js Flakiness
1 ... > eventually passes the assertion
e2e/origin/commands/waiting.cy.ts Flakiness
1 cy.origin waiting > #consoleProps > .wait()
This comment includes only the first 5 flaky tests. See all 7 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@jennifer-shehane jennifer-shehane removed their request for review May 11, 2022 14:03
@jennifer-shehane jennifer-shehane removed the request for review from a team May 11, 2022 14:47
@BlueWinds BlueWinds requested review from flotwig and removed request for emilyrohrbough May 11, 2022 15:41
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

@ZachJW34 We already have some tests that validate running --e2e in Node versions (including 17.3.0), I think it would be a good idea to add a test for CT as well: https://github.com/cypress-io/cypress/blob/b0c8db34348be31f32795f3f67c3408403d59ace/system-tests/test-binary/node_versions_spec.ts

Otherwise I can see this issue cropping back up. Let me know if I can assist with this, the Docker test experience on Mac is not great, you may want to create a Linux VM.

@@ -68,7 +68,7 @@ function webpackDevServer4 (
const { devServerConfig: { cypressConfig: { devServerPublicPathRoute } } } = config
const WebpackDevServer = config.sourceWebpackModulesResult.webpackDevServer.module
const webpackDevServerConfig = {
host: 'localhost',
host: '127.0.0.1',
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we should never use 'localhost' in the App due to issues like this. We've even had users with an incorrect localhost entry in /etc/hosts, which causes mind boggling issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was absolutely baffled as to what was going on until I stumbled upon the issues I listed above.

@@ -4,7 +4,7 @@
"private": true,
"main": "index.js",
"scripts": {
"build": "gulp build",
"build": "cross-env NODE_OPTIONS=\"$(node ../../scripts/use-legacy-openssl.js)\" gulp build",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work in Windows? IDK how the command substitution is handled cross-platform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I'll give it a test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't work on Windows, changed to running webpack in a child process so the env variables could be manipulated more easily

@ZachJW34
Copy link
Contributor Author

@flotwig I'll see how far I can get adding the tests and if I get stuck I'll reach out!

@ZachJW34
Copy link
Contributor Author

@flotwig Added binary system tests, tried it out on WSL which was painful but I eventually got it working

@ZachJW34 ZachJW34 requested a review from flotwig May 12, 2022 00:50
@@ -9,6 +9,15 @@ function smokeTestDockerImage (dockerImage: string) {
specDir: 'tests',
project: 'todos',
})

systemTests.it(`can run in ${dockerImage}`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

clean up the hierarchy of tests here? right now it's:

e2e binary node versions > can run in ${dockerImage}
e2e binary node versions > can run in ${dockerImage}

but something like this would be better:

binary node versions > ct > can run in ${dockerImage}
binary node versions > e2e > can run in ${dockerImage}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, addressed in 0654882

@ZachJW34 ZachJW34 requested a review from flotwig May 13, 2022 14:25
@ZachJW34 ZachJW34 merged commit 0f786ab into 10.0-release May 13, 2022
@ZachJW34 ZachJW34 deleted the zachw/ct-node-17 branch May 13, 2022 16:01
karthikbtf added a commit to terraware/terraware-web that referenced this pull request Feb 20, 2023
See cypress-io/github-action#534 (comment) (thanks @charlie-tf )

The default webpack configuration's host value was changed from localhost to 127.0.0.1 to support the Node 17+ changes with how DNS names are resolved. Addressed in cypress-io/cypress#21430.
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.

3 participants