-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Implement the locator.getBy* methods
#5106
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
ankur22
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.
Nice work 🙇
I think we should test the change, even though most of the implementation is already covered in the get_by_test.go file.
It could just be a subset of those tests translated to work with locator instead of page, so for each it could just be:
- a happy path test that works with the required fields;
- a happy path test that works with an option;
- a happy path using locator chaining e.g.
page.locator(#table).getByText('Hello world').click();. - a failing test to ensure we are testing
nullvalues for the required fields.
It might just be a case of copying the whole file and using locator instead of page.
Happy for you to merge this PR in and work on the test in another PR if you wish.
6cb93a0 to
c343343
Compare
c343343 to
b6a4edd
Compare
ankur22
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.
LGTM 🚀
AgnesToulet
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.
LGTM! Agree that these should be tested though but I guess you are waiting for #5105 to be merged to build upon your changes there.
Yeah, exactly! I'm awaiting for the other PR to be merged to try to get |
|
Hi @AgnesToulet, @ankur22, I've reused the changes introduced in #5105 to also test the Locator API mappings introduced in this changeset, on the HTML doc's root ( Thanks for your support! 🙌🏻 |
ankur22
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.
LGTM 😄
74061bb to
d23f35a
Compare
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.
Dear reviewer, take a look at the following discussion #5106 (comment) to get more context why this changed is needed. Thanks! 🙇🏻
…sed on Playwright's behavior
d23f35a to
4f3497a
Compare
| }) | ||
| } | ||
|
|
||
| // We test the 'document' role independently, because the expectations |
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.
Dear reviewer, take a look at the following discussion #5106 (comment) to get more context why this changed is needed. Thanks! 🙇🏻
What?
It implements all the
getBy*methods for thelocatorAPI.Why?
Because we have already implemented all those methods for
pageandframe, and we want to implement them as well forlocatoras a way to increase our compatibility with Playwright, as described in #5029.Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #5029