Testing product sets - @W-12414116@ #1003

vmarta · 2023-02-24T18:23:34Z

We'd like to add tests to verify the functionality of product sets in our retail-react-app.

Types of Changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Breaking change (could cause existing functionality to not work as expected)
Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

Removing a public function or component or prop

Adding a required argument to a function

Changing the data type of a function parameter or return value

Adding a new peer dependency to package.json

Changes

(change1)

How to Test-Drive This PR

(step1)

Checklists

General

Changes are covered by test cases
CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

There are no changes to UI

or...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

Changes include a UI text update in the Retail React App (which requires translation)

vmarta · 2023-02-24T19:58:16Z

packages/template-retail-react-app/app/components/product-view-modal/index.jsx

-        <Modal data-testid={'sf-product-view-modal'} size="4xl" isOpen={isOpen} onClose={onClose}>
+        <Modal size="4xl" isOpen={isOpen} onClose={onClose}>
            <ModalOverlay />
-            <ModalContent>
+            <ModalContent containerProps={{'data-testid': 'product-view-modal'}}>


Unlike other Chakra components, Modal does not accept any props you pass in. So to set the test id, using containerProps like this is one way to do it.

vmarta · 2023-02-24T19:59:38Z

packages/template-retail-react-app/app/hooks/use-add-to-cart-modal.js

@@ -62,13 +62,18 @@ export const AddToCartModal = () => {
    const intl = useIntl()
    const basket = useBasket()
    const size = useBreakpointValue({base: 'full', lg: '2xl', xl: '4xl'})
-    const {currency, productItems, productSubTotal, itemAccumulatedCount} = basket
+    const {currency, productItems, productSubTotal} = basket


itemAccumulatedCount is a duplicate, when we're calculating totalQuantity in the line below. Removing the duplicate is also aligned with what's happening in the hooks-integration branch.

vmarta · 2023-02-24T20:03:39Z

packages/template-retail-react-app/app/pages/product-detail/index.jsx

-                behavior: 'smooth',
-                block: 'end'
-            })
+            if (ref.scrollIntoView) {


Added this check because during testing, scrollIntoView is not available function.

vmarta · 2023-02-24T20:05:54Z

packages/template-retail-react-app/app/pages/product-detail/index.test.js

+    )
+}
+
+const basketWithProductSet = {


Mocking this data inside this test file. I'm not sure what our approach is. Sometimes the mocked data is in separate file, sometimes not. I guess it depends on whether the data is going to be reused?

I agree, and I would add one more consideration: sometimes I separate the mock data just because it obscures the test and makes it harder to think about if the mock data is too long.

I would vote to move this out to a <test-name>.mock.js file and have it live in the same directory (similar naming convention keeps them adjacent in your IDE) just because it distracts from the test

vmarta · 2023-02-24T20:08:54Z

packages/template-retail-react-app/app/pages/product-detail/index.test.js

+    test('render multi-product layout', async () => {
+        renderWithProviders(<MockedPageWithProductSet />)
+
+        await waitFor(() => {


Technically, I don't need to wait here. But I'm foreseeing the time when the hooks-integration work is merged. By then, the page-level component would use hooks to fetch data. And this asynchronous behaviour would require calling waitFor.

I'm thinking most things should await / waitFor for that reason. Not many of our components are "dumb" enough to not need the pattern

vmarta · 2023-02-24T20:11:37Z

packages/template-retail-react-app/app/utils/test-utils.js

@@ -142,6 +161,7 @@ export const TestProviders = ({
                                                    value={addToCartModal}
                                                >
                                                    {children}
+                                                    <AddToCartModal />


Our TestProviders were missing this modal. We had the context in there (line 160), but not the modal to render yet.

bfeister · 2023-02-24T20:24:26Z

packages/template-retail-react-app/app/commerce-api/mock-data.js

@@ -2985,6 +2985,1625 @@ export const mockedCustomerProductListsDetails = {
    total: 1
 }

+// This wishlist has more varied products
+export const mockedCustomerProductListsDetails2 = {


What happened to the generator function we were working on when we paired on this? Can you call out the specific bits that are different about this mock vs the other?

I've moved this data into a separate file ("something.mock.js" format) that is in a more specific location than the "app/commerce-api/mock-data.js". Because the mocked data is intended to be used only for a particular test.

@vmarta I think my main question got missed here. I realize I didn't really specify this, but I'm mainly interested in "how big is the difference between the existing mock data and this new mock data". If they're only 30% similar, I can understand not using the mock generator function pattern. If they're 90% similar, I would expect we want to use the generator pattern.

@bfeister oh sorry, the new mocked data is completely different in this case.

bfeister · 2023-02-24T20:28:51Z

packages/template-retail-react-app/app/hooks/use-add-to-cart-modal.test.js

@@ -560,18 +561,24 @@ const MOCK_PRODUCT = {
    c_width: 'Z'
 }

-test('Renders AddToCartModal', () => {
+test('Renders AddToCartModal with multiple products', () => {


I tend to make all tests use at least some amount of await / async / waitFor so I would make this an async function

bfeister · 2023-02-24T20:29:10Z

packages/template-retail-react-app/app/hooks/use-add-to-cart-modal.test.js

            }
        ]
    }
-    const {getByText} = renderWithProviders(
+
+    renderWithProviders(


I would await renderWithProviders(

Our renderWithProviders, which would call render from the testing library, is not an async method. It returns an object. So calling await here would not do anything.

bfeister

Looks good. I think we could do some cleanup around items I mentioned, but no "blockers". I can approve if urgent / time is short

bfeister · 2023-02-24T20:30:24Z

packages/template-retail-react-app/app/hooks/use-add-to-cart-modal.test.js

+    expect(screen.getAllByText(MOCK_PRODUCT.name)[0]).toBeInTheDocument()
+    expect(screen.getByRole('dialog', {name: /23 items added to cart/i})).toBeInTheDocument()
+
+    const numOfRowsRendered = screen.getAllByTestId('product-added').length
+    expect(numOfRowsRendered).toEqual(MOCK_DATA.itemsAdded.length)


I remember you saying something about "awaiting too many things" which I'm still a bit confused about. If you want all of the expect conditions to be true at the same time (usually true, I believe) then I would wrap this in waitFor(() => {})

I think I won't use the waitFor in this case (in the product-sets feature branch). Maybe it becomes necessary within the hooks-integration work.

After the component is rendered, I expect all those expect conditions to be true.. all at the same time, yup. Because one render of the component is enough. It won't be re-rendered by some data fetching or async operation.

bfeister · 2023-02-24T20:31:44Z

...emplate-retail-react-app/app/pages/account/wishlist/partials/wishlist-primary-action.test.js

    })
 })
+
+test('the View Full Details button', () => {


I realize it's unlikely to matter for such a tiny component, but I might still use async here and waitFor just to make sure the tests don't behave differently or fail in CI / different OS runtimes

bfeister · 2023-02-24T20:32:21Z

...emplate-retail-react-app/app/pages/account/wishlist/partials/wishlist-primary-action.test.js

+            const modal = screen.getByTestId('product-view-modal')
+            expect(modal).toBeVisible()
+        },
+        // Seems like rendering the modal takes a bit more time


Awesome find! 🎉

bfeister · 2023-02-24T20:34:41Z

packages/template-retail-react-app/app/pages/product-detail/index.test.js

+    )
+}
+
+const basketWithProductSet = {


I agree, and I would add one more consideration: sometimes I separate the mock data just because it obscures the test and makes it harder to think about if the mock data is too long.

I would vote to move this out to a <test-name>.mock.js file and have it live in the same directory (similar naming convention keeps them adjacent in your IDE) just because it distracts from the test

bfeister · 2023-02-24T20:35:18Z

packages/template-retail-react-app/app/pages/product-detail/index.test.js

+    test('render multi-product layout', async () => {
+        renderWithProviders(<MockedPageWithProductSet />)
+
+        await waitFor(() => {


I'm thinking most things should await / waitFor for that reason. Not many of our components are "dumb" enough to not need the pattern

bfeister · 2023-02-24T20:36:30Z

packages/template-retail-react-app/app/pages/product-detail/index.test.js

+        global.server.use(
+            rest.post('*/baskets/:basketId/items', (req, res, ctx) => {
+                return res(ctx.json(basketWithProductSet))
+            })
+        )


@yunakim714 did we ever decide whether this pattern (doing setup outside of a beforeEach) can cause race conditions. Seems like this should be in beforeEach to be sure we avoid race conditions

We can scope this server.use in a describe block to avoid it being used by other tests

describe('add to cart test', () => { beforeEach(() => { // this will apply to the tests inside this describe block global.server.use() }) test('something', () => {}) })

Not related to product sets, but needing CI to pass.

* Product sets: basic PLP (#878) * Show product sets on PLP (alongside other products) For now, the product sets would look the same as any other kinds of products. * Parse the product type * Add todo * Ignore linting of this line for now * Add "Starting at" label * Product sets: basic pdp (@W-12301851@) (#897) * Product set: render the children products with minimal UI * No longer showing breadcrumbs in the child items * Individual swatches now have correct `value` There was a subtle bug where the `value` of the SwatchGroup was passed down to override all the values of the children Swatch components. * 1st attempt at saving swatches state in the page url * Avoid null in the url search params * Some refactoring * Add-to-cart now works for child items * Show accordion for each child item * Add todo * Rename variable for accuracy The child product itself is actually not a set, but is considered a set product. * Optional argument * Some refactoring and fix linting * Helper hook for url search params * Clean up * Rename module for a specific use case (PDP) * Show/hide add-to-cart buttons accordingly * Modal shows product set data * Parent product * Variant's product image shows up in add-to-cart modal * Rename variables for clarity * Fix linting error * Add horizontal rules to separate the parent and child items * Tweak whitespace * Localize the Starting At label * Add todo * Product sets: wishlist (@W-12301966@) (#917) * Starting At label * 1st attempt at updating wishlist for product sets * "Select Options" → "View Options" * Some refactoring * Render add-all-to-cart button if necessary * Better way to fetch `setProducts` data * Switch to zzrf-009 temporarily * For debugging purpose I'll revert this commit later * Simpler requirement * Update what quantity meant for product set * Some refactoring * Add comment * Add-to-wishlist for the individual child items * Rename variable * Update button text on PDP * Update button text on Wishlist page * Revert "Switch to zzrf-009 temporarily" This reverts commit 484c8d8. * PDP: show wishlist button on mobile * Fix formatting issues (prettier) * PR feedback * Clearer as to which is product or variant * Make it consistent with addToWishlist signature * [Feature] Add `Add Set to Cart` Button for Product Sets (#931) * Product sets: basic PLP (#878) * Variant's product image shows up in add-to-cart modal * Get modal showing multiple items added to cart * Add scroll to after validation Co-authored-by: Vincent Marta <[email protected]> * Einstein API to support product sets (#962) * verify viewcategory and viewsearch is working * viewproduct event is sent for every child product * addtocart event fires for all children when add set to cart * parent set only sends id * Design enhancements for product sets (#975) * remove complete the set carousel * remove complete the set translations * sticky buttons on mobile view * fix modal vertical margins on desktop * move recos to modalbody and cleanup margins * add dividers between product set items in mobile view * fix margins so sticky buttons do not overlap with content * add lazy loading of product set children product images * remove height - does nothing * bump build size to 55 * add conditional for complete the set and change loading var name * lint * revert translations * Cleanup * added to cart modal is scrollable and 90% of viewport * remove dialogprops * lint * Testing product sets - @W-12414116@ (#1003) * Product tile: test product set's price label * Test product set rendering * Test callbacks are called properly * itemAccumulatedCount is redundant * Test that modal can render multiple products * Finally the add-to-cart modal shows up in testing * Clean up * Test error messages * Test lazy loading of the child products * Split a test into smaller ones * Use `describe` to group all the related tests * Update comment * Test the wishlist and its primary action buttons * Clean up * Some refactoring * Some refactoring * Linting fix * Update comment * Test `getDisplayVariationValues` * Test the usePDPSearchParams * Tweak another test to pass Not related to product sets, but needing CI to pass. * Move mocked data into their own files * Moving it to within `beforeEach` --------- Co-authored-by: Ben Chypak <[email protected]> Co-authored-by: yunakim714 <[email protected]>

vmarta added 16 commits February 17, 2023 16:15

Product tile: test product set's price label

7cb7bc3

Test product set rendering

eab9fad

Test callbacks are called properly

7aa5147

itemAccumulatedCount is redundant

b0b94a4

Test that modal can render multiple products

ee66061

Finally the add-to-cart modal shows up in testing

d1e15d2

Clean up

aab2990

Test error messages

40a45fa

Test lazy loading of the child products

c649c19

Split a test into smaller ones

5496607

Use describe to group all the related tests

f46f6be

Update comment

f89299f

Test the wishlist and its primary action buttons

560825f

Clean up

0fd3c3f

Some refactoring

5f8d763

Some refactoring

de72da8

vmarta commented Feb 24, 2023

View reviewed changes

bfeister reviewed Feb 24, 2023

View reviewed changes

vmarta added 7 commits February 24, 2023 13:06

Linting fix

c8b26a0

Update comment

169e5bd

Test getDisplayVariationValues

ea2a73a

Test the usePDPSearchParams

ee599f4

Tweak another test to pass

6fd0759

Not related to product sets, but needing CI to pass.

Move mocked data into their own files

429a269

Moving it to within beforeEach

bfd4a79

vmarta marked this pull request as ready for review February 27, 2023 22:21

vmarta requested a review from a team as a code owner February 27, 2023 22:21

bfeister approved these changes Feb 27, 2023

View reviewed changes

vmarta merged commit 56771cb into feature/product-sets Feb 27, 2023

vmarta deleted the test-product-sets branch February 27, 2023 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing product sets - @W-12414116@ #1003

Testing product sets - @W-12414116@ #1003

vmarta commented Feb 24, 2023 •

edited

Loading

vmarta Feb 24, 2023

vmarta Feb 24, 2023

vmarta Feb 24, 2023

vmarta Feb 24, 2023

bfeister Feb 24, 2023

vmarta Feb 24, 2023

bfeister Feb 24, 2023

vmarta Feb 24, 2023

bfeister Feb 24, 2023

vmarta Feb 27, 2023

bfeister Feb 27, 2023

vmarta Feb 27, 2023

bfeister Feb 24, 2023

bfeister Feb 24, 2023

vmarta Feb 27, 2023

bfeister left a comment

bfeister Feb 24, 2023

vmarta Feb 27, 2023

bfeister Feb 24, 2023

bfeister Feb 24, 2023

bfeister Feb 24, 2023

bfeister Feb 24, 2023

bfeister Feb 24, 2023

alexvuong Feb 24, 2023

Testing product sets - @W-12414116@ #1003

Testing product sets - @W-12414116@ #1003

Conversation

vmarta commented Feb 24, 2023 • edited Loading

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarta commented Feb 24, 2023 •

edited

Loading