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.

Sign up for GitHub

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

Ensure that each <CheckboxControl> component has a unique ID #45655

Merged
merged 4 commits into from
Mar 29, 2024
Merged

Ensure that each <CheckboxControl> component has a unique ID #45655

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed144e9
Ensure that each <CheckboxControl> component has a unique id
nielslange Mar 27, 2024
f9dc91b
Add changefile(s) from automation for the following project(s): wooco…
invalid-email-address Mar 27, 2024
422637e
Optimise setup and teardown settings
nielslange Mar 27, 2024
6c1875b
Wrapping setup and teardown in act()
nielslange Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions plugins/woocommerce-blocks/assets/js/blocks/checkout/test/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
import { render, screen, waitFor, act } from '@testing-library/react';
import { previewCart } from '@woocommerce/resource-previews';
import { dispatch } from '@wordpress/data';
import { CART_STORE_KEY as storeKey } from '@woocommerce/block-data';
import { CART_STORE_KEY, CHECKOUT_STORE_KEY } from '@woocommerce/block-data';
import { default as fetchMock } from 'jest-fetch-mock';
import { allSettings } from '@woocommerce/settings';

/**
* Internal dependencies
Expand Down Expand Up @@ -33,9 +34,7 @@ import Fee from '../inner-blocks/checkout-order-summary-fee/frontend';
import Discount from '../inner-blocks/checkout-order-summary-discount/frontend';
import Shipping from '../inner-blocks/checkout-order-summary-shipping/frontend';
import Taxes from '../inner-blocks/checkout-order-summary-taxes/frontend';

import { defaultCartState } from '../../../data/cart/default-state';

import Checkout from '../block';

jest.mock( '@wordpress/compose', () => ( {
Expand Down Expand Up @@ -94,16 +93,16 @@ describe( 'Testing cart', () => {
return Promise.resolve( '' );
} );
// need to clear the store resolution state between tests.
dispatch( storeKey ).invalidateResolutionForStore();
dispatch( storeKey ).receiveCart( defaultCartState.cartData );
dispatch( CART_STORE_KEY ).invalidateResolutionForStore();
dispatch( CART_STORE_KEY ).receiveCart( defaultCartState.cartData );
} );
} );

afterEach( () => {
fetchMock.resetMocks();
} );

it( 'renders checkout if there are items in the cart', async () => {
it( 'Renders checkout if there are items in the cart', async () => {
render( <CheckoutBlock /> );
await waitFor( () => expect( fetchMock ).toHaveBeenCalled() );

Expand Down Expand Up @@ -164,4 +163,26 @@ describe( 'Testing cart', () => {

expect( fetchMock ).toHaveBeenCalledTimes( 1 );
} );

it( 'Ensures checkbox labels have unique IDs', async () => {
allSettings.checkoutAllowsGuest = true;
allSettings.checkoutAllowsSignup = true;
dispatch( CHECKOUT_STORE_KEY ).__internalSetCustomerId( 0 );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dispatch( CHECKOUT_STORE_KEY ).__internalSetCustomerId( 0 );
allSettings.checkoutData.customer_id = 1;

Using this works as well and reduces the risk of depending on data store and reflect actual use case (of the value coming from the server).

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried allSettings.checkoutData.customer_id = 0;, but this results in the following checkbox label IDs:

[["checkbox-control-6", "checkbox-control-7", "terms-and-conditions"]]

While using dispatch( CHECKOUT_STORE_KEY ).__internalSetCustomerId( 0 );, provides these checkbox label IDs:

[["checkbox-control-6", "checkbox-control-7", "checkbox-control-8", "terms-and-conditions"]]

For allSettings.checkoutData.customer_id = 0;, I'm also seeing this error:

'allSettings.checkoutData' is of type 'unknown'.ts(18046)

I do not see any TS errors for allSettings.checkoutAllowsGuest = true; and allSettings.checkoutAllowsSignup = true;.

Copy link
Member

Choose a reason for hiding this comment

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

Switching to allSettings.checkoutData.customer_id = 1; did pass the test for me, should the test have failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test passed as there are still three checkboxes visible. Setting the customer ID to 0 then shows a fourth checkbox.

Copy link
Member

Choose a reason for hiding this comment

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

You must reset those values to their original values at the end of the your test to not pollute other tests. Each test should leave the global state like it was found.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at

woocommerce/plugins/woocommerce-blocks/tests/js/setup-globals.js

Line 216 in 7d6d2c9

customer_id: 1,

the customer_id is set to 1 by default. Given that allSettings.checkoutData.customer_id = 0; does not lead to the expected result, as one of the checkboxes is not visible then, I'd place the following line at the end of the test, to reset the global state:

dispatch( CHECKOUT_STORE_KEY ).__internalSetCustomerId( 1 );

Would you agree that this is the way forward in this case, @senadir?

Copy link
Member

Choose a reason for hiding this comment

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

Also reset allSettings.checkoutAllowsGuest , allSettings.checkoutAllowsSignup to their default values.

Copy link
Member Author

Choose a reason for hiding this comment

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

For allSettings.checkoutAllowsGuest and allSettings.checkoutAllowsSignup, I'm seeing that they're undefined by default. So, I'd add the following lines at the end of the test:

allSettings.checkoutAllowsGuest = undefined;
allSettings.checkoutAllowsSignup = undefined;


// Render the CheckoutBlock
render( <CheckoutBlock /> );

// Wait for the component to fully load, assuming fetch calls or state updates
await waitFor( () => expect( fetchMock ).toHaveBeenCalled() );

// Query for all checkboxes
const checkboxes = screen.getAllByRole( 'checkbox' );

// Extract IDs from checkboxes
const ids = checkboxes.map( ( checkbox ) => checkbox.id );

// Ensure all IDs are unique
const uniqueIds = new Set( ids );
expect( uniqueIds.size ).toBe( ids.length );
} );
} );
4 changes: 4 additions & 0 deletions plugins/woocommerce/changelog/45655-add-42046-CheckboxControl-e2e-tests
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: dev

Add an e2e test to ensure that each <CheckboxControl> component has a unique ID.
Loading