-
Notifications
You must be signed in to change notification settings - Fork 2
Add initial glsp-playwright implementation #1
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
Conversation
ndoschek
left a comment
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 Haydar for that great contribution already! 🎉
As discussed with you on Friday, I have some general remarks for the PR overall:
- Please provide a summary of the features in the PR description
- Squash commits and rebase
- Let's start with the standalone test framework first and add the others (vscode/theia) in a following PRs
- Please include the Playwright installation in the build script (this ensures that on (initial) the necessary dependencies are installed/updated)
- Please extend README for core package and the custom test package
- Headers are currently turned off via ESLint, if we want to merge it, the headers need to be aligned according to the repo license, hence please add the headers (e.g. as in other glsp repos)
- Also, please doublecheck if the package.json files are aligned as well.
- Please doublecheck the scripts, if they are all used actually. E.g. the code coverage via nyc will not work with playwright afaik.
I will look into the details of the implementation this week.
martin-fleck-at
left a comment
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.
Hi @haydar-metin ,
Thank you very much for that really great, initial contribution! I tested the standalone variant and I added a few question and remarks regarding the code in-line but overall it seems to work well.
As Nina already mentioned, there are also some "organizational" aspects we definitely need to consider:
- Documentation: You introduce a lot of concepts like Flows or Capabilities. We definitely need more documentation to properly explain their purpose. It was also not as straight-forward for me to get it to run since installing playwright was not that easy but once it runs, it runs really smooth.
- License: We definitely need a license header on the files. It should be 'EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0' as it is also in the other GLSP repositories.
- Content Review: Since you are not an Eclipse committer and this repository is already under the Eclipse organization, we probably need to do a project content review (https://www.eclipse.org/projects/handbook/#ip-request). We can file the request as soon as the our own code review is done.
- Split: It would be great if we could split out the 'standalone' case as the main contribution and then simply add additional PRs for the 'vscode' and 'theia' case to more clearly separate the functionality.
- Versions: Could you please update @playwright/test to the latest version and @theia/playwright to the overall theia-integration version (i.e., "1.34.2") or maybe even the latest version.
Nevertheless, I want to hightlight that this is a fantastic change and I really like the way the tests read, it is very natural. I think we just need to make sure that this - as a framework - can be consumed, extended and understood easily.
Thank you!
packages/glsp-playwright-core/src/glsp/graph/svg-metadata-api.ts
Outdated
Show resolved
Hide resolved
packages/glsp-playwright-core/src/integration/theia/po/theia-glsp-editor.po.ts
Outdated
Show resolved
Hide resolved
|
Thank you both very much for the feedback! 🎉
|
ndoschek
left a comment
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.
Thanks a lot @haydar-metin for the update! Especially the effort you put into documentation is awesome ✨ !
I did a shallow review, and have some minor comments on the setup, I'll have a deeper look next week.
Thanks again!
martin-fleck-at
left a comment
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 very much for the update @haydar-metin! The change looks really good to me now, just a minor thing in the package.json.
If @ndoschek also approves this PR, I think we can start the process with the Eclipse foundation to get it merged.
ndoschek
left a comment
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.
Thanks a lot again @haydar-metin!
Overall it looks great to me and I think this is a very good first version of the GLSP playwright library! 🎉
I have one minor request, but not blocking, and we could add that also in the next step:
It would be nice to have a top-level test script in the main package.json that delegates to the workflow example test and also mention it how to run the example tests from the top-level README to make it as easy as possible for anyone interested to run the tests.
martin-fleck-at
left a comment
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.
So we agreed that we adapt the scripts in a follow-up PR. With this, I also approve and I'll trigger the process within the Eclipse foundation.
martin-fleck-at
left a comment
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.
So we agreed that we adapt the scripts in a follow-up PR. With this, I also approve and I'll trigger the process within the Eclipse foundation.
|
I opened the content review: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/8507 |
|
Content was approved so I merged the PR 🥳 |
This PR is part of my diploma research conducted at the Model Engineering lab of TU Wien in the Business Informatics Group.
The initial version provides the following content:
Integrations
Theia and VSCode will be included in the next PRs.
Common Page Objects
Interaction Possibilities (CRUD)
Example
Please follow the
README.mdprovided in the examples folder. 37 test cases are provided.