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

deps: update to electron 10.1.5 #3547

Closed
wants to merge 15 commits into from
Closed

deps: update to electron 10.1.5 #3547

wants to merge 15 commits into from

Conversation

karanbirsingh
Copy link
Contributor

@karanbirsingh karanbirsingh commented Oct 28, 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 typings & 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. 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.

impact on unified-e2e-reliability across 3 trials (42/45 passing test runs)

(below failures were on a conflicting merge with master, rerunning and will update)

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:97:5)
  • I saw one failure where the app title hadn't updated to Automated Checks yet. I'm not sure whether this might be a test-timing issue since the data relies on the left nav store.

impact on PR build (6 test runs): two PR builds ran without any e2e test failures

impact on build-unsigned-release-packages: several consecutive runs failing, all with the same signature in the same test

RequestError: read ECONNRESET
    at ClientRequest.<anonymous> (D:\a\1\s\node_modules\webdriver\node_modules\got\dist\source\core\index.js:953:111)
    at Object.onceWrapper (events.js:417:26)
    at ClientRequest.emit (events.js:322:22)
    at ClientRequest.EventEmitter.emit (domain.js:482:12)
    at ClientRequest.origin.emit (D:\a\1\s\node_modules\webdriver\node_modules\@szmarczak\http-timer\dist\source\index.js:39:20)
    at Socket.socketErrorListener (_http_client.js:426:9)
    at Socket.emit (events.js:310:20)
    at Socket.EventEmitter.emit (domain.js:482:12)
    at emitErrorNT (internal/streams/destroy.js:92:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:60:3)
    at TCP.onStreamRead (internal/stream_base_commons.js:205:27)

I'm unable to repro the unsigned build test failure locally with the electron mirror downloaded. Even if the other builds weren't flaky, this failure is a blocker because we're unable to generate installers.

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 changed the title deps: update to electron 10 deps: update to electron 10.1.5 Oct 28, 2020
@@ -37,7 +37,7 @@ const electronAutoUpdateCheck = new AutoUpdaterClient(autoUpdater);
const createWindow = () => {
mainWindow = new BrowserWindow({
show: false,
webPreferences: { nodeIntegration: true },
webPreferences: { nodeIntegration: true, enableRemoteModule: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to leave a comment here explaining the same thing the PR description does about why we do this

@karanbirsingh
Copy link
Contributor Author

Closing to iterate on the unsigned-build tests

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