-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Basic playwright electron support #12207
Basic playwright electron support #12207
Conversation
e1d7afe
to
54798d2
Compare
@xai could you rebase your changes on latest master to make sure the ubuntu test failures are not due to the old flakiness of the test suite? |
toMatchSnapshot: { threshold: 0.15 } | ||
viewport: { width: 1920, height: 1080 }, | ||
trace: { | ||
mode: 'retain-on-failure' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're running on CI: where will the videos be available and are we sure we have enough space to store the videos for all PR's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I will just disable this.
) { } | ||
|
||
playwrightOptions(workspace?: TheiaWorkspace): object { | ||
const executablePath = this.electronAppPath + '/node_modules/.bin/electron'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work on Windows: I'm getting the message electron.launch: Failed to launch: Error: spawn ../electron/node_modules/.bin/electron ENOENT
and the electron app is not launched.
doing this in ElectronLaunchOptions.playwrightOptions(...)
works for me, but probably, we need to dispatch on the platform here:
const executablePath = path.resolve(this.electronAppPath, 'node_modules/.bin/electron.cmd');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! There were several issues with paths on Windows. Following your suggestion on using path
instead of modifying strings, it should now work on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ecxept that now nothing works on Windows anymore, due to #12405 😬
@@ -197,7 +197,8 @@ export class ElectronMainApplication { | |||
} | |||
|
|||
async start(config: FrontendApplicationConfig): Promise<void> { | |||
this.useNativeWindowFrame = this.getTitleBarStyle(config) === 'native'; | |||
const args = this.processArgv.getProcessArgvWithoutBin(process.argv); | |||
this.useNativeWindowFrame = this.getTitleBarStyle(config) === 'native' && !args.includes('--no-native-window-frame'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to make this a theia CliContribution
that we can include into apps to be tested?
@@ -305,13 +306,19 @@ export class ElectronMainApplication { | |||
|
|||
async openDefaultWindow(): Promise<BrowserWindow> { | |||
const [uri, electronWindow] = await Promise.all([this.createWindowUri(), this.createWindow()]); | |||
if (!this.useNativeWindowFrame) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this just a bug before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Linux, I ended up with two menubars, so I got rid of the native one using this workaround. Maybe I should try and figure out why that happens instead of fixing the symptom 😉
Hello:
HTML-based menus don't seem to work on MacOS: So, the test failed 😢 : mac@Titans-iMac playwright % yarn ui-tests -g "electron"
yarn run v1.22.19
$ yarn && playwright test --config=./configs/playwright.config.ts -g electron
[1/5] 🔍 Validating package.json...
[2/5] 🔍 Resolving packages...
success Already up-to-date.
$ yarn clean && yarn build
$ rimraf lib *.tsbuildinfo
$ tsc --incremental && npx playwright install chromium
Running 7 tests using 1 worker
✓ 1 …s:34:9 › Theia Electron Application › should load and show main content panel (15ms)
✘ 2 ../../src/tests/theia-electron-app.test.ts:38:9 › Theia Electron Application › open about dialog using menu (30.0s)
✘ 3 ../../src/tests/theia-electron-app.test.ts:47:9 › Theia Electron Application › toggle explorer view using menu (30.0s)
✓ 4 ../../src/tests/theia-electron-app.test.ts:55:9 › Theia Electron Application › open quick command palette (5.1s)
✓ 5 ../../src/tests/theia-electron-app.test.ts:62:9 › Theia Electron Application › open about dialog using command (5.8s)
✓ 6 ../../src/tests/theia-electron-app.test.ts:73:9 › Theia Electron Application › select all using command (11.4s)
✓ 7 …/../src/tests/theia-electron-app.test.ts:80:9 › Theia Electron Application › toggle explorer view using command (7.1s)
1) ../../src/tests/theia-electron-app.test.ts:38:9 › Theia Electron Application › open about dialog using menu
page.waitForSelector: Timeout 30000ms exceeded.
=========================== logs ===========================
waiting for locator('#theia\\:menubar .p-MenuBar-itemLabel').locator('text=Help') to be visible
============================================================
at ../../src/theia-main-menu.ts:47
45 |
46 | protected menuBarItem(label = ''): Promise<ElementHandle<SVGElement | HTMLElement> | null> {
> 47 | return this.page.waitForSelector(this.menuBarItemSelector(label));
| ^
48 | }
49 |
50 | protected menuBarItemSelector(label = ''): string {
at TheiaMenuBar.menuBarItem (/Users/mac/Work/eclipsesource_theia/examples/playwright/src/theia-main-menu.ts:47:26)
at TheiaMenuBar.openMenu (/Users/mac/Work/eclipsesource_theia/examples/playwright/src/theia-main-menu.ts:30:40)
at /Users/mac/Work/eclipsesource_theia/examples/playwright/src/tests/theia-electron-app.test.ts:40:34
2) ../../src/tests/theia-electron-app.test.ts:47:9 › Theia Electron Application › toggle explorer view using menu
page.waitForSelector: Timeout 30000ms exceeded.
=========================== logs ===========================
waiting for locator('#theia\\:menubar .p-MenuBar-itemLabel').locator('text=View') to be visible
============================================================
at ../../src/theia-main-menu.ts:47
45 |
46 | protected menuBarItem(label = ''): Promise<ElementHandle<SVGElement | HTMLElement> | null> {
> 47 | return this.page.waitForSelector(this.menuBarItemSelector(label));
| ^
48 | }
49 |
50 | protected menuBarItemSelector(label = ''): string {
at TheiaMenuBar.menuBarItem (/Users/mac/Work/eclipsesource_theia/examples/playwright/src/theia-main-menu.ts:47:26)
at TheiaMenuBar.openMenu (/Users/mac/Work/eclipsesource_theia/examples/playwright/src/theia-main-menu.ts:30:40)
at /Users/mac/Work/eclipsesource_theia/examples/playwright/src/tests/theia-electron-app.test.ts:48:34
Slow test file: ../../src/tests/theia-electron-app.test.ts (1.5m)
Consider splitting slow test files to speed up parallel execution
2 failed
../../src/tests/theia-electron-app.test.ts:38:9 › Theia Electron Application › open about dialog using menu
../../src/tests/theia-electron-app.test.ts:47:9 › Theia Electron Application › toggle explorer view using menu
5 passed (1.9m)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. |
Does "playwright cannot access native UI elements" mean native file/folder selection dialogs such as 'Add folder to workspace' is not available? |
Yes, correct. This is the second thing that I am currently working on. The plan is to use the plain FileDialogService instead of ElectronFileDialogservice for the Playwright tests. |
Thank you very much for trying this on MacOS! So using the command-line switch, you end up with no menus at all? Not good. On Linux, I suddenly had two menubars, so I set the visibility to false in https://github.com/eclipsesource/theia/blob/54798d2dc3a81b8a985c318b63cb678fd0309bfa/packages/core/src/electron-main/electron-main-application.ts#L310 as well as in line 320. What happens on MacOS if that is removed again? Also, to have the complete information here, can you please look at whether a "window.titleBarStyle" preference is set in your (Edit: typo) |
253ef88
to
1dca53e
Compare
a70cf99
to
032e71e
Compare
Now the DefaultFileDialogService is used when the |
Hello
This pipeline requires Xvfb and NodeJS extension. |
291b749
to
f59e791
Compare
Using a CLI switch to disable native elements was a bad idea by me, so I now replaced this with an environment variable THEIA_ELECTRON_DISABLE_NATIVE_ELEMENTS. As playwright electron is the only use case we are aware of so far, it is not justified to clutter the command line arguments with things not relevant to users. |
f59e791
to
2b018b0
Compare
ad81bb8
to
87dcd72
Compare
548e3de
to
d421ec9
Compare
Current status of this PR (I also updated the PR description):
While testing, I noticed two remaining problems that in my opinion are not specific to the changes introduced by this PR:
|
Just rebased again because of a conflict. Current state:
As discussed with @planger, we currently consider our support for electron playwright experimental (Playwright itself also considers its support for Electron automation experimental). This also means that Windows is not supported at this time, as we still observe plenty of stability issues there. |
I'm O.K. to merge this PR, since it won't break the existing PR checks, however, there are a couple of problems on Windows, which should be addressed in the near future:
I doubt most of these problems are specific to the work in this PR, though, so following the principle of "don't make it worse", I think we can merge this one. |
Thanks for your review @tsmaeder! We should certainly look into the Windows support in near future. |
Contributed on behalf of STMicroelectronics. Change-Id: I7c817b00262229ab096ecbbacca5c465b9fa6302
This patch adds the possibility to disable natively rendered elements by setting the environment variable THEIA_ELECTRON_DISABLE_NATIVE_ELEMENTS to 1. We need this functionality for testing menu actions or use cases that involve file choosers using electron playwright. When the environment variable is set, the app loader enforces the rendering of HTML menus that playwright can access. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
* Use unified AppLoader for both electron and browser tests * Control early window loading of electron via environment variable THEIA_ELECTRON_NO_EARLY_WINDOW * Introduce environment variable USE_ELECTRON to run playwright tests in electron * Add ui-tests-electron target to package.json * Adjust workflow to use the new ui-tests-ci target (only one worker + both electron and browser tests) * Merge electron playwright tests into the existing tests * Skip tests that are currently too flaky in electron: - theia-output-view.test.ts - theia-quick-command.test.ts Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Ensures that the workspace is defined before electron launches. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Instead of using useNativeDialogs() to change bindings, we use to decide whether to call the extended FileDialogService or the parent implementation. This should simplify debugging and help moving away from the environment variable approach towards preferences or something else. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
This generalized flag applies to all electron-based UI and should therefore go to core. Also, the name is more general now. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
As suggested in the code review, we remove the explicit class and convert the options within `TheiaElectronAppLoader` if necessary. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Replaced with factory methods within TheiaApp class. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
path.normalize() does not transform drive letters to lower case, so we revert back to the original version. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <[email protected]>
423c681
to
f8f01fe
Compare
Rebased because of failed license check. |
What it does
Allows to write tests against an Electron app by starting the Electron app first and then attaching Playwright to it. Also, it solves the workspace problem by creating it upfront and pointing to it on app start.
Because playwright cannot access native UI elements, we introduce an optional environment variable
THEIA_ELECTRON_DISABLE_NATIVE_ELEMENTS
that, when set to1
, enforces the rendering of HTML-based menus and file chooser dialogs, which can be accessed by playwright.Playwright also requires us to disable early rendering of the window, which we also do via an environment variable (
THEIA_ELECTRON_NO_EARLY_WINDOW
).A unified TheiaAppLoader is introduced that can be used in both browser and electron tests.
All existing tests are adjusted to use the new AppLoader and can be run in Electron if the environment variable
USE_ELECTRON
is set totrue
.For convenience, a yarn target
ui-tests-electron
has been added to the playwrightpackage.json
.Furthermore, the target
ui-tests-ci
has been added (runs all playwright tests in the browser and in Electron) and the Github workflow has been adjusted.Contributed on behalf of STMicroelectronics
How to test
yarn electron build
yarn electron start
Note that, #12405 still prevents running the playwright tests on Windows or MacOS, which means testing this PR currently requires Linux.
I noticed some flakiness in both browser and electron playwright tests (especially the ones for text-editor, terminal, and outline-view), but I could reproduce these issues on master, so in my opinion, they are not specific to changes introduced in this PR.
Review checklist
Reminder for reviewers