8000 Fix playwright tests on Windows by mvtec-richter · Pull Request #15684 · eclipse-theia/theia · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix playwright tests on Windows #15684

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 18, 2025
Merged

Fix playwright tests on Windows #15684

merged 3 commits into from
Jun 18, 2025

Conversation

mvtec-richter
Copy link
Contributor

What it does

Simplify helper methods and properties for filesystem paths and urls to workspace.
In my opinion, previously the meaning of urlEncode and escape was not clear. This was part of the reason, the methods didn't work for Windows. I tried to align the naming of the methods with the specific use cases.
Unfortunately, I don't have access to MacOS to test this.

Fixes #15447

How to test

Run playwright tests on Windows and Linux.

Follow-ups

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Contributed by MVTec Software GmbH

Review checklist

Reminder for reviewers

@mvtec-richter
Fix playwright tests on Windows
28f3e35 
* Simplify helper methods and properties for filesystem paths and urls
  to workspace
* Make sure they work for Windows and Linux
* Fixes eclipse-theia#15447

Signed-off-by: Florian Richter <florian.richter@mvtec.com>
@github-project-automation github-project-automation bot moved this to Waiting on reviewers in PR Backlog May 28, 2025
@JonasHelming JonasHelming requested a review from planger June 16, 2025 18:30
planger
planger approved these changes Jun 16, 2025
View reviewed changes
Copy link
Contributor
@planger planger left a comment

Choose a reason for hiding this comment

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

I can't test on Windows, but on Linux this seems to still work fine. Thank you!

@github-project-automation github-project-automation bot moved this from Waiting on reviewers to Needs merge in PR Backlog Jun 16, 2025
@planger
Copy link
Contributor
planger commented Jun 16, 2025

@tsmaeder Can you test this on Windows please?

@tsmaeder
Copy link
Contributor

I get

 5 failed
    src\tests\theia-explorer-view.test.ts:60:9 › Theia Explorer View › should be possible to close and reopen it
    src\tests\theia-notebook-editor.test.ts:87:9 › Theia Notebook Editor interaction › should execute all cells
    src\tests\theia-notebook-editor.test.ts:177:9 › Theia Notebook Cell interaction › Execute code cell and read output
    src\tests\theia-notebook-editor.test.ts:186:9 › Theia Notebook Cell interaction › Check execution count matches
    src\tests\theia-notebook-editor.test.ts:280:9 › Theia Notebook Cell interaction › Check Collapse output switch `o` works
  3 skipped
  86 passed (9.8m)

on Windows 11, but I did the playwright tests ever work correctly on WIndows anyway? @mvtec-richter what are your results on Windows?

@mvtec-richter
Copy link
Contributor Author

I get

 5 failed
    src\tests\theia-explorer-view.test.ts:60:9 › Theia Explorer View › should be possible to close and reopen it
    src\tests\theia-notebook-editor.test.ts:87:9 › Theia Notebook Editor interaction › should execute all cells
    src\tests\theia-notebook-editor.test.ts:177:9 › Theia Notebook Cell interaction › Execute code cell and read output
    src\tests\theia-notebook-editor.test.ts:186:9 › Theia Notebook Cell interaction › Check execution count matches
    src\tests\theia-notebook-editor.test.ts:280:9 › Theia Notebook Cell interaction › Check Collapse output switch `o` works
  3 skipped
  86 passed (9.8m)

on Windows 11, but I did the playwright tests ever work correctly on WIndows anyway? @mvtec-richter what are your results on Windows?

I'll have a look into this. The notebook tests also failed on Linux. I will merge the master and see if I can fix them if they are not fixed already.

@planger
Copy link
Contributor
planger commented Jun 17, 2025 •
edited
Loading

Yes, those tests are somewhat flaky, see also #14450
The theia-explorer-view test should be stable though, as far as I have observed.

@mvtec-richter
Copy link
Contributor Author

The playwright tests previously did not work at all on Windows. The tests couldn't even open a workspace, because the paths were not handled correctly. This should be fixed now.
@tsmaeder For me, some of the notebook tests failed, because the ipykernel python package was not installed. The error messages further explain how to install it.
I fixed one tests, however, I'd like to fix the remaining notebook editor tests in a separate pull request, so the changes don't get mixed up.

@mvtec-richter
Simplify waitForCellStatus
0bdaae3 
* The previous implementation did not wait at all. Furthermore only waiting for success and error together was used until now.

Signed-off-by: Florian Richter <florian.richter@mvtec.com>
@mvtec-richter mvtec-richter force-pushed the fix-playwright-tests-on-windows branch from c4e8401 to 0bdaae3 Compare June 17, 2025 13:10
@tsmaeder
Copy link
Contributor

because the ipykernel python package was not installed.

@mvtec-richter certainly not your fault or part of this PR, but I wonder if it makes sense to use the highly complex python notebooks to test our notebook support. Wouldn't something with less dependencies make more sense and use less resources?

@planger
Copy link
Contributor
planger commented Jun 18, 2025

Yeah agreed. I think this PR already certainly improves the situation on Windows and doesn't break anything on Linux. So from my point of view, this PR will add value as is.

If we are at it, it might be worth checking if we can fix the theia-explorer-view.test.ts on Windows, but even if we can't, this shouldn't block this PR, as this test didn't work before either afaik.

@tsmaeder
Copy link
Contributor

@planger don't you want to merge this one?

@planger
Copy link
Contributor
planger commented Jun 18, 2025

I do want to merge this one (sorry if I wasn't clear :-)). Happy to merge it if you agree!

@tsmaeder
Copy link
Contributor

You're a committer, you can decide these things all by yourself (unless you want me to review the code).

@planger
Copy link
Contributor
planger commented Jun 18, 2025

Sure, I wanted to make sure you are on board as you had a look and some comments!
Thanks for the feedback! 👍

@planger planger merged commit 60fb708 into eclipse-theia:master Jun 18, 2025
10 of 11 checks passed
@github-project-automation github-project-automation bot moved this from Needs merge to Done in PR Backlog Jun 18, 2025
@github-actions github-actions bot added this to the 1.63.0 milestone Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@planger planger planger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
PR Backlog
Archived in project
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.

Broken playwright test on Windows using TheiaTextEditor
3 participants
@mvtec-richter @planger @tsmaeder
0