-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 page.on('response')
#4296
base: add/page-on-request
Are you sure you want to change the base?
Add page.on('response')
#4296
Conversation
05b8969
to
6ac9bd0
Compare
3dd1239
to
78ccfc3
Compare
6ac9bd0
to
09fbe5d
Compare
50e414d
to
e51b45d
Compare
221509d
to
294a817
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.
My comments about this one would be similar to #4290. They are very alike.
p.eventHandlersMu.RLock() | ||
defer p.eventHandlersMu.RUnlock() | ||
for _, h := range p.eventHandlers[EventPageResponseCalled] { | ||
err := func() error { | ||
// Handlers can register other handlers, so we need to | ||
// unlock the mutex before calling the next handler. | ||
p.eventHandlersMu.RUnlock() | ||
defer p.eventHandlersMu.RLock() | ||
|
||
// Call and wait for the handler to complete. | ||
return h(PageOnEvent{ | ||
Response: resp, | ||
}) | ||
}() | ||
if err != nil { | ||
p.logger.Warnf("onResponse", "handler returned an error: %v", err) | ||
return | ||
} | ||
} | ||
} |
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.
We start to duplicate this logic in Page.urlTagName
, Page.onRequest
, and Page.onConsoleAPICalled
(although this one differs a little). What do you think about introducing a helper function in #4290?
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 had a feeling you were going to point that out 😄 Sure, i think that should be done in a separate refactor PR 👍
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 refactored the test based on comments made in #4290.
2709a6c
to
522a411
Compare
294a817
to
fea5839
Compare
This is where the response will be set for the page.on handler to read.
This method is to be called when a new response has been received from the WuT to chrome. It takes the response and sends it to the page.on handler where the user test script can read the response data.
There seems to be a race condition with chrome and CDP requests when trying to retrieve the response body. If the call to retrieve the response body is too "quick", then we get an error back. However with retries and a sleep it seems to solve the issue with the small tests i have performed. I suspect this will be a greater issue in the wild.
The refactor tries to work with the dynamic server values. It also works with the defined types in common instead of a map.
Work with slices.IndexFunc and sort HeadersArray before comparing the response and expected.
fea5839
to
0833a1d
Compare
What?
This implements the
page.on('response')
API from Playwright. When the handler is created, all subsequent responses will be directed to the handler, where the user can interact with the read only response object. The response is intercepted after it is received from the website under test to the browser.An example usage is:
Why?
This can be useful to:
Checklist
make lint
) and all checks pass.make tests
) and all tests pass.Related PR(s)/Issue(s)
Updates: #4234
Linked to: #4290