-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
chore(deps): Updating electron to v18 + node v16.13.2 #21418
Conversation
Thanks for taking the time to open a PR!
|
@@ -1247,7 +1257,7 @@ jobs: | |||
|
|||
runner-integration-tests-firefox: | |||
<<: *defaults | |||
resource_class: medium | |||
resource_class: medium+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resources were bumped for the Firefox tests due to increased memory consumption. #21319 was logged to investigate further.
@@ -896,7 +904,9 @@ commands: | |||
yarn binary-build --platform $PLATFORM --version $(node ./scripts/get-next-version.js) | |||
- run: | |||
name: Zip the binary | |||
command: yarn binary-zip --platform $PLATFORM | |||
command: | | |||
apt-get update && apt-get install -y zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zip
was removed from our base image generator in a recent slimming down effort. We can add it back/generate a new image if there's a concern with pulling it in here.
cy.spy(cy, 'ensureElementIsNotAnimating') | ||
|
||
cy.get('button:first').trigger('tap', { animationDistanceThreshold: 1000 }).then(() => { | ||
cy.get('button:first').trigger('tap', { animationDistanceThreshold: 1000 }).then(($btn) => { | ||
const { fromElWindow } = Cypress.dom.getElementCoordinatesByPosition($btn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests related to animationDistanceThreshold
across trigger/type command tests were failing in a similar way in Firefox.
Firefox reports positions/scroll offsets with something like 13 digits of precision. We do a lot rounding in our coordinate logic that could be impacted by this. These tests also seem to be influenced by how the AUT is being scaled; I can get them to pass/fail in develop by running the tests with the AUT scaled at different sizes.
I updated these tests to grab the reference element window coordinates after the element has been scrolled into position, which makes us less susceptible to variances within Firefox's reported scroll position. This allows them to pass consistently without need to add 1px buffers to our assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can use a similar technique to remove this 1px hack for Firefox in 10.0-release? https://github.com/cypress-io/cypress/blob/10.0-release/packages/driver/cypress/e2e/commands/actions/type.cy.js#L360-L377
cy.reload() | ||
cy.get(':checkbox[name="colors"][value="blue"]').should('not.be.checked') | ||
|
||
cy.window().should('not.have.prop', 'cy_testCustomValue', true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firefox now caches input values between reloads, causing this test to fail. Updated to use a window prop that we can be certain will be reset between reloads.
@@ -56,7 +56,8 @@ | |||
"allowSyntheticDefaultImports": true, | |||
"esModuleInterop": true, | |||
"noErrorTruncation": true, | |||
"experimentalDecorators": true | |||
"experimentalDecorators": true, | |||
"skipLibCheck": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web-config
was the only package that didn't have skipLibCheck enabled. I attempted to update our node types to v16 but it seems to cause more problems than it fixed; #21329 was logged to dedicate more time this effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web-config
is used in several packages btw. What motivated this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping to electron 18 caused yarn type-check
to fail for this package due to conflicts in some deep transient dependencies. I assume this was due to electron's bumping to @types/node@16: https://github.com/cypress-io/cypress/pull/21418/files#diff-51e4f558fae534656963876761c95b83b6ef5da5103c4adef6768219ed76c2deR8720-R8723. Looks like we might have been inadvertently(?) resolving against v15 before?
My type-config-fu is admittedly weak. It seems like we can resolve the failures if I add an explicit @types/node@14 dependency to this package. Would that be preferred (in the short term, I know going to 16 across the board would be ideal)? My understanding was that this flag forced a check on types we are actually referencing rather than on all dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this seems fine after reading the config doc: https://www.typescriptlang.org/tsconfig#skipLibCheck I don't think you need to change it. The doc says we can use Yarn resolutions
maybe to pin it to 14 everywhere, but again, I don't think it's really an issue.
I wonder if this will solve #15932 🤔 |
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
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 |
@lmiller1990 I was still able to reproduce #15932 😞 : |
done() | ||
}) | ||
|
||
const variable = () => {} | ||
|
||
cy.origin('http://idp.com:3500', { args: variable }, (variable) => { | ||
cy.origin('http://foobar.com:3500', { args: variable }, (variable) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This spec would hang across all browsers when visiting idp.com; switching to foobar.com like all other specs in this file caused the test to pass (well, fail in the way we expect it too). This is odd, but not sure if it's indicative of a larger problem or the same problem recorded in #21300.
Note: we're currently pinned to electron 18.0.4 because 18.1.0 introduced a breaking change that has since been fixed (within the last day): electron/electron#34150 We should be able to consume the 18.2.2 release when it goes out. |
Any news when this dependency bump will happen, I'm just asking for some security related matters (the old dependencies have a few minor issues, nothing really critical) Maybe soon, in a few weeks or it is still being at an early work of being adopted in the main branch ? Needs a few months of work still ? |
@AkechiShiro I'd anticipate these dependency bumps to be included in our next 9.x release in two weeks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see us finally updating electron. I left one comment about a potential update we can make in 10.0-release, that will be landing soon.
cy.spy(cy, 'ensureElementIsNotAnimating') | ||
|
||
cy.get('button:first').trigger('tap', { animationDistanceThreshold: 1000 }).then(() => { | ||
cy.get('button:first').trigger('tap', { animationDistanceThreshold: 1000 }).then(($btn) => { | ||
const { fromElWindow } = Cypress.dom.getElementCoordinatesByPosition($btn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can use a similar technique to remove this 1px hack for Firefox in 10.0-release? https://github.com/cypress-io/cypress/blob/10.0-release/packages/driver/cypress/e2e/commands/actions/type.cy.js#L360-L377
packages/driver/cypress/integration/e2e/origin/config_env.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work @tbiethman, I know this was a bear of an upgrade.
Will this be a separate PR to close #21236?
import * as path from 'path' | ||
import * as plist from 'plist' | ||
import * as semver from 'semver' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this from the package.json
now I think.
// we can detect same browser under different aliases | ||
// tell them apart by the name and the version property | ||
if (!goalBrowsers) { | ||
goalBrowsers = browsers | ||
} | ||
|
||
// BigSur (darwin 20.x) and Electron 12+ cause huge performance issues when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🎉 🎉 🎉 🎉 🎉
@@ -56,7 +56,8 @@ | |||
"allowSyntheticDefaultImports": true, | |||
"esModuleInterop": true, | |||
"noErrorTruncation": true, | |||
"experimentalDecorators": true | |||
"experimentalDecorators": true, | |||
"skipLibCheck": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web-config
is used in several packages btw. What motivated this change?
Recording breaking four ✅ - let's ship it 😎 , unless some action around #21418 (comment) is needed? |
@lmiller1990 That comment has been addressed, seems like it's just not showing the full convo in Zach's review summary. We're good to go. |
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
electron
from15.5.1
to18.0.4
.16.5.0
to16.13.2
.Additional details
The Upgrading Electron guide was followed to produce this PR.
I included inline comments where additional clarification was needed. A few changes of note:
origin
related tests in newer versions of Firefox. An issue was logged, and the affected tests have been tweaked to pass in the meantime without being outright skipped.How has the user experience changed?
There shouldn't be any functional difference in the user experience beyond any changes arising from the bundled Chromium update. If other differences are seen, please raise them.
PR Tasks
cypress-documentation
?type definitions
?cypress.schema.json
?