-
Notifications
You must be signed in to change notification settings - Fork 394
test(e2e): delivery address #3607
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
cf1412a
test(e2e): delivery address CRUD tests with MSW
itsjustriley 2337d8e
fix(skeleton): use human-readable aria-label for territory code input
itsjustriley 8ddf34b
refactor(e2e): verify mutation persistence via re-navigation
itsjustriley 371f0fc
fix(e2e): restore DOM-based delete completion signal
itsjustriley 13799b3
fix(e2e): use count-based delete signal and stronger non-presence ass…
itsjustriley 79c90c0
refactor(e2e): replace networkidle with proper completion signals
itsjustriley d20dcdf
docs(e2e): document waitForResponse deviation in updateAddress
itsjustriley 849a7d3
refactor(e2e): register DeliveryAddressUtil as Playwright fixture
itsjustriley File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| 'skeleton': patch | ||
| '@shopify/cli-hydrogen': patch | ||
| '@shopify/create-hydrogen': patch | ||
| --- | ||
|
|
||
| Fix broken `aria-label` on territory code input in address form. The label was the raw developer string `"territoryCode"` instead of a human-readable `"Country code"`. |
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| import {expect, Locator, Page} from '@playwright/test'; | ||
|
|
||
| export const EMPTY_STATE_MESSAGE = 'You have no addresses saved.'; | ||
|
|
||
| export interface AddressFormData { | ||
| firstName: string; | ||
| lastName: string; | ||
| company?: string; | ||
| address1: string; | ||
| address2?: string; | ||
| city: string; | ||
| zoneCode: string; | ||
| zip: string; | ||
| territoryCode: string; | ||
| phoneNumber?: string; | ||
| defaultAddress?: boolean; | ||
| } | ||
|
|
||
| export class DeliveryAddressUtil { | ||
| constructor(private page: Page) {} | ||
|
|
||
| async navigateToAddresses() { | ||
| await this.page.goto('/account/addresses'); | ||
| await expect( | ||
| this.page.getByRole('heading', {level: 2, name: 'Addresses'}), | ||
| ).toBeVisible(); | ||
| } | ||
|
|
||
| /** | ||
| * Returns each existing address form (excluding the create form). | ||
| * The create form has id="NEW_ADDRESS_ID"; existing forms have GID-based ids. | ||
| * We select forms with Save/Delete buttons, which only appear on existing addresses. | ||
| */ | ||
| getExistingAddresses(): Locator { | ||
| return this.page | ||
| .locator('form') | ||
| .filter({has: this.page.getByRole('button', {name: 'Save'})}); | ||
| } | ||
|
|
||
| getCreateAddressForm(): Locator { | ||
| return this.page | ||
| .locator('form') | ||
| .filter({has: this.page.getByRole('button', {name: 'Create'})}); | ||
| } | ||
|
|
||
| getEmptyState(): Locator { | ||
| return this.page.getByText(EMPTY_STATE_MESSAGE); | ||
| } | ||
|
|
||
| private static readonly FIELD_LABEL_MAP: Record< | ||
| keyof Omit<AddressFormData, 'defaultAddress'>, | ||
| string | ||
| > = { | ||
| firstName: 'First name', | ||
| lastName: 'Last name', | ||
| company: 'Company', | ||
| address1: 'Address line 1', | ||
| address2: 'Address line 2', | ||
| city: 'City', | ||
| zoneCode: 'State/Province', | ||
| zip: 'Zip', | ||
| territoryCode: 'Country code', | ||
| phoneNumber: 'Phone Number', | ||
| }; | ||
|
|
||
| async fillAddressForm(form: Locator, data: Partial<AddressFormData>) { | ||
| for (const [field, label] of Object.entries( | ||
| DeliveryAddressUtil.FIELD_LABEL_MAP, | ||
| )) { | ||
| const value = data[field as keyof typeof data]; | ||
| if (value !== undefined && typeof value === 'string') { | ||
| await form.getByLabel(label).fill(value); | ||
| } | ||
| } | ||
| if (data.defaultAddress !== undefined) { | ||
| const checkbox = form.getByRole('checkbox'); | ||
| const isChecked = await checkbox.isChecked(); | ||
| if (data.defaultAddress !== isChecked) { | ||
| await checkbox.click(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async createAddress(data: AddressFormData) { | ||
| const countBefore = await this.getExistingAddresses().count(); | ||
| const form = this.getCreateAddressForm(); | ||
| await this.fillAddressForm(form, data); | ||
| const createButton = form.getByRole('button', {name: 'Create'}); | ||
| await createButton.click(); | ||
| // Wait for a new existing-address form to appear, proving the full | ||
| // create -> revalidate -> re-render cycle completed. | ||
| await this.assertAddressCount(countBefore + 1); | ||
| } | ||
|
|
||
| // DEVIATION: waitForResponse is used here despite the e2e guideline to | ||
| // "wait for the visible effect rather than the underlying mechanism." | ||
| // The skeleton's AddressForm has no visible success feedback (no toast, no | ||
| // flash), and the inputs are uncontrolled (defaultValue) so user-typed values | ||
| // persist in the DOM regardless of mutation success. There is no user-visible | ||
| // effect to wait for. Tests that call updateAddress must re-navigate afterward | ||
| // to verify persistence via a fresh mount from MSW state. | ||
| async updateAddress(form: Locator, data: Partial<AddressFormData>) { | ||
| await this.fillAddressForm(form, data); | ||
| const saveButton = form.getByRole('button', {name: 'Save'}); | ||
| const actionResponse = this.page.waitForResponse((res) => | ||
| res.url().includes('/account/addresses'), | ||
| ); | ||
| await saveButton.click(); | ||
| await actionResponse; | ||
| } | ||
|
|
||
| async deleteAddress(form: Locator) { | ||
| const countBefore = await this.getExistingAddresses().count(); | ||
| const deleteButton = form.getByRole('button', {name: 'Delete'}); | ||
| await expect(deleteButton).toBeVisible(); | ||
| await deleteButton.click(); | ||
| // Wait for the address count to decrease, proving the full | ||
| // delete -> revalidate -> re-render cycle completed. We can't use | ||
| // not.toBeVisible() on the delete button because Playwright locators | ||
| // are lazy: after the form is removed, .first()/.last() shifts to | ||
| // the next form whose delete button IS visible. | ||
| await this.assertAddressCount(countBefore - 1); | ||
| } | ||
|
|
||
| async assertAddressCount(count: number) { | ||
| if (count === 0) { | ||
| await expect(this.getEmptyState()).toBeVisible(); | ||
| return; | ||
| } | ||
| await expect(this.getExistingAddresses()).toHaveCount(count); | ||
| } | ||
|
|
||
| async assertAddressVisible( | ||
| data: Partial<Omit<AddressFormData, 'defaultAddress'>>, | ||
| ) { | ||
| const entries = Object.entries(data).map(([field, value]) => ({ | ||
| label: | ||
| DeliveryAddressUtil.FIELD_LABEL_MAP[ | ||
| field as keyof typeof DeliveryAddressUtil.FIELD_LABEL_MAP | ||
| ], | ||
| value: value as string, | ||
| })); | ||
|
|
||
| if (entries.length === 0) { | ||
| throw new Error('assertAddressVisible requires at least one field'); | ||
| } | ||
|
|
||
| const [matchField, ...remainingFields] = entries; | ||
| const forms = this.getExistingAddresses(); | ||
| const count = await forms.count(); | ||
|
|
||
| for (let i = 0; i < count; i++) { | ||
| const form = forms.nth(i); | ||
| const actual = await form.getByLabel(matchField.label).inputValue(); | ||
| if (actual === matchField.value) { | ||
| for (const field of remainingFields) { | ||
| await expect(form.getByLabel(field.label)).toHaveValue(field.value); | ||
| } | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| throw new Error( | ||
| `No address found matching ${matchField.label}="${matchField.value}"`, | ||
| ); | ||
| } | ||
| } | ||
This file contains hidden or 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
fredericoo marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| export const MSW_SCENARIOS = { | ||
| customerAccountLoggedIn: 'customer-account-logged-in', | ||
| deliveryAddresses: 'delivery-addresses', | ||
| } as const; | ||
|
|
||
| export type MswScenario = (typeof MSW_SCENARIOS)[keyof typeof MSW_SCENARIOS]; |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
we can check if the saveButton text became "Saving" and then if it became "Save" again, better than checking for response
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.
tried this - the "Saving" text (and the disabled state) is too ephemeral with instant MSW responses for Playwright to observe. the entire fetcher cycle (idle → submitting → loading → idle) completes within a single microtask batch, so Playwright's polling never catches the intermediate state. tested both
toBeVisible()on the "Saving" text andtoBeDisabled()on the button - both time out.kept
waitForResponsewith an updated deviation comment explaining the microtask timing. IIRC this is the only reliable signal when the round-trip has no durable user-visible effect (uncontrolled inputs + instant mock responses).