Skip to content
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

Closed
wants to merge 5 commits into from
Closed

Conversation

y-lakhdar
Copy link
Contributor

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

Copy link

Pull Request Report

PR Title

❌ Title should follow the conventional commit spec:
<type>(optional scope): <description>

Example: feat(headless): add result-list controller

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 235.9 235.9 0
commerce 339.1 339.1 0
search 409.5 409.5 0
insight 395.4 395.4 0
recommendation 248.5 248.5 0
ssr 403.5 403.5 0
ssr-commerce 351.4 351.4 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

@y-lakhdar y-lakhdar marked this pull request as draft September 26, 2024 15:10
Copy link
Contributor

@fbeaudoincoveo fbeaudoincoveo left a 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);
Copy link
Contributor

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.

Copy link
Contributor Author

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();
Copy link
Contributor

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

Copy link
Collaborator

@louis-bompart louis-bompart left a 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';
Copy link
Collaborator

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));

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.

Copy link
Contributor Author

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.

@y-lakhdar
Copy link
Contributor Author

Closing for now.
Let's wait and see how scoping E2E tests addresses the throttling issue (#4484)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants