E2E Playwright Migration: convert merchant-subscriptions-renew-action-scheduler spec #10143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
…ons-renew-action-scheduler-spec
…ons-renew-action-scheduler-spec
Update WIP spec
…ons-renew-action-scheduler-spec
Add changelog
tpaksu
requested changes
Jan 17, 2025
Comment on lines
67
to
68
| const { merchantPage } = await getMerchant( browser ); | ||
| page = merchantPage; |
There was a problem hiding this comment.
Since you already added useMerchant just after the describe, the page is already merchantPage. I suggest using getMerchant only if you also need to use getShopper in the code, for the context not to mix, and prevent unintended use of page.
Suggestion: Instead of useMerchant you can write let shopperPage, merchantPage: Page;, assign them in the beforeAll callback, and then use them in any test you want.
There was a problem hiding this comment.
Since you already added useMerchant just after the describe, the page is already merchantPage. I suggest using getMerchant only if you also need to use getShopper in the code, for the context not to mix, and prevent unintended use of page.
TIL that beforeAll doesn't assign the page yet with useMerchant, because it only assigns pages per test basis. But you can still remove useMerchant and use the specific pages from getShopper and getMerchant assigned to shopperPage and merchantPage respectively on the global scope.
There was a problem hiding this comment.
Thanks for the review! I was running into some issues with that very thing with beforeAll and got it working in a sub-optimal way as you pointed out with useMerchant and the redundant page assignments. I have cleaned this up in 30107c1.
Comment on lines
49
to
50
| const { shopperPage } = await getAnonymousShopper( browser ); | ||
| page = shopperPage; |
There was a problem hiding this comment.
You don't need to assign shopperPage to page, to be clear in the code, it'll be probably best to use shopperPage instead of page. See my other comment about useMerchant.
There was a problem hiding this comment.
I've fixed this up in 30107c1.
…ons-renew-action-scheduler-spec
tpaksu
approved these changes
Jan 21, 2025
…ons-renew-action-scheduler-spec
allie500
deleted the
dev/10081-e2e-convert-merchant-subscriptions-renew-action-scheduler-spec
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10081
Changes proposed in this Pull Request
merchant-subscriptions-renew-action-schedulerspec to Playwright E2E tests.merchant-subscriptions-renew-action-schedulerspec from Puppeteer E2E tests.shouldRunActionSchedulerTests.Testing instructions
npm run build:clientnpm run test:e2e-pw merchant-subscriptions-renew-action-schedulernpm 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