Implement the frame.getBy* methods
#5105
Conversation
There was a problem hiding this comment.
Woohoo! Nice and easy 😄
I think we should test the change still, 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 frame 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 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 frame instead of page.
Happy for you to merge this PR in and work on the test in another PR if you wish.
Thanks @ankur22! 🫶 -- What I like about this approach is that hopefully later it will be pretty easy to adjust it to #5106, with an empty or equivalent locator that make existing test cases still applicable. Beyond that, the only other approach I could think of is directly testing the JS surface, but I see we don't have tests like that neither for |
| } | ||
|
|
||
| for implName, impl := range getByRoleImplementations { | ||
| t.Run(implName, func(t *testing.T) { //nolint:paralleltest |
There was a problem hiding this comment.
I'm intentionally not calling t.Parallel(), as I think others suggested to not use the same browser instance for parallel tests (the reason why I refactored the TestGetByRole* to follow the same pattern as the other tests in this file).
Instead, I only use t.Run as a syntatic way of running the same test code that calls the function and does some assertions over both implementations, on the same instance, which must be fine, but not in parallel.
ankur22
left a comment
There was a problem hiding this comment.
Thanks for the changes! I like how you incorporated the frame into the existing tests 👍 LGTM 🚀
| tb := newTestBrowser(t, withFileServer()) | ||
| p := tb.NewPage(nil) | ||
| opts := &common.FrameGotoOptions{ | ||
| Timeout: common.DefaultTimeout, | ||
| } | ||
| _, err := p.Goto( | ||
| tb.staticURL("get_by_role_implicit.html"), | ||
| opts, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.Equal(t, tt.expected, c) | ||
|
|
||
| if tt.expectedText != "" { | ||
| text, _, err := l.TextContent(sobek.Undefined()) | ||
| require.NoError(t, err) | ||
| require.Equal(t, tt.expectedText, text) | ||
| getByRoleImplementations := map[string]interface { | ||
| GetByRole(role string, opts *common.GetByRoleOptions) *common.Locator | ||
| }{ | ||
| "page": p, | ||
| "frame": p.MainFrame(), | ||
| } |
There was a problem hiding this comment.
What about putting these lines in a function? I feel like it's repeated a lot in this test file (at least for the GetByRole function which has a lot of tests).
There was a problem hiding this comment.
Right, good idea! What do you think about 353a197?
What?
It adds the mapping for all the
getBy*methods for theframeAPI.Why?
Because we have already implemented all those methods, and we want to expose them as a way to increase our compatibility with Playwright, as described in #5037.
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 #5037