Upgrade Electron to v14. Replace Spectron with Playwright. #1985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mason-fish
force-pushed
the
electron-bump
branch
2 times, most recently
from
e0abb50 to
41167c5
Compare
added 20 commits
December 9, 2021 07:56
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
mason-fish
force-pushed
the
electron-bump
branch
from
41167c5 to
2503f8d
Compare
Signed-off-by: Mason Fish <mason@brimsecurity.com>
mason-fish
force-pushed
the
electron-bump
branch
from
2503f8d to
19f1bf7
Compare
added 2 commits
December 9, 2021 12:05
Signed-off-by: Mason Fish <mason@brimsecurity.com>
Signed-off-by: Mason Fish <mason@brimsecurity.com>
mason-fish
changed the title
jameskerr
approved these changes
Dec 9, 2021
| xvfb-run -d -s "-screen 0 1280x1024x24" npm run itest -- --ci --forceExit | ||
|
|
||
| integration_test: | ||
| playwright_integration_test: |
There was a problem hiding this comment.
No, but I can play around with adding it back in separately
| import {app as electronApp, remote} from "electron" | ||
| const app = electronApp || (remote && remote.app) | ||
| import {app as electronApp} from "electron" | ||
| const app = electronApp |
There was a problem hiding this comment.
This can run from the renderer process, so we'll need a way to tell the renderer what the environment looks like. We can make an ipc call and set a global.
|
|
||
| const app = electronApp || (remote && remote.app) | ||
| const app = electronApp |
There was a problem hiding this comment.
This also may run from the renderer process.
| @@ -1,5 +1,6 @@ | |||
| import path from "path" | |||
| import fs from "fs-extra" | |||
| import {ReadableStream} from "web-streams-polyfill/ponyfill" | |||
There was a problem hiding this comment.
I added a web-file-polyfill for this in my own tests. You'll see it in the upcoming PR.
|
|
||
| export const dialog = { | ||
| showMessageBox: jest.fn() | ||
| } |
Merged
This was referenced Aug 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #1934
Lot's of changes here:
Replacing our integration test framework from Spectron to Playwright: Includes 3 of our original spectron test suites (queries, ingest, and zed-events). Unfortunately there is still a lot of inconsistency with running this on Windows (only in CI, I have not seen a single failure when running locally on my PC) but since we are so desperate to upgrade electron (which requires ditching Spectron) then we can test Windows manually in our pre-release cadence for now and tackle the CI issues more later.
Upgrading electron from 11 to 14: There are a few small changes involved here but the most noteworthy is that the
remotemodule has been extracted out of theelectronpackage and is now it's own npm scoped module at@electron/remote. I followed their provided migration instructions for this transition and the biggest blocker I hit was changing the way we mock that in our unit tests.There are very few changes to the app itself other than the upgrade-related api changes. I did end up synchronizing/awaiting the
window.loadFile()uses and re-order the launch of the hidden window. I did this so that the app would open more deterministically which then allowed the new integration tests to perform more consistently. I also re-worked some of the file input logic in the ingest flow so that we use the providedFileobject instead of the filepath. This was also for testing purposes since Playwright does not currently set the filepath onFileobjects the way that Electron does (and for this reason, in tests, we do NOT run or test thebrimcap indexsub-operation facilitated by the brimcap plugin). Finally, we also have upgraded to node 14! I found no issues with that aspect of the upgrade.