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

Update dispute challenge/accept e2e tests to match changes to transaction details and order edit screens #7448

Merged
merged 17 commits into from
Oct 12, 2023
Merged

Update dispute challenge/accept e2e tests to match changes to transaction details and order edit screens #7448

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3981b96
Remove unused merchantWCP method
Jinksi Oct 10, 2023
f3fef3b
Add placeholder changelog entry
Jinksi Oct 10, 2023
250e77e
Add data-testid attrs to dispute buttons for e2e test targets
Jinksi Oct 11, 2023
3efa7ee
Use data-testid attrs for open/accept dispute flow utils
Jinksi Oct 11, 2023
5f57879
Update merchant dispute winning challenge e2e test
Jinksi Oct 11, 2023
bec6683
Let e2e test page wait for accept dispute modal to load
Jinksi Oct 11, 2023
448b8a0
Update merchant dispute accept e2e test
Jinksi Oct 11, 2023
f855197
Update merchant save dispute challenge e2e test
Jinksi Oct 11, 2023
0090cf2
Revert unintended change
Jinksi Oct 11, 2023
e209cff
Use disputed order notice to navigate to transaction details
Jinksi Oct 11, 2023
9f3bb59
Move `openChallengeDispute` logic inline to simplify code
Jinksi Oct 11, 2023
d04810d
Move `openAcceptDispute` logic inline to simplify code
Jinksi Oct 11, 2023
c878253
Merge branch 'develop' into fix/7377-dispute-e2e-tests
shendy-a8c Oct 11, 2023
104b482
Merge branch 'develop' into fix/7377-dispute-e2e-tests
haszari Oct 12, 2023
3e9f30f
Rename file `disputes-save-for-editing` to `disputes-save-draft-chall…
Jinksi Oct 12, 2023
876468e
Update save-draft-challenge test names
Jinksi Oct 12, 2023
72c7467
Merge branch 'develop' into fix/7377-dispute-e2e-tests
Jinksi Oct 12, 2023
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
5 changes: 5 additions & 0 deletions changelog/fix-7377-dispute-e2e-tests
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: dev
Comment: Not user-facing: update e2e tests for dispute challenge/accept flows to match new Transaction Dispute Details UI


3 changes: 3 additions & 0 deletions client/payment-details/dispute-details/dispute-awaiting-response-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
>
<Button
variant="primary"
data-testid="challenge-dispute-button"
disabled={ isLoading }
onClick={ () => {
wcpayTracks.recordEvent(
Expand All @@ -275,6 +276,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
<Button
variant="tertiary"
disabled={ isLoading }
data-testid="open-accept-dispute-modal-button"
onClick={ () => {
wcpayTracks.recordEvent(
disputeAcceptAction.acceptButtonTracksEvent,
Expand Down Expand Up @@ -333,6 +335,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
</Button>
<Button
variant="primary"
data-testid="accept-dispute-button"
onClick={ () => {
wcpayTracks.recordEvent(
disputeAcceptAction.modalButtonTracksEvent,
Expand Down
37 changes: 20 additions & 17 deletions tests/e2e/specs/wcpay/merchant/merchant-disputes-save-for-editing.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,17 @@
* External dependencies
*/
import config from 'config';
const { merchant, shopper, evalAndClick } = require( '@woocommerce/e2e-utils' );

/**
* Internal dependencies
*/
import { fillCardDetails, setupProductCheckout } from '../../../utils/payments';

import { merchantWCP } from '../../../utils';

const { merchant, shopper } = require( '@woocommerce/e2e-utils' );
import { uiLoaded } from '../../../utils';

let orderId;

describe.skip( 'Disputes > Save dispute for editing', () => {
describe( 'Disputes > Save dispute for editing', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming of the suites, and the tests is a good place to take care and be really clear.

I think this test is focused on verifying that merchants can save and resume their response to dispute. Is that correct?

If so, something like this might be clearer: Disputes > Merchant can save and resume draft dispute challenge

These suite names, and the individual tests, are likely to align very closely to flows. So we might already have a list of names we can use.

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 naming of the suites, and the tests is a good place to take care and be really clear.

💯

I've updated the filename from merchant-disputes-save-for-editing.spec.js → merchant-disputes-save-draft-challenge.spec.js in 3e9f30f.

I've also updated the test names within this file in 876468e.

beforeAll( async () => {
await page.goto( config.get( 'url' ), { waitUntil: 'networkidle0' } );

Expand Down Expand Up @@ -42,23 +41,25 @@ describe.skip( 'Disputes > Save dispute for editing', () => {
} );

it( 'should show a dispute in payment details', async () => {
// Pull out and follow the link to avoid working in multiple tabs
const paymentDetailsLink = await page.$eval(
'p.order_number > a',
( anchor ) => anchor.getAttribute( 'href' )
);

await merchantWCP.openPaymentDetails( paymentDetailsLink );
// Click the order dispute notice.
await expect( page ).toClick( '[type="button"]', {
text: 'Respond now',
} );
await page.waitForNavigation( {
waitUntil: 'networkidle0',
} );

// Verify we have a dispute for this purchase
await expect( page ).toMatchElement( 'li.woocommerce-timeline-item', {
text: 'Payment disputed as Product not received.',
// Verify we see the dispute details on the transaction details page.
await expect( page ).toMatchElement( '.dispute-notice', {
text: 'The cardholder claims the product was not received',
} );
} );

it( 'should be able to save dispute for editing', async () => {
// Click to challenge the dispute
await merchantWCP.openChallengeDispute();
// Click the challenge dispute button.
await evalAndClick( '[data-testid="challenge-dispute-button"]' );
await page.waitForNavigation( { waitUntil: 'networkidle0' } );
await uiLoaded();

await page.waitForSelector(
'div.wcpay-dispute-evidence .components-flex.components-card__header',
Expand Down Expand Up @@ -105,6 +106,8 @@ describe.skip( 'Disputes > Save dispute for editing', () => {
// Reload the page
await page.reload();

await uiLoaded();
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was flaky in my local environment: adding this seemed to make the test more robust.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds important and impactful 😁 … if the UI isn't loaded and we start expecting stuff, we're in trouble.


// Verify the previously selected Product type was saved
await expect( page ).toMatchElement(
'[data-testid="dispute-challenge-product-type-selector"]',
Expand Down
68 changes: 33 additions & 35 deletions tests/e2e/specs/wcpay/merchant/merchant-disputes-submit-losing.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,17 @@
* External dependencies
*/
import config from 'config';
const { merchant, shopper, evalAndClick } = require( '@woocommerce/e2e-utils' );

/**
* Internal dependencies
*/
import { merchantWCP } from '../../../utils';
import { uiLoaded } from '../../../utils';
import { fillCardDetails, setupProductCheckout } from '../../../utils/payments';

const { merchant, shopper } = require( '@woocommerce/e2e-utils' );

let orderId;

describe.skip( 'Disputes > Submit losing dispute', () => {
describe( 'Disputes > Submit losing dispute', () => {
beforeAll( async () => {
await page.goto( config.get( 'url' ), { waitUntil: 'networkidle0' } );

Expand Down Expand Up @@ -40,46 +40,44 @@ describe.skip( 'Disputes > Submit losing dispute', () => {
} );

it( 'should process and confirm a losing dispute', async () => {
// Pull out and follow the link to avoid working in multiple tabs
const paymentDetailsLink = await page.$eval(
'p.order_number > a',
( anchor ) => anchor.getAttribute( 'href' )
);

await merchantWCP.openPaymentDetails( paymentDetailsLink );
// Click the order dispute notice.
await expect( page ).toClick( '[type="button"]', {
text: 'Respond now',
} );
await page.waitForNavigation( {
waitUntil: 'networkidle0',
} );

// Verify we have a dispute for this purchase
await expect( page ).toMatchElement( 'li.woocommerce-timeline-item', {
text: 'Payment disputed as Product not received.',
// Verify we see the dispute details on the transaction details page.
await expect( page ).toMatchElement( '.dispute-notice', {
text: 'The cardholder claims the product was not received',
} );

// Accept the dispute
await merchantWCP.openAcceptDispute();
// Open the accept dispute modal.
await evalAndClick( '[data-testid="open-accept-dispute-modal-button"' );
await uiLoaded();
// Click the accept dispute button.
await evalAndClick( '[data-testid="accept-dispute-button"]' );
// Wait for the accept POST request to resolve and the status chip to update with the new status.
await expect( page ).toMatchElement( '.chip', {
text: 'Disputed: Lost',
timeout: 10000,
} );

// If webhooks are not received, the dispute status won't be updated in the dispute list page resulting in test failure.
// Workaround - Open payment details page again and check dispute's status.
await merchantWCP.openPaymentDetails( paymentDetailsLink );
// Check the dispute details footer
await expect( page ).toMatchElement(
'.transaction-details-dispute-footer *',
{
text: 'This dispute was accepted and lost',
}
);

// Confirm buttons are not present anymore since a dispute has been accepted.
await expect( page ).not.toMatchElement(
// eslint-disable-next-line max-len
'div.transaction-details-dispute-details-body div.transaction-details-dispute-details-body__actions button.components-button.is-primary',
{
text: 'Challenge dispute',
}
'[data-testid="challenge-dispute-button"]'
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀 These testids are a great pattern. Is this commonplace, or can we encourage wider use?

Copy link
Member Author

Choose a reason for hiding this comment

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

data-testid is a common pattern from react-testing-library, although it is the least preferred method of querying an element (user-facing methods are preferred).

In the spirit of the guiding principles, it is recommended to use this (data-testid) only after the other queries don't work for your use case.
...
That said, they are way better than querying based on DOM structure or styling css class names.

The WooCommerce guide Best Practices for Writing and Running End-to-End Tests recommends a similar approach:

Include your own automation IDs
To avoid maintaining selectors in the future, it’s always a good idea to include your own data selector, such as test-automation-id
...
instead of generally searching for visible elements on the page and using their IDs, classes, and XPaths.

);
await expect( page ).not.toMatchElement(
// eslint-disable-next-line max-len
'div.transaction-details-dispute-details-body div.transaction-details-dispute-details-body__actions button.components-button.is-tertiary',
{
text: 'Accept dispute',
}
'[data-testid="open-accept-dispute-modal-button"]'
);

// Confirm dispute status is Lost.
await page.waitForSelector( 'li.woocommerce-timeline-item' );
await expect( page ).toMatchElement( 'li.woocommerce-timeline-item', {
text: 'Dispute lost. The bank ruled in favor of your customer.',
} );
} );
} );
23 changes: 16 additions & 7 deletions tests/e2e/specs/wcpay/merchant/merchant-disputes-submit-winning.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const {

let orderId;

describe.skip( 'Disputes > Submit winning dispute', () => {
describe( 'Disputes > Submit winning dispute', () => {
beforeAll( async () => {
await page.goto( config.get( 'url' ), { waitUntil: 'networkidle0' } );

Expand Down Expand Up @@ -50,15 +50,24 @@ describe.skip( 'Disputes > Submit winning dispute', () => {
'p.order_number > a',
( anchor ) => anchor.getAttribute( 'href' )
);
await merchantWCP.openPaymentDetails( paymentDetailsLink );

// Verify we have a dispute for this purchase
await expect( page ).toMatchElement( 'li.woocommerce-timeline-item', {
text: 'Payment disputed as Transaction unauthorized.',
// Click the order dispute notice.
await expect( page ).toClick( '[type="button"]', {
text: 'Respond now',
} );
await page.waitForNavigation( {
waitUntil: 'networkidle0',
} );

// Verify we see the dispute details on the transaction details page.
await expect( page ).toMatchElement( '.dispute-notice', {
text: 'The cardholder claims this is an unauthorized transaction',
} );

// Challenge the dispute
await merchantWCP.openChallengeDispute();
// Click the challenge dispute button.
Copy link
Contributor

Choose a reason for hiding this comment

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

Clear names and comments in these e2e tests might help make them stable and easier to maintain (as we iterate on the software and flows).

For this example, I think this is a setup step (not a test expectation), and it does quite a few things.

Would this comment accurately reflect the intention?

// Simulate merchant challenging the dispute.
// Wait for the dispute challenge page to load and render.

This also explicitly states the outcome (expected state). I think that's useful because the actual test expectations apply to that page.

await evalAndClick( '[data-testid="challenge-dispute-button"]' );
await page.waitForNavigation( { waitUntil: 'networkidle0' } );
await uiLoaded();

// Select product type
await expect( page ).toSelect(
Expand Down
39 changes: 0 additions & 39 deletions tests/e2e/utils/flows.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
const {
merchant,
shopper,
evalAndClick,
uiUnblocked,
clearAndFillInput,
setCheckbox,
Expand All @@ -26,7 +25,6 @@ import { uiLoaded } from './helpers';

const SHOP_MY_ACCOUNT_PAGE = baseUrl + 'my-account/';
const MY_ACCOUNT_PAYMENT_METHODS = baseUrl + 'my-account/payment-methods';
const WC_ADMIN_BASE_URL = baseUrl + 'wp-admin/';
const MY_ACCOUNT_SUBSCRIPTIONS = baseUrl + 'my-account/subscriptions';
const MY_ACCOUNT_EDIT = baseUrl + 'my-account/edit-account';
const MY_ACCOUNT_ORDERS = SHOP_MY_ACCOUNT_PAGE + 'orders/';
Expand Down Expand Up @@ -589,43 +587,6 @@ export const merchantWCP = {
} );
},

openDisputeDetails: async ( disputeDetailsLink ) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This helper function is no longer relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

… because the dispute details screen is goneburgers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. I mentioned it part of the #7363 tasks.

await Promise.all( [
page.goto( WC_ADMIN_BASE_URL + disputeDetailsLink, {
waitUntil: 'networkidle0',
} ),
uiLoaded(),
] );
await uiLoaded();
},

openChallengeDispute: async () => {
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've removed this helper function and moved the logic inline to simplify the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this – it's not just simplification. You are removing a problematic abstraction and making each test more self-contained (decoupled).

await Promise.all( [
evalAndClick(
// eslint-disable-next-line max-len
'div.transaction-details-dispute-details-body div.transaction-details-dispute-details-body__actions button.components-button.is-primary'
),
page.waitForNavigation( { waitUntil: 'networkidle0' } ),
uiLoaded(),
] );
},

openAcceptDispute: async () => {
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've removed this helper function and moved the logic inline to simplify the code – this was only used in one test.

await Promise.all( [

This comment was marked as outdated.

Sign in to view

page.removeAllListeners( 'dialog' ),
evalAndClick(
// eslint-disable-next-line max-len
'div.transaction-details-dispute-details-body div.transaction-details-dispute-details-body__actions button.components-button.is-tertiary'
),
evalAndClick(
// eslint-disable-next-line max-len
'.transaction-details-dispute-accept-modal__actions button.components-button.is-primary'
),
page.waitForNavigation( { waitUntil: 'networkidle0' } ),
uiLoaded(),
] );
},

openPaymentDetails: async ( paymentDetailsLink ) => {
await Promise.all( [
page.goto( paymentDetailsLink, {
Expand Down
Loading