-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Browser: add route.fulfill #4961
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
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.
Good work, despite it being your first PRs 🙇. I've posted some suggestions. We should also fix the linter errors.
| @@ -0,0 +1,73 @@ | |||
| package common | |||
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'm not sure about this refactoring. I'd keep all HTTP-related code in http.go as before, as I don't mind about the file line length. No strong opinion, though.
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'll revert this change! Now that I think again about this, I think it makes sense to keep it in http.go. And if the file size becomes an issue, it would probably make more sense to split it into three to have response.go, request.go and route.go. Reverted in 2e31eae
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.
❤️ In that case, I'd suggest http_response.go, http_request.go, etc.
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 job @AgnesToulet 👏🏻 Nothing stood out to me, besides what @inancgumus mentioned, so I'll wait on his further review comments to 👍🏻
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! Looks good. Just a couple of things to consider first before we can merge this in.
| route.Continue() | ||
|
|
||
| if err := route.Continue(); err != nil { | ||
| m.logger.Warnf("FrameManager:requestStarted", |
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.
Should this also be an error log, like it is in onRequestPaused?
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 believe the other one better be a warning to reduce log clutter.
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 think this one is important because if it fails, it means than the request is stalling and navigation will probably never ends. But at the same time, there is no user option here so I don't know what the user can do about this error (same for the other one in onRequestPaused, pretty important but not much users can do about it).
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.
Updated in 55db130 for consistency. I have no strong opinion on the log level but I think it should not happen often and is important enough to be an error.
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'm okay with using the "error" level for logging. However, if this routing issue is caused by a non-mainframe navigation, such as a CSS route, it could lead to an increase in log entries. This was a significant problem in the past, as we were logging numerous errors that were not related to the success or failure of test runs.
The base branch was changed.
c2e8aaf to
4a66aa0
Compare
|
|
||
| action := fetch.FulfillRequest(request.interceptionID, responseCode) | ||
|
|
||
| if opts.ContentType != "" { |
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'm wondering here... Is it better to keep it like this or actually remove this field from the FulfillOptions struct and directly add it to the Headers option field in parseFulfillOptions method, WDYT?
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 believe it's better to be as close to Playwright as possible in UX terms.
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.
Yes I agree, this would only be "under the hood". The API spec would stay the same but we wouldn't bother with this field in the other layers of the codebase.
eb86c95 to
0ed7528
Compare
| case "body": | ||
| fopts.Body = obj.Get(k).String() |
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.
Same comment as I gave in the continue PR - this should use common.ToBytes and handle this as binary data by the most part.
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.
Updated in adc5394
| } | ||
| case "status": | ||
| fopts.Status = obj.Get(k).ToInteger() | ||
| } |
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.
Looking at https://playwright.dev/docs/api/class-route#route-fulfill - you are not handling path, json and response . Can we have a comment here. Also I feel like handling all but response is likely fairly quick 🤔 .
But if not maybe lets throw an exception instead of silently doing the wrong thing
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 think we should be ok to wait for feedback for response and json.
Instead of working with path we should probably encourage users to work with the current idiomatic k6 way of reading a file and then passing it the bytes to body.
So, yeah, happy with throwing an error if a user tries to work with those fields that aren't implemented.
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.
Updated in 67f24be
I agree that json should be quick to implement and used by a lot of users but wanted to keep the scope of the PR as small as possible. We can definitely iterate on it in future releases.
* Refactor: move route types and function to its own file * Refactor: add route mapping and return errors * Add fulfill function * Add fulfill test * add comments * move back route struct to http.go * add request mapping * remove forgotten log * fix linter issues * rename var * remove pointer to fulfilloptions * remove extra return * update log level to keep it consistent * support buffer in body input * return error for unsupported options
What?
This PR adds the
route.fulfillfunction. Used inpage.route, it allows to fulfill a request with a given response.Why?
This helps with Playwright compatibility and allow to mock requests while testing.
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)
Part of #4502
Known limitation
As you can see in the test script below, if you want to fulfill a request to an external website you need to handle CORS yourself. Playwright had the same issue described here with multiple proposed solution. They fixed it by adding CORS for external request if no overrides are provided: see this function. I tried to do the same but struggled with the request origin not being set on our side. I don't think it's blocking this PR though but I'd like to know your thoughts about this issue, if we should try to follow Playwright or choose another option (document it or allow it if an option is set).
Test script
To test the full workflow, I'm using the following k6 script. It's not part of the PR because it depends on a change on quickpizza: grafana/quickpizza#225 but you can play with it if you run QuickPizza locally.