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

CI: Run test in Electron as well #1224

Open
Prinzhorn opened this issue Jul 14, 2024 · 10 comments
Open

CI: Run test in Electron as well #1224

Prinzhorn opened this issue Jul 14, 2024 · 10 comments

Comments

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Jul 14, 2024

Looking at this #1036 (comment) I think it would be great if we could catch these things early and also if Electron specific PRs would actually show that tests are now passing.

Is this feasible? Too slow? It should be possible to run Jest with an Electron process and also run @electron/rebuild before running the tests.

@m4heshd you seem to be knowledgeable regarding GitHub CI stuff

@mceachen
Copy link
Member

We’d want to run with both supported electron builds and a phase that pulls the latest beta that doesn’t mark the build as “failed.” That could be as simple as splitting the beta electron build into a different .yml file.

@m4heshd
Copy link
Contributor

m4heshd commented Jul 15, 2024

It would most probably slow down the tests significantly however, it's worth the effort.

I'm gonna have to look into how Mocha could handle this. The first thing that comes to my mind is to execute the Mocha process via Electron using ELECTRON_RUN_AS_NODE=1 env. Not sure if it would work as intended though. Need to run some tests. electron-mocha seems to be dedicated to doing exactly this.

Since it's not recommended to run DB tasks on the renderer process in the first place, I think it's unnecessary to test that scenario. So simply testing on the main process with ELECTRON_RUN_AS_NODE would do.

@Prinzhorn
Copy link
Contributor Author

Since it's not recommended to run DB tasks on the renderer process in the first place, I think it's unnecessary to test that scenario.

Agreed, definitely only run tests in the main process

@TheOneTheOnlyJJ
Copy link

Given how common using better-sqlite3 with Electron is, this makes total sense and would improve the DX for many.

@neoxpert
Copy link
Contributor

While it is not recommended to run (concurrent) DB tasks in the renderer, I came across several projects, that use a hidden browser window for decoupling the persistence layer from the main process. But in the end - if everything goes right - the renderer process should behave the same as the main process when it comes down to loading native node modules. At least until today I had no issues testing the better-sqlite3 integration with electron-mocha in the main and renderer process.

@TheOneTheOnlyJJ
Copy link

TheOneTheOnlyJJ commented Jul 15, 2024

While it is not recommended to run (concurrent) DB tasks in the renderer, I came across several projects, that use a hidden browser window for decoupling the persistence layer from the main process. But in the end - if everything goes right - the renderer process should behave the same as the main process when it comes down to loading native node modules. At least until today I had no issues testing the better-sqlite3 integration with electron-mocha in the main and renderer process.

Given how this issue arose because things that were presumed to work ended up not working after a new electron version due to the lack of explicit tests, I believe not covering the greatest area possible with these new tests is an invitation for the same problems to appear further down the line.
While not making sense to have separate tests as of now for both the main and renderer processes, a future electron version may introduce breaking changes that will require such tests to be written.

@m4heshd
Copy link
Contributor

m4heshd commented Jul 15, 2024

renderer process should behave the same as the main process when it comes down to loading native node modules.

It unfortunately doesn't. There's a countless amount of issues open in Electron about the very same issue and the team's response is always the same. They DO NOT recommend it. That's why they even unplugged the remote module and marked it as risky. If somebody utilizes Node's native features on the renderer, it's on them to maintain that unreliable mess. I've experienced this firsthand myself. This is also why Tauri went on a much safer path when it comes to that.

@Prinzhorn
Copy link
Contributor Author

I came across several projects, that use a hidden browser window for decoupling the persistence layer from the main process

I use a Worker Thread spawned from main to host the entire application logic (to not block main). There is no reason to use better-sqlite3 in renderer. Ever. Using a hidden browser window might have been the recommended approach five years ago.

@m4heshd
Copy link
Contributor

m4heshd commented Jul 15, 2024

I use a Worker Thread spawned from main to host the entire application logic (to not block main). There is no reason to use better-sqlite3 in renderer. Ever. Using a hidden browser window might have been the recommended approach five years ago.

This and only this. I do not recommend any other practice when it comes to Electron. I learned an expensive lesson many years ago and this is the exact method I adapted to use. I even updated the threads documentation when I finally got it to be stable for production.

Final conclusion, DO NOT perform important logic on the renderer unless you're super lazy. Let it simply be the UI Renderer (which it's supposed to be) and a messenger.

@mceachen
Copy link
Member

I came across several projects, that use a hidden browser window for decoupling the persistence layer from the main process

I use a Worker Thread spawned from main to host the entire application logic (to not block main). There is no reason to use better-sqlite3 in renderer. Ever. Using a hidden browser window might have been the recommended approach five years ago.

We should pin this to this project (maybe add it to the README?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants