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.

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

[Internal review] playwright: Update to latest versions and add new page objects #75

Closed
wants to merge 1 commit into from
Closed

[Internal review] playwright: Update to latest versions and add new page objects #75

wants to merge 1 commit into from

Conversation

ndoschek
Copy link

@ndoschek ndoschek commented Apr 3, 2023 •
edited
Loading

What it does

  • Update playwright to the latest version
    • Update test configuration to adapt to latest version
    • Remove obsolete multiple config files and scripts
  • Add page objects for OutputView, OutputChannel, Toolbar, MonacoEditor
  • Extend tests by testing the new page objects
  • Extend ExplorerView tests with compact folders
  • Extend QuickCommand tests
  • Update GETTING_STARTED.md and DEVELOPING.md
  • Update build script to use ensure clean and lint are called
    • Ensure all necessary playwright dependencies are installed on initial build

Signed-off-by: Nina Doschek [email protected]

How to test

  • Run playwright tests via yarn test:playwright from root directory

Review checklist

Reminder for reviewers

@ndoschek ndoschek requested a review from planger April 3, 2023 09:08
planger
planger requested changes Apr 5, 2023
View reviewed changes
Copy link

@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.

Excellent job, thank you!
I also tested the VS Code Playwright extension a bit more and it really seems to be very helpful. Thanks for updating Playwright and pointing us to this nice extension!
Also the new page objects are pretty neat.

I just had a few comments, especially about updating and clarifying the documentation. Otherwise, this looks great. Thanks!

examples/playwright/docs/DEVELOPING.md Outdated Show resolved Hide resolved
examples/playwright/docs/DEVELOPING.md Show resolved Hide resolved
examples/playwright/docs/GETTING_STARTED.md Outdated Show resolved Hide resolved
examples/playwright/docs/GETTING_STARTED.md Outdated Show resolved Hide resolved
examples/playwright/docs/GETTING_STARTED.md Outdated Show resolved Hide resolved
examples/playwright/src/theia-explorer-view.ts
// check if first segment folder needs to be expanded first (if folder has never been expanded, it will not show the compact folder structure)
const firstSegment = path.split('/')[0];
const selector = this.treeNodeSelector(firstSegment);
const folderElement = await viewElement?.$(selector);
Copy link

Choose a reason for hiding this comment

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

Is this safe, if the files have not been yet loaded into the file explorer? We recently had a few flaky tests because we expected file nodes whereas the file explorer has not yet been populated. Therefore, we've switched to waits in a few places, see

Copy link

Choose a reason for hiding this comment

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

Did you double-check the changes of the two PRs above to avoid re-introducing flakiness with the compact file support?

Copy link
Author

Choose a reason for hiding this comment

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

Oh sorry I overlooked that previously. I added a waitForVisibleFileNodes to the compact file state getter.

examples/playwright/src/theia-output-channel.ts Outdated Show resolved Hide resolved
examples/playwright/src/theia-output-view.ts Show resolved Hide resolved
examples/playwright/src/theia-output-view.ts Show resolved Hide resolved
examples/playwright/src/theia-output-view.ts Show resolved Hide resolved
@planger
Copy link

planger commented Apr 5, 2023

Ah and also please make sure the fixes of eclipse-theia#12373 and eclipse-theia#12310 are incorporated (also in new code) to avoid having flaky file explorer tests again.

@ndoschek
Copy link
Author

ndoschek commented Apr 6, 2023

Thank you very much for your review @planger!
I addressed your comments in a following commit and added some comments inline to yours as well.

@ndoschek ndoschek requested a review from planger April 6, 2023 10:14
@ndoschek ndoschek force-pushed the ndoschek/update-playwright branch 3 times, most recently from 299b7e8 to 2206bc3 Compare April 6, 2023 10:52
planger
planger requested changes Apr 6, 2023
View reviewed changes
Copy link

@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.

Excellent, thank you!

If I didn't misunderstand anything, we don't install the dependencies of this package when running yarn in the root, right? Because I think there has been a change (eclipse-theia#12248) to avoid that on purpose and with that avoid slowing down the Theia build. Thus, we should make sure building the root Theia repo doesn't automatically download the Playwright dependencies.

Just a few more minors below. If those are addressed / confirmed, feel free to open the PR against Theia directly.

Thank you!

examples/playwright/docs/DEVELOPING.md Outdated Show resolved Hide resolved
examples/playwright/docs/GETTING_STARTED.md Outdated Show resolved Hide resolved
examples/playwright/docs/GETTING_STARTED.md Outdated Show resolved Hide resolved
examples/playwright/package.json Outdated Show resolved Hide resolved
examples/playwright/src/theia-explorer-view.ts
// check if first segment folder needs to be expanded first (if folder has never been expanded, it will not show the compact folder structure)
const firstSegment = path.split('/')[0];
const selector = this.treeNodeSelector(firstSegment);
const folderElement = await viewElement?.$(selector);
Copy link

Choose a reason for hiding this comment

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

Did you double-check the changes of the two PRs above to avoid re-introducing flakiness with the compact file support?

@ndoschek
playwright: Update to latest versions and add new page objects
bfecfc9 
- Update `playwright` to the latest version
  - Update test configuration to adapt to latest version
  - Remove obsolete multiple config files and scripts
- Add page objects for OutputView, OutputChannel, Toolbar, MonacoEditor
- Extend tests by testing the new page objects
- Extend ExplorerView tests with compact folders
- Extend QuickCommand tests
- Update GETTING_STARTED.md and DEVELOPING.md
- Update build script to use ensure clean and lint are called
  - Ensure all necessary playwright dependencies are installed on initial build

Signed-off-by: Nina Doschek <[email protected]>
@planger planger self-requested a review April 6, 2023 12:03
@ndoschek
Copy link
Author

ndoschek commented Apr 6, 2023

Thanks again @planger, I doublechecked again, I was not aware of that earlier change, but that makes a lot of sense now. As the prepare script was removed, we lost some functionality (i.e. clean and lint) which I now packed into the build script.
And I doublechecked, the playwright dependencies are not downloaded with the regular theia build, only on the first theia/playwright build (either triggered separately or on the first test run).

@ndoschek
Copy link
Author

ndoschek commented Apr 6, 2023

Closing in favour of eclipse-theia#12384

@ndoschek ndoschek closed this Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@planger planger Awaiting requested review from planger

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ndoschek @planger