-
Notifications
You must be signed in to change notification settings - Fork 68
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
E2E Playwright Migration: convert Order Partial Refund spec #10194
base: develop
Are you sure you want to change the base?
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: 0 B Total Size: 1.36 MB ℹ️ View Unchanged
|
Add restoreCurrencies Update shopper and merchant vars
Implement test loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, I guess this is still a WIP, as it is missing most of the verifications of the updated rows, refund block, and the partial refund note added after the refund operation. Let me know if skipping those were intentional, but I'm more inclined that those should be included in the PR.
tests/e2e-pw/specs/merchant/merchant-orders-partial-refund.spec.ts
Outdated
Show resolved
Hide resolved
} | ||
} ); | ||
|
||
for ( let i = 0; i < dataTable.length; i++ ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: You can use this for accessing the elements in the dataTable
easily:
Object.entries(dataTable).forEach(([title, { lineItems, refundInputs }]) => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this suggestion. I wasn't able to implement it exactly this way. I also need the i
for targeting the correct orderId
from the orderIds
array. I took a similar approach in f68aa46.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could get the index with:
Object.entries(dataTable).forEach(([title, { lineItems, refundInputs }], i) => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I got the index with the i
like that, but Typescript doesn't like when I use Object.entries(dataTable).forEach
instead of just dataTable.forEach
. If I use your suggestion, I get:
Property 'refundInputs' does not exist on type '[string, { lineItems: [string, number][]; refundInputs: { refundQty: number; refundAmount: number; }[]; }]'.ts(2339)
I don't get it with lineItems
b/c I remove them since they aren't used in the test, only in the beforeAll setup. If I add them back, I also get the Typescript warning about them not existing on the dataTable type.
dataTable.forEach
is working well though, so I think we can stick with it.
tests/e2e-pw/specs/merchant/merchant-orders-partial-refund.spec.ts
Outdated
Show resolved
Hide resolved
tests/e2e-pw/specs/merchant/merchant-orders-partial-refund.spec.ts
Outdated
Show resolved
Hide resolved
Thanks for the review, Taha! Skipping those was not intentional. I must have just missed them when converting the spec. I'll double check it and get those added. I'll re-request a review once that is done. |
Add comments for clarity
@tpaksu, I've addressed your feedback, and I think I've covered all the refund line items, order notes, etc checks after the refund is processed that I missed the first time around. Could you give this one another review when you have a moment? |
.nth( j ) | ||
.fill( refundAmount.toString() ); | ||
|
||
await merchantPage.keyboard.press( 'Tab' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an issue but out of curiousity, why do we need this Tab press if we're filling with locators? To trigger the calculations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #9963
Changes proposed in this Pull Request
Testing instructions
npm run test:e2e-pw merchant-orders-partial-refund
npm run changelog
to add a changelog file, choosepatch
to 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