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

Add E2E testing to track events across all page types #385

Merged
merged 24 commits into from
Mar 11, 2024
Merged

Conversation

mikkamp
Copy link
Contributor

@mikkamp mikkamp commented Mar 11, 2024

Changes proposed in this Pull Request:

With the focus on shifting towards blocks yet still supporting classic pages, we have to cover the following types of pages to track their events:

  1. Traditional pages using the classic shortcodes.
  2. Default WooCommerce pages which are created during install, most have been switched to blocks when starting with a new site.
  3. The "Products (Beta)" block.
  4. The "Product Collection (Beta)" block. This one is intended to replace the Products block.

To test this manually is a large burden. This PR copies over the E2E we have for tracking in GLA and modifies them to work with the GA events we send. This makes it easier to test the events across several different page/block types.

The tests in this PR did highlight some inconsitencies, and events that aren't being tracked, so the following changes are needed to make these tests pass:

Detailed test instructions:

  1. Install any required patches as listed above
  2. Install all npm dependencies nvm use && npm ci
  3. Build assets either through npm start or npm run build
  4. Ensure we don't have any conflicting E2E docker instances running (like the one from GLA)
  5. Start the wp-env site npm run wp-env:up
  6. Install browser (only needed once) npx playwright install chromium
  7. Run tests npm run test:e2e
  8. Confirm tests are successful

Note: workflow is included (direct copy from GLA). We can only trigger it once this PR is merged (unless you want to clone the repo to test it).

Additional details:

These test aim to cover the most common scenarios, but while testing I still came across the following inconsistencies/parts we aren't covering (these can be addressed in follow up PR's):

  • Remove from cart with shortcode [woocommerce_cart] isn't always tracked because of immediate page redirect (not in E2E)
  • Remove from cart / Begin checkout is not consistent between blocks and shortcode, category and variation data differ.
  • Change cart quantity is not tracked with shortcode [woocommerce_cart] or with blocks (PR Remove unused function from formatters #375 removes it)
  • Add shipping info doesn't seem to be used anywhere (mentioned in Remove unused function from formatters #375 (comment))
  • Select content is difficult to test if it is followed up by a page redirect (no E2E tests written yet)
  • begin_checkout logs total item price (including discounts and price * quantity), purchase logs product object price (not final price we paid in the order). Should these be consistent?
  • When we hook into woocommerce_add_to_cart, should we get final price from cart_item_data? Otherwise we'll track product prices vs. discounted price.
  • When we change the settings to redirect to cart page after add_to_cart, do we still capture all details?
  • Testing with grouped / bundled products (grouped product seems to only track first product added to cart)

Changelog entry

  • Dev - Add E2E testing to track events across all page types.

@mikkamp mikkamp self-assigned this Mar 11, 2024
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Mar 11, 2024
@mikkamp mikkamp requested a review from a team March 11, 2024 09:31
@tomalec tomalec self-requested a review March 11, 2024 10:33
Comment on lines 49 to 51
if ( ! ( await pageExistsByTitle( title ) ) ) {
await createPage( title, content );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📜 💅
If we're going to extract those to shared lib, we could consider all create*Page functions to return the created page id. to keep their behavior consistent

Copy link
Member

@ianlin ianlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only verified if all the e2e tests were passed. Here's my step:

  • Run e2e tests on this branch, most of the tests failed which is expected
  • Check out a new branch based off this PR's branch dev/add-e2e-tests
  • Merge origin/fix/add_to_cart_selector from Fix - Selectors in Product Blocks #378 into the new branch
  • Merge origin/fix/get-data-for-variations from Get correct variation data when formatting product #384 into the new branch
  • Update code locally based on Fix - Selectors in Product Blocks #378 (comment)
    --- a/assets/js/src/integrations/classic.js
    +++ b/assets/js/src/integrations/classic.js
    @@ -162,7 +162,9 @@ export function trackClassicPages( {
      // Handle selection and add_to_cart in **Block** product listing the classic way.
      // Attach click event listeners to a whole product card, as some links may not have the product_id data attribute.
      document
    -   .querySelectorAll( '.products-block-post-template .product' )
    +   .querySelectorAll(
    +     '.products-block-post-template .product, .wc-block-product-template .product'
    +   )
        ?.forEach( ( productCard ) => {
          // Get the Product ID from a child node containing the relevant attribute
          const productId = productCard
  • Run e2e tests again, all tests passed except:
    6 [chromium] › gtag-events/blocks-pages.test.js:160:2 › GTag events on block pages › Add to cart event is sent from a product collection block shop page
    
    Which is related to the code change from Fix - Selectors in Product Blocks #378 (comment). Not sure if I change the code locally wrong?

I saw @tomalec started reviewing this PR as well I would leave the rest of the review to him for now. I would review the rest tomorrow if needed.

@mikkamp
Copy link
Contributor Author

mikkamp commented Mar 11, 2024

Run e2e tests again, all tests passed except:

[chromium] › gtag-events/blocks-pages.test.js:160:2 › GTag events on block pages › Add to cart event is sent from a product collection block shop page

Which is related to the code change from #378 (comment). Not sure if I change the code locally wrong?

The code you changed is right, did you rebuild the assets after that change? I'm not quite sure why it would be failing otherwise. Maybe you can check the site at http://localhost:8889 to see how it handles the "Product Collection Shop" page. Make sure to add some test settings to enable tracking, during E2E that's enabled and cleared.

const data = getEventData( request, 'add_to_cart' );
expect( data.product1 ).toEqual( {
id: simpleProductID.toString(),
nm: 'Simple product',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💅 Maybe instead of hardcoding its name, we could read it from config.products.simple.name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💅 Also, we could cache simpleProductID.toString() and simpleProductPrice.toString()in beforeAll

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but also disagree here. I personally don't like to abstract too much information in the expect calls. I want to see clearly what data I'm expecting without having to look it up what it should match too. It's true that if they are constants then there is no harm as they can't be modified, but this is more personal preference. Ofcourse I'm breaking my own by not using a constant for simpleProductPrice, but that's mainly because I copied that logic from GLA.

For the same reason I don't often make a lot of the expect parts in tests "DRY", which could easily be done in this case since a lot of expect similar data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can relate to being explicit in expects, so I'm fine leaving it as simpleProduct*.toString().

But for the same reason, I'd use config.products.simple.name as this is what we expect from the perspective of this file. We didn't set Simple product here. So we expect it to be "whatever is set in config"

Comment on lines 52 to 54
if ( ! ( await pageExistsByTitle( title ) ) ) {
return await createPage( title, content );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not returning the ID if the page exists.

Suggested change
if ( ! ( await pageExistsByTitle( title ) ) ) {
return await createPage( title, content );
}
return (
( await pageExistsByTitle( title ) ) ||
( await createPage( title, content ) )
);

Copy link
Member

@tomalec tomalec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the code, run locally, LGTM
Left a few cosmetic comments.

Thank you very much! :) Having E2Es in place could boost the speed and confidence in our development :)

@@ -0,0 +1 @@
{"title":"Product Collection Block Shop","pageContent":"<!-- wp:woocommerce/product-collection {\"id\":\"6ec42bd9-442e-43ac-85f9-75b234103549\",\"queryId\":0,\"query\":{\"perPage\":9,\"pages\":0,\"offset\":0,\"postType\":\"product\",\"order\":\"desc\",\"orderBy\":\"date\",\"search\":\"\",\"exclude\":[],\"inherit\":false,\"taxQuery\":{},\"isProductCollectionBlock\":true,\"featured\":false,\"woocommerceOnSale\":false,\"woocommerceStockStatus\":[\"instock\",\"outofstock\",\"onbackorder\"],\"woocommerceAttributes\":[],\"woocommerceHandPickedProducts\":[]},\"tagName\":\"div\",\"displayLayout\":{\"type\":\"flex\",\"columns\":3,\"shrinkColumns\":true}} -->\n<div class=\"wp-block-woocommerce-product-collection\"><!-- wp:woocommerce/product-template -->\n<!-- wp:woocommerce/product-image {\"imageSizing\":\"thumbnail\",\"isDescendentOfQueryLoop\":true} /-->\n\n<!-- wp:post-title {\"textAlign\":\"center\",\"level\":3,\"isLink\":true,\"style\":{\"spacing\":{\"margin\":{\"bottom\":\"0.75rem\",\"top\":\"0\"}}},\"fontSize\":\"medium\",\"__woocommerceNamespace\":\"woocommerce/product-collection/product-title\"} /-->\n\n<!-- wp:woocommerce/product-price {\"isDescendentOfQueryLoop\":true,\"textAlign\":\"center\",\"fontSize\":\"small\"} /-->\n\n<!-- wp:woocommerce/product-button {\"textAlign\":\"center\",\"isDescendentOfQueryLoop\":true,\"fontSize\":\"small\"} /-->\n<!-- /wp:woocommerce/product-template -->\n\n<!-- wp:query-pagination {\"layout\":{\"type\":\"flex\",\"justifyContent\":\"center\"}} -->\n<!-- wp:query-pagination-previous /-->\n\n<!-- wp:query-pagination-numbers /-->\n\n<!-- wp:query-pagination-next /-->\n<!-- /wp:query-pagination -->\n\n<!-- wp:woocommerce/product-collection-no-results -->\n<!-- wp:group {\"layout\":{\"type\":\"flex\",\"orientation\":\"vertical\",\"justifyContent\":\"center\",\"flexWrap\":\"wrap\"}} -->\n<div class=\"wp-block-group\"><!-- wp:paragraph {\"fontSize\":\"medium\"} -->\n<p class=\"has-medium-font-size\"><strong>No results found</strong></p>\n<!-- /wp:paragraph -->\n\n<!-- wp:paragraph -->\n<p>You can try <a href=\"#\" class=\"wc-link-clear-any-filters\">clearing any filters</a> or head to our <a href=\"#\" class=\"wc-link-stores-home\">store's home</a></p>\n<!-- /wp:paragraph --></div>\n<!-- /wp:group -->\n<!-- /wp:woocommerce/product-collection-no-results --></div>\n<!-- /wp:woocommerce/product-collection -->"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ 💅

Why do we use /__fixtures__/ insted of /fixtures/? Do we autogenerate them somehow? If not, maybe we could format the content to be more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific reasoning, I manually generated these.
Copied directly from GLA, where we initially copied it from the WC Blocks E2E tests. So I'm not tied to a particular method, I just needed something up and running.

@mikkamp
Copy link
Contributor Author

mikkamp commented Mar 11, 2024

@tomalec Thanks for the review. Since this is approved and working, I'm going to merge it so we can focus on the other issues and get them tested. We can always refactor / change any other details later if needed.

@mikkamp mikkamp merged commit 128ef39 into trunk Mar 11, 2024
6 checks passed
@mikkamp mikkamp deleted the dev/add-e2e-tests branch March 11, 2024 16:58
@mikkamp mikkamp mentioned this pull request Mar 12, 2024
11 tasks
@tomalec tomalec mentioned this pull request Mar 12, 2024
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: dev Developer-facing only change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants