-
Notifications
You must be signed in to change notification settings - Fork 34
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
test(atomic): POC - cache API requests #4468
Conversation
Pull Request ReportPR Title❌ Title should follow the conventional commit spec: Example: Live demo linksBundle Size
SSR Progress
Detailed logssearch : buildInteractiveResultsearch : buildInteractiveInstantResult search : buildInteractiveRecentResult search : buildInteractiveCitation search : buildGeneratedAnswer recommendation : missing SSR support case-assist : missing SSR support insight : missing SSR support commerce : missing SSR support |
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 😁
@@ -19,3 +20,13 @@ export const makeAxeBuilder: Parameters< | |||
|
|||
await use(makeAxeBuilder); | |||
}; | |||
|
|||
base.beforeEach(async ({page}) => { | |||
await page.route(/commerce\/v2\/search/, cacheHandler); |
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 also currently have playwright tests that call non-commerce API endpoints, so we'd have to modify this accordingly.
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. We also need to handle request to the listing endpoint.
|
||
export const cacheHandler = async (route: Route, request: Request) => { | ||
console.log('intercepted ---', request.url()); | ||
const {clientId, ...rest} = request.postDataJSON(); |
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.
If we want to handle search API requests too, I think we could do:
const {clientId, analytics, ...rest} = request.postDataJSON();
although this might be too broad...
another option would be to filter out non-static params case-by-case, based on the request.url
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.
That's a darn smart approach.
@@ -0,0 +1,40 @@ | |||
import {APIResponse, Request, Route} from '@playwright/test'; | |||
import {createHash} from 'crypto'; |
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 remember you talked about browser compat for crypto.
fwiw: globalThis.crypto.subtle.digest
would probably do a similar trick as createHash, for both NodeJS & the browser
export const cacheHandler = async (route: Route, request: Request) => { | ||
console.log('intercepted ---', request.url()); | ||
const {clientId, ...rest} = request.postDataJSON(); | ||
const key = hashRequest(request.url(), JSON.stringify(rest)); |
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.
question: Are you using the same authentication for each requests? Two exact request with the same payload may have different response depending on the authentication used.
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 point! IIRC it's the same authentication for all test suites. I don't foresee the need of changing the authentication for specific tests since we target a public endpoint.
Closing for now. |
Use Playwright routing capabilities to intercept and cache HTTPS requests.
This does not change the way we do tests. The only change is that all fixtures need to pull the
test
object from the base-fixture rather than directly from@playwright/test
.For details: https://docs.google.com/document/d/1EgDvTyny5YBQgIUIvAZmTJG_OochTZdpikui6pjoT_M/edit#heading=h.uu6jdy7o7j5b