Update dispute challenge/accept e2e tests to match changes to transaction details and order edit screens #7448
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +45 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
Jinksi
changed the title
| @@ -105,6 +105,8 @@ describe.skip( 'Disputes > Save dispute for editing', () => { | |||
| // Reload the page | |||
| await page.reload(); | |||
|
|
|||
| await uiLoaded(); | |||
There was a problem hiding this comment.
This test was flaky in my local environment: adding this seemed to make the test more robust.
There was a problem hiding this comment.
Sounds important and impactful 😁 … if the UI isn't loaded and we start expecting stuff, we're in trouble.
| }, | ||
|
|
||
| openAcceptDispute: async () => { | ||
| await Promise.all( [ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Jinksi
changed the title
| @@ -589,43 +587,6 @@ export const merchantWCP = { | |||
| } ); | |||
| }, | |||
|
|
|||
| openDisputeDetails: async ( disputeDetailsLink ) => { | |||
There was a problem hiding this comment.
This helper function is no longer relevant.
There was a problem hiding this comment.
… because the dispute details screen is goneburgers?
There was a problem hiding this comment.
Nice. I mentioned it part of the #7363 tasks.
| await uiLoaded(); | ||
| }, | ||
|
|
||
| openChallengeDispute: async () => { |
There was a problem hiding this comment.
I've removed this helper function and moved the logic inline to simplify the code.
There was a problem hiding this comment.
I like this – it's not just simplification. You are removing a problematic abstraction and making each test more self-contained (decoupled).
| ] ); | ||
| }, | ||
|
|
||
| openAcceptDispute: async () => { |
There was a problem hiding this comment.
I've removed this helper function and moved the logic inline to simplify the code – this was only used in one test.
|
In the GH check, the e2e tests modified in this PR are passing, yet a subsequent test fails. I'll have to investigate to see if a cleanup action needs to be taken after the dispute tests. |
|
The failing |
Good idea, thanks @haszari. I've added the following to the PR description: These e2e test suites were skipped in #7088 and now have been updated and re-enabled:
|
There was a problem hiding this comment.
I've reviewed the code and don't see any blockers. Note I didn't run tests locally, but I'm assuming they run now 😁 . I checked the last run and I see an unrelated test is failing (merchant-orders-status-change.spec.js). Does that mean our test succeeded (concurrent/independent tests), or is it blocked (bail on first fail)?
I haven't spent a lot of time on our e2e suite so took this PR as an opportunity to dive in and understand. I have some thoughts/feedback for tests in general, none is blocking for this PR (and most is really unrelated). Mentioning here in the spirit of improving things as we go – if you think any of my suggestions are worth handling in this PR, go for it!
- 🙌
testidapproach is much more explicit and robust. Should we encourage wider use? Principle: prefer explicit id over implicit or structural matchers. - (Nitpick) Tests often use relative ancestor imports paths. Principle: avoid relative imports, so modules can be moved easily (to a different place in the tree).
- Test suites and individual tests should be named clearly, based on merchant intent. Principle: test what the user needs to do. Our flows list and our test suite would ideally be very similar.
- Test scripts and expectations should be documented with comments. These comments have two benefits:
- Describe at a high level the programmer/tester intention. E.g.
Simulate merchant clicking dispute notice on order page. - Explicitly document implicit states that will be tested or are required (part of path to test). E.g.
Wait for transaction detail page to load and render. [implied in these tests – theexpect()s typically confirm something on the transaction details screen]
- Describe at a high level the programmer/tester intention. E.g.
| @@ -105,6 +105,8 @@ describe.skip( 'Disputes > Save dispute for editing', () => { | |||
| // Reload the page | |||
| await page.reload(); | |||
|
|
|||
| await uiLoaded(); | |||
There was a problem hiding this comment.
Sounds important and impactful 😁 … if the UI isn't loaded and we start expecting stuff, we're in trouble.
| { | ||
| text: 'Challenge dispute', | ||
| } | ||
| '[data-testid="challenge-dispute-button"]' |
There was a problem hiding this comment.
🚀 These testids are a great pattern. Is this commonplace, or can we encourage wider use?
There was a problem hiding this comment.
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 astest-automation-id
...
instead of generally searching for visible elements on the page and using their IDs, classes, and XPaths.
| @@ -589,43 +587,6 @@ export const merchantWCP = { | |||
| } ); | |||
| }, | |||
|
|
|||
| openDisputeDetails: async ( disputeDetailsLink ) => { | |||
There was a problem hiding this comment.
… because the dispute details screen is goneburgers?
| await uiLoaded(); | ||
| }, | ||
|
|
||
| openChallengeDispute: async () => { |
There was a problem hiding this comment.
I like this – it's not just simplification. You are removing a problematic abstraction and making each test more self-contained (decoupled).
| } ); | ||
|
|
||
| // Challenge the dispute | ||
| await merchantWCP.openChallengeDispute(); | ||
| // Click the challenge dispute button. |
There was a problem hiding this comment.
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.
|
|
||
| let orderId; | ||
|
|
||
| describe.skip( 'Disputes > Save dispute for editing', () => { | ||
| describe( 'Disputes > Save dispute for editing', () => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
|
I'll merge this now with some next steps as per our discussion:
|
On this, it looks like our test scripts aren't getting the same linter checks as our app code (e.g. comments should be sentences). Would be nice if they were consistent. |
@haszari a quick test of this in VSCode suggests that this only applies to PHP comments, not JS. This requires some investigation. |
Thanks for checking! I must be confusing our PHP and JS standards again 😵💫 |
Fixes #7377
Fixes #7397 (adds e2e tests for disputed order notice → transaction details flow)
Changes proposed in this Pull Request
This PR updates e2e tests for merchant dispute challenge/accept flows to work with the new Transaction Dispute Details UI (#6883).
These e2e test suites were skipped in #7088 and now have been updated and re-enabled:
tests/e2e/specs/wcpay/merchant/merchant-disputes-save-for-editing.spec.jsDisputes > Save dispute for editingtests/e2e/specs/wcpay/merchant/merchant-disputes-submit-losing.spec.jsDisputes > Submit losing disputetests/e2e/specs/wcpay/merchant/merchant-disputes-submit-winning.spec.jsDisputes > Submit winning disputeI've also used the dispute notice's Respond now button on the order edit screen to navigate to the transaction details screen for these flows, which is a critical flow. This replaces the previous method of navigating the browser directly to the transaction details URL. Expanding our e2e test coverage to include this flow fixes #7397.
TODO:
Testing instructions
E2E Tests - Pull Request / WC - latest | wcpay - merchant
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge