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

Electron13 #2609

Merged
merged 5 commits into from
Jul 4, 2021
Merged

Electron13 #2609

merged 5 commits into from
Jul 4, 2021

Conversation

khassel
Copy link
Collaborator

@khassel khassel commented Jul 4, 2021

Managed to get the e2e tests running with electron v13 and spectron v15.

Tested new electron version on pi3 and ubuntu desktop.

Related to #2408.

@MichMich
Copy link
Collaborator

MichMich commented Jul 4, 2021

Great work!

I notice that almost all changes are the same electron options object in all different tests. Maybe it's worth it to extract that to a separate file so it can be reused?

@codecov-commenter
Copy link

Codecov Report

Merging #2609 (d617d4a) into develop (aad8141) will decrease coverage by 0.17%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2609      +/-   ##
===========================================
- Coverage    70.64%   70.47%   -0.18%     
===========================================
  Files           16       16              
  Lines          821      823       +2     
===========================================
  Hits           580      580              
- Misses         241      243       +2     
Impacted Files Coverage Δ
js/electron.js 72.22% <0.00%> (-2.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aad8141...d617d4a. Read the comment docs.

@khassel
Copy link
Collaborator Author

khassel commented Jul 4, 2021

Maybe it's worth it to extract that to a separate file so it can be reused?

You mean something like

	defaultElectronOptions: {
		webPreferences: {
			nodeIntegration: true,
			enableRemoteModule: true,
			contextIsolation: false
		}

in a seperate file and then

	electronOptions: defaultElectronOptions

in every test config?

@MichMich
Copy link
Collaborator

MichMich commented Jul 4, 2021

Jean, or even better. A default test config that can be extended.

let config = testConfig({foo: bar})

The testConfig method will return the default config (including the electronOptions) and it will extend/overwrite with whatever you add as an argument.

Of course this can be a separate PR. But it might save some work in the future and makes PR's like these much cleaner.

@khassel
Copy link
Collaborator Author

khassel commented Jul 4, 2021

o.k., will try this in a new PR.

Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

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

LGTM!

@MichMich MichMich merged commit 8b484ee into MagicMirrorOrg:develop Jul 4, 2021
@khassel khassel deleted the electron13 branch July 4, 2021 19:41
@khassel khassel mentioned this pull request Jul 5, 2021
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.

4 participants