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

Add electron tests #28

Merged
merged 1 commit into from
Mar 29, 2024
Merged

Conversation

xai
Copy link
Contributor

@xai xai commented Feb 29, 2024

  • Update theia dependency to 1.46.1
  • Use new TheiaApploader
  • Add workaround for Electron playwright multi-root issue
  • Adjust workflows
  • Rename existing performance reports to performance/chromium

The electron performance tests depend on Theia > 1.46.1 because they need to access the metrics endpoint.

@xai xai force-pushed the electron-playwright branch 2 times, most recently from c4cc3a8 to 8ff3f08 Compare March 26, 2024 13:12
@xai xai marked this pull request as ready for review March 26, 2024 13:30
@xai xai force-pushed the electron-playwright branch 2 times, most recently from bddfe24 to 5c31aa0 Compare March 26, 2024 13:54
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.

Thanks for this PR! Looks good overall, just a few initial inline comments that caught my attention when looking through the change. Thank you!

python-version: "3.11"

- name: Test (playwright electron)
uses: GabrielBB/xvfb-action@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably avoid using this GH action, as it is deprecated.

with:
node-version: "16.x"
registry-url: "https://registry.npmjs.org"
- name: Use Python 3.x
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to pin this to version 3.11?

@xai
Copy link
Contributor Author

xai commented Mar 27, 2024

I also noticed that we currently use several different versions of actions in the workflows (e.g., v2, v3, and v4 of checkout). If that is ok, I would create a follow-up PR to fix this while at the same time replacing the (mutable) tags with commit hashes.

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.

Thank you very much @xai! This looks great so far. I'm eager to try it on the CI infrastructure.

Aside from a minor comment inline I just wanted to double-check the following questions:

  • Do we keep the history of the browser performance tests when switching base directory or is there something we need to do?
  • How does it impact the allure reports when both browser and electron write into the same allure-results folders before we generate the allure report? Are they then both showing up in the report or does one override the other?
  • Can we push a simple performance/index.html with two links that point to performance/Browser and performance/Electron to allow people who still may have the old link to navigate to the reports instead of getting a 404? Or would that be cumebrsome?
  • As a follow-up we should also update the readme and modify/add the badge to point to both of the performance pages.

Comment on lines 37 to 45
if (app.isElectron) {
const explorer = await app.openView(TheiaExplorerView);
await explorer.waitForVisibleFileNodes();
console.log(`app.workspace.path = ${app.workspace.path}`);
const workspaceBasename = app.workspace.path.split('/').pop();
if (workspaceBasename) {
await app.page.getByText(workspaceBasename).click();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe extract this into an own function that takes the app as a parameter to avoid repeating this code in several tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, I will adjust the PR!

* Update theia dependency to 1.46.1
* Use new TheiaApploader
* Add workaround for Electron playwright multi root issue
* Support different projects (e.g., chromium or Electron) in performance reports
* Remove dependency to deprecated xvfb-action

Contributed on behalf of STMicroelectronics

Signed-off-by: Olaf Lessenich <[email protected]>
@planger planger self-requested a review March 29, 2024 11:08
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.

Thank you for the update! Looks good now.
As discussed offline, the allure reporting should be combined automatically and to maintain the history of the performance metrics, we'll adjust the gh_pages branch after merging accordingly.

@planger planger merged commit 53cbac9 into eclipse-theia:main Mar 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants