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

chore(deps): update to electron 10.1.5 #3599

Merged
merged 34 commits into from
Nov 5, 2020
Merged

chore(deps): update to electron 10.1.5 #3599

merged 34 commits into from
Nov 5, 2020

Conversation

karanbirsingh
Copy link
Contributor

@karanbirsingh karanbirsingh commented Nov 2, 2020

Description of changes

This PR updates electron to 10.1.5. Breaking changes here. I don't see any UI regressions so far; most of the complexity is around end-to-end tests (although several recent PRs have improved flakiness).

background

The Electron team will drop support for electron 8 when electron 11 ships in a few weeks (see info here and here). We should expect a new major release every 3 months in line with every other major Chromium update.

As usual each Electron update requires an associated Spectron update. The typings changed since webdriverio was updated from 4.x to 6.x - changes are mostly scoped to SpectronAsyncClient for now to get things working without touching all the e2e tests themselves.

enableRemoteModule

mean we need to set enableRemoteModule to true in our main and codec-test electron apps. We were hoping to explicitly disable it with the guidance that came with electron 9/10, but this comment explains spectron's dependency on the module. It might be worth keeping in mind when trying electron-playwright to see if it helps with flakiness issues similar to Chromedriver failed to start.

waitForTimeout
I was running into this issue where the implicit wait timeout added by spectron exceeds the timeout we pass to waitForFluentLeftNavToDisappear. Spectron adds an implicit timeout of 5000 for the session requests. The chain waitForExist -> isExisting/$$ -> findElement uses the implicit timeout, leading to wrapping-timeout failures. I first tried reducing the implicit timeout to 1000ms; but spectron reuses the value for the script execution timeout. I've doubled the wrapping timeout on waitForSelectorToDisappear.

ECONNRESET
With this update we started seeing a new failure with the callstack:

Error: Failed to create session.
read ECONNRESET
          at Object.startWebDriverSession (D:\a\1\s\node_modules\webdriver\build\utils.js:34:15)
          at processTicksAndRejections (internal/process/task_queues.js:93:5)
          at Function.newSession (D:\a\1\s\node_modules\webdriver\build\index.js:30:45)
          at Object.<anonymous>.exports.remote (D:\a\1\s\node_modules\spectron\node_modules\webdriverio\build\index.js:60:22)

This was similar to the spectron issue here, which we resolved earlier by adding enableRemoteModule to the codec-test app. This issue is more like the got issue here. Adding an agent-base resolution as suggested did not remove the error for us. I added this signature to the retry logic - an example is here .

validation

suggestions encouraged to debug the above two failures

Pull request checklist

  • Addresses an existing issue: #0000
  • Ran yarn fastpass
  • Added/updated relevant unit test(s) (and ran yarn test)
  • Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
  • (UI changes only) Added screenshots/GIFs to description above
  • (UI changes only) Verified usability with NVDA/JAWS

@karanbirsingh karanbirsingh requested a review from a team as a code owner November 2, 2020 22:56
@karanbirsingh karanbirsingh marked this pull request as draft November 3, 2020 00:48
@karanbirsingh karanbirsingh marked this pull request as ready for review November 4, 2020 20:24
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

I think the reliability runs look acceptable; 2/60 failures is on par with existing behavior, and the one with the exciting exit code doesn't feel like a spectron failure (it suggests that node AV'd somehow)

@karanbirsingh karanbirsingh merged commit 2e589d3 into master Nov 5, 2020
@karanbirsingh karanbirsingh deleted the electron10 branch November 5, 2020 00:20
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