-
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
#10793 Fix playwright tests for Windows and MacOS #10826
#10793 Fix playwright tests for Windows and MacOS #10826
Conversation
@colin-grant-work : As far as I remember you had a mac, right? Would you mind evalluating the fixes on the Playwright tests on your machine? |
I've still got quite a number of failures running this code on MacOS:
|
Thanks for your feedback @colin-grant-work! |
@ndoschek, here is the full failure log:
And a zip of the |
Just to confirm that I also experienced issues with the tests on macOS using the following:
I get the following test summary:
Additional Info
|
Thanks for your feedback, I will have another look! |
Thanks to @colin-grant-work 's screenshots I could track it down a little, and it seems that the alternative shortcut for opening the command palette ( I tested again with Ubuntu 20.04, Windows 11 and MacOS 12.2.1 and it worked on my side in multiple runs. |
@ndoschek, I'm a bit suspicious of the fix, because |
With the new code, all the tests pass for me. 👍 Were so many tests failing because they depend on the command palette and the shortcut was failing, or were they failing because they were bailing after the first test of the command palette failed? |
I'm still suspicious of the fix a bit: I brought back the OS check: |
@colin-grant-work Thanks a lot for re-testing so quickly!
Yes, they depend on the command palette, as the generic function to open any
I also rebased the fix, but unfortunately I cannot tell either, why they failed for you in the first run (also, as I tested them on all three OSs, before and after). If you agree, I would also prefer to stick to the initial shortcut (and hence to drop the additional commit). |
I confirmed that the tests now pass on macOS, I'm not sure what the difference is.
I confirmed that the original shortcut also works, I'm fine with reverting back to it. |
I'm also in favor of reverting to the original shortcut 👍 . @vince-fugnitto, is it possible that it all had to do with global shortcuts somehow? |
- Fix workspace path encoding for Windows - Stabilize workspace path fetching - Fix explorer review title button selectors - Introduce OSUtil helper functions - Fix shortcuts for MacOS - Improve opening of TheiaViews (speeds up tests by approx. 10s) Contributed on behalf of STMicroelectronics Signed-off-by: Nina Doschek <ndoschek@eclipsesource.com> Fixes eclipse-theia#10793
Thank you both for your feedback! 👍 |
@colin-grant-work, @vince-fugnitto |
@ndoschek, it looks like CI is green (I suspect @vince-fugnitto re-ran the failing builds - unfortunately, the issues with Playwright on MacOS are not our only CI issues, but this PR will go some way to addressing at least one CI issue, so it's progress). Thanks for the contribution! |
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.
Looks good to me. The tests now pass on a variety of OS's, so they can be run by developers in various environments.
What it does
Contributed on behalf of STMicroelectronics
Signed-off-by: Nina Doschek ndoschek@eclipsesource.com
Fixes #10793
Note: I ran tests multiple times on Linux, MacOS and Windows and can confirm that tests are passing (different execution times due to different system specs).
How to test
Run playwright tests:
yarn
yarn browser build
yarn test:playwright
Review checklist
Reminder for reviewers