-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add waitForURL in frame and page #4920
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
base: master
Are you sure you want to change the base?
Conversation
8b84803
to
2af3f2c
Compare
0e59b82
to
a05d7b7
Compare
82d6e92
to
e651203
Compare
17a3e84
to
8639485
Compare
2a7395a
to
afdd0d8
Compare
6614b61
to
0c0050c
Compare
ff4ae78
to
daa30bb
Compare
var val string | ||
switch url.ExportType() { | ||
case reflect.TypeOf(string("")): | ||
val = fmt.Sprintf("'%s'", url.String()) // Strings require quotes | ||
default: // JS Regex, CSS, numbers or booleans | ||
val = url.String() // No quotes | ||
} |
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.
I will refactor this into a helper method once this PR is merged in. There are several other PRs which work with similar/same logic to differentiate between regex and strings.
The implementation and the behaviour was copied from Playwright.
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.
Great work! 🚀
Do we plan to deprecate WaitForNavigation
(and maybe remove it in the next major version)? If so, do we have/should we have an issue to plan for that? I see it's deprecated in PW doc and I think it makes sense to remove it from our API as our goal is to have a simpler API.
@@ -2891,3 +2891,109 @@ func TestWaitForNavigationWithURL_RegexFailure(t *testing.T) { | |||
) | |||
assert.ErrorContains(t, err, "Unexpected token *") | |||
} | |||
|
|||
func TestWaitForURL(t *testing.T) { |
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.
Nit: It might be clearer to split this into several tests (the %s
are not easy to map with the actual value and if one test fails here, it won't let the others to continue, which helps with debugging and fixing all the failing tests faster).
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.
Yeah, i agree. I will refactor the test in a refactoring PR along with this change i want to make.
I'm not 100% sure what the next steps are for |
What?
This adds support for a asked for API from Playwright --
page.waitForURL
. This API does a few things more than the equivalentpage.waitForNavigation({ url: ... })
:page.waitForNavigation({ url: ... })
.Why?
This is a more robust API than
page.waitForNavigation
when a user wants to wait for a specific URL, since it builds onpage.waitForNavigation
to prevent race conditions where a page completes navigation beforepage.waitForNavigation
is setup.It's just another convenient way of waiting for something specific when a navigation occurs, which is especially important when many redirects occur.
Note: It's still recommended that the user wait for specific elements or just work with the locator API which auto waits when the page does navigate to the specified URL.
Example usage:
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)
#4385