✅ [RUM-17233] Add Shopify checkout views and actions e2e test - #4941
✅ [RUM-17233] Add Shopify checkout views and actions e2e test#4941lierniel wants to merge 5 commits into
Conversation
Bundles Sizes Evolution
|
|
BeltranBulbarellaDD
left a comment
There was a problem hiding this comment.
LGTM! Just some minor comments.
|
|
||
| export function buildShopifyUrl(): string { | ||
| return SHOPIFY_STORE_URL | ||
| } |
There was a problem hiding this comment.
Maybe since it's a constant we don't need this?
| // Matches the CDN URL used by the store's Theme Liquid snippet and Custom Pixel for the main | ||
| // bundle and its dynamically-imported chunks (e.g. the session replay recorder), served from | ||
| // `https://www.datadoghq-browser-agent.com/<site>/v<major>/[chunks/]<name->]datadog-rum-shopify.js` | ||
| const SHOPIFY_ASSET_URL_PATTERN = |
There was a problem hiding this comment.
Okay not related specifically to your PR but how about making a function to reuse with Salesforce? Since it's the same pattern except for datadog-rum-shopify vs datadog_rum_salesforce?
| // enough to trip Cloudflare's wall or Shopify's checkout rate limit | ||
| test.beforeAll(async () => { | ||
| await new Promise((resolve) => setTimeout(resolve, 15_000)) | ||
| }) |
There was a problem hiding this comment.
I think we can remove this. Since for now we only have 1 test and it's scheduled.
There was a problem hiding this comment.
Actually we still could have a few retries happening sequentially even with scheduled setup. I would keep it just in case, I believe it doesn't hurt to have anyway
| const cart = (await response.json()) as { item_count: number } | ||
| return cart.item_count | ||
| }) | ||
| await expect.poll(getCartItemCount, { timeout: 10000 }).toBeGreaterThan(0) |
There was a problem hiding this comment.
I think we can remove the timeout. At least I did locally and we are good.
There was a problem hiding this comment.
This timeout is only for polling /cart.js with the interval (playwright docs). We're waiting for the amount of the items in cart in the response to be greater than 0 in order to avoid clicking on the "Checkout" button before items is actually landed to the cart. It might bypass without this polling a few times, but in a higher volumes of runs it could hit.
I was running it in the loop of 40 sequential runs when I hit it and after this fix it doesn't happen anymore, therefore I'd rather keep it - but let me know if you disagree and want to discuss!
There was a problem hiding this comment.
Oh okay, makes sense, it's just that with the first promise and then the timeout seemed redundant at least in 1/2 runs I tried.
There was a problem hiding this comment.
Oh, indeed, the timeout option may be redundant since it defaults to 30s - I'll remove it 👍
|
|
||
| // Card fields sync back to the parent form via an async postMessage; give it a moment before | ||
| // clicking "Pay now" so the click doesn't land before the form is submittable. | ||
| await page.waitForTimeout(500) |
There was a problem hiding this comment.
By default the page.getByRole wait's 30s. So I don't think this is necessary.
There was a problem hiding this comment.
Yeah, but the thing is: the Pay now button is available immediately so page.getByRole will be executed right away without waiting for the element to appear (because it's already on the page from the start). But before clicking on Pay now I want to make sure that card fields form (which is on separate iframe) is synced with the parent form on the top frame.
Without that it might happen that regardless of the card fields was already filled, clicking on the Pay now button fails because card details are missing (the parent form haven't received the update from card fields form).
That's why I added this waitForTimeout(500) - it's enough time for the card details form to finish syncing with parent form and now clicking on the Pay now button never fails
|
|
||
| await page.getByRole('button', { name: 'Pay now' }).click() | ||
|
|
||
| await expect(page.getByRole('heading', { name: /thank you/i })).toBeVisible({ timeout: 30000 }) |
There was a problem hiding this comment.
(I tested locally and it passed)
|
|
||
| const orderedViews = [...intakeRegistry.rumViewEvents] | ||
| .sort((a, b) => a.date - b.date) | ||
| .filter((event, index, events) => events.findIndex((e) => e.view.id === event.view.id) === index) |
There was a problem hiding this comment.
All of this is because we navigate back. Do we care about this assert? To go back to the page? Since we know the SDK worked correctly until now?
There was a problem hiding this comment.
I'm not sure I got it properly, but we want to verify that the session stays the same after transition storefront page -> checkout page and vice verca. Here I'm sorting view events by time and filter them by uniq id to confirm that they came in correct order with right url
There was a problem hiding this comment.
Oh yeah vice verca I understand. Well it makes sense.
| // Shopify apps don't serve a locally-generated page body; this factory only intercepts the | ||
| // bootstrap script request and injects the RUM configuration read by the store's Theme Liquid | ||
| // snippet and Custom Pixel. | ||
| export async function shopifySetup(options: SetupOptions, servers: Servers, page: Page): Promise<string> { |
There was a problem hiding this comment.
suggestion: those setup functions are templates to render an HTML page the e2e test will load. It seems like you are doing very different things here. What about moving this logic in shopify.scenario.ts?
There was a problem hiding this comment.
I was following the example of the Salesforce, which has similar setup pattern
It seems like you are doing very different things here
Actually not that different - the part of rendering an HTML page for e2e test is injecting the local sdk version and passing the init options for it - and this is exactly what I'm doing here. This method doesn't render the html - it's rendered by predefined Shopify dev-store - but I still need to override the sdk bundle path and pass the init config for it. Therefore I believe it's the right place to do this, it would look overly cumbersome in shopify.scenario.ts - but let me know if you disagree, I'm open to the discussion :)
| }) | ||
|
|
||
| // Flow: add to cart -> checkout -> thank you -> back to storefront | ||
| createTest('shopify checkout views and actions') |
There was a problem hiding this comment.
nitpick: this test is huge. Is there any way to split it, so it's easier to maintain and understand what's failing?
There was a problem hiding this comment.
Yeah, but the reason behind putting everything in one test is the Claudflare bot wall we were discussing on Wednesday. I wanted to avoid parallel runs as much as possible - so I've created one big test to cover everything. It's true that with this approach it's harder to maintain and understand what is failing, but it's a trade of for avoiding multiple runs from happening at the same time to bypass Claudflare.
However I believe we could soften the assertions with expect.soft() so test would keep running even if some assertions fail to better understand that's really broken, wdyt?
There was a problem hiding this comment.
suggestion: this file is not just about shopify urls. Maybe rename it to shopify.ts or shopifyUtils.ts?
Motivation
RUM-17233: cover the Shopify integration with e2e tests. The dedicated
browser-rum-shopifybundle (#4878) explicitly scoped e2e tests out; this closes that gap by exercising the full checkout flow against a real Shopify dev store.Changes
buildShopifyUrl/unlockShopifyStorePassword(dev store password gate),withShopifyApp()/shopifySetup(intercepts the CDN bundle request and injects RUM configuration into both the storefront's Theme Liquid snippet and the checkout's Custom Pixel sandbox), and agetShopifyStorePassword()secret accessor.shopify.scenario.ts, covering the full flow: storefront → product page → add to cart → checkout → thank-you page → back to storefront. Asserts:browser-rum-shopify)test.slow(), since this hits a live dev store that's sensitive to request volume (Cloudflare bot protection, checkout rate limiting) and the full flow runs close to the default test timeout.Test instructions
To run locally:
Or trigger the according pipeline on the gitlab: https://gitlab.ddbuild.io/DataDog/browser-sdk/-/pipelines
Chromium only (
--disable-web-securityis required to bypass the dev store's CSP/CORS restrictions). Runs against the livecustom-pixel-e2e.myshopify.comdev store, so repeated back-to-back local runs may need a few minutes' gap to avoid tripping Cloudflare's verification wall.Checklist