enhance/deliverytime-react-e2e-test #2904
Open
Conversation
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
Contributor
WalkthroughAdds a new Playwright E2E test validating vendor delivery-time UI flows (access and settings) and extends test data with a new vendor dashboard path for the React delivery-time settings page. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as Playwright Test
participant Browser
participant App as Vendor Dashboard (Web)
participant API as Backend API
rect rgb(235, 245, 255)
note right of Tester: Access flow
Tester->>Browser: Launch vendor context and login
Browser->>App: Navigate to Delivery Time page
App-->>Browser: Render "Delivery Time & Store Pickup"
Tester->>App: Select Delivery / Store Pickup / All Events
Tester->>App: Switch views (week/day/list/month)
Tester->>App: Navigate months (next/prev) and Today
end
rect rgb(240, 255, 240)
note right of Tester: Settings flow
Tester->>App: Open Delivery Time settings
App-->>Browser: Show settings labels and controls
Tester->>App: Toggle Home Delivery / Store Pickup
Tester->>App: Adjust blocked buffer, time slot, orders per slot
Tester->>App: Save settings
App->>API: Submit settings update
API-->>App: Success response
App-->>Browser: Toast "Delivery settings has been saved successfully!"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/pw/tests/e2e/deliverytime-react.spec.ts (3)
40-50: Assert view changes, not just clicks.After switching views/months, assert state (e.g., aria-pressed or visible calendar grid) to catch regressions.
Example:
await aPage.getByRole('button', { name: /week/i }).click(); await expect(aPage.getByRole('button', { name: /week/i })).toHaveAttribute('aria-pressed', 'true');
62-74: Use label-based locators or regex to reduce brittleness.Hard-coding punctuation/spacing in label text is fragile. Prefer
getByLabelor tolerant regex.Examples:
await expect(aPage.getByLabel(/Delivery Support/i)).toBeVisible(); await expect(aPage.getByLabel(/Delivery blocked buffer/i)).toBeVisible(); await expect(aPage.getByLabel(/Time slot/i)).toBeVisible(); await expect(aPage.getByLabel(/Order per slot/i)).toBeVisible();
52-94: Test isolation: avoid persisting mutated settings across runs.This test edits and saves vendor settings but doesn’t restore them. Consider:
- snapshot/restore via API,
- or store previous values and revert in
afterAll,- or use a disposable vendor.
I can help wire a small helper to fetch current delivery-time settings via REST and restore after the test. Want that?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/pw/tests/e2e/deliverytime-react.spec.ts(1 hunks)tests/pw/utils/testData.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/pw/tests/e2e/deliverytime-react.spec.ts (2)
tests/pw/pages/vendorPage.ts (1)
VendorPage(20-388)tests/pw/utils/testData.ts (1)
data(63-2916)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (2)
tests/pw/utils/testData.ts (1)
1232-1233: Confirm React hash route format (‘#/settings’ vs ‘#settings’).If the React vendor dashboard uses HashRouter semantics, the path should typically include a slash after the hash. Verify the actual route and adjust to avoid 404s or silent redirects.
Apply if applicable:
- settingsDeliveryTimeReact: 'dashboard/new/#settings/delivery-time', + // If HashRouter is in use, include the leading slash + settingsDeliveryTimeReact: 'dashboard/new/#/settings/delivery-time',tests/pw/tests/e2e/deliverytime-react.spec.ts (1)
20-20: No change required — Playwright supports per-testtagintest(...)options.Playwright's test API accepts a details object with
tag: string | string[];{ tag: ['@pro', '@vendor'] }is valid — leave as-is.
Comment on lines
+1
to
+18
| import { test, Page, expect } from "@playwright/test"; | ||
| import { data } from '@utils/testData'; | ||
| import { VendorPage } from '@pages/vendorPage'; | ||
|
|
||
| test.describe('Delivery time test', () => { | ||
| let vendor: VendorPage; | ||
| let aPage: Page; | ||
|
|
||
| test.beforeAll(async ({ browser }) => { | ||
| const vendorContext = await browser.newContext(data.auth.vendorAuth); | ||
|
|
||
| aPage = await vendorContext.newPage(); | ||
| vendor = new VendorPage(aPage); | ||
| }); | ||
|
|
||
| test.afterAll(async () => { | ||
| await aPage.close(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Close the BrowserContext to avoid resource leaks.
vendorContext is created but never closed; only the page is closed. Keep a suite-scoped reference and close it in afterAll.
-import { test, Page, expect } from "@playwright/test";
+import { test, Page, expect, BrowserContext } from "@playwright/test";
@@
test.describe('Delivery time test', () => {
let vendor: VendorPage;
let aPage: Page;
+ let vendorContext: BrowserContext;
@@
- test.beforeAll(async ({ browser }) => {
- const vendorContext = await browser.newContext(data.auth.vendorAuth);
+ test.beforeAll(async ({ browser }) => {
+ vendorContext = await browser.newContext(data.auth.vendorAuth);
@@
test.afterAll(async () => {
- await aPage.close();
+ await aPage.close();
+ await vendorContext.close();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { test, Page, expect } from "@playwright/test"; | |
| import { data } from '@utils/testData'; | |
| import { VendorPage } from '@pages/vendorPage'; | |
| test.describe('Delivery time test', () => { | |
| let vendor: VendorPage; | |
| let aPage: Page; | |
| test.beforeAll(async ({ browser }) => { | |
| const vendorContext = await browser.newContext(data.auth.vendorAuth); | |
| aPage = await vendorContext.newPage(); | |
| vendor = new VendorPage(aPage); | |
| }); | |
| test.afterAll(async () => { | |
| await aPage.close(); | |
| }); | |
| import { test, Page, expect, BrowserContext } from "@playwright/test"; | |
| import { data } from '@utils/testData'; | |
| import { VendorPage } from '@pages/vendorPage'; | |
| test.describe('Delivery time test', () => { | |
| let vendor: VendorPage; | |
| let aPage: Page; | |
| let vendorContext: BrowserContext; | |
| test.beforeAll(async ({ browser }) => { | |
| vendorContext = await browser.newContext(data.auth.vendorAuth); | |
| aPage = await vendorContext.newPage(); | |
| vendor = new VendorPage(aPage); | |
| }); | |
| test.afterAll(async () => { | |
| await aPage.close(); | |
| await vendorContext.close(); | |
| }); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/deliverytime-react.spec.ts around lines 1 to 18, the
browser context vendorContext is created in beforeAll but not closed, causing a
resource leak; change the test to store vendorContext in a suite-scoped variable
(e.g., declare let vendorContext: BrowserContext above beforeAll), assign it
inside beforeAll when creating the context, use vendorContext.newPage() to
create the page, and then close the context in afterAll (await
vendorContext.close()) instead of only closing the page.
Comment on lines
+22
to
+23
| await aPage.getByRole('link', { name: ' Delivery Time' }).first().click(); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid brittle icon glyph in accessible name.
The leading glyph “” can change with icon fonts. Match on text only.
- await aPage.getByRole('link', { name: ' Delivery Time' }).first().click();
+ await aPage.getByRole('link', { name: /Delivery Time/i }).first().click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await aPage.getByRole('link', { name: ' Delivery Time' }).first().click(); | |
| await aPage.getByRole('link', { name: /Delivery Time/i }).first().click(); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/deliverytime-react.spec.ts around lines 22-23, the test
clicks a link using an accessible name that includes an icon glyph (“ Delivery
Time”), which is brittle; update the selector to match only the visible text
(e.g., use getByRole('link', { name: 'Delivery Time' }) or a regex like
/Delivery Time/ ) so the test no longer depends on the icon glyph.
Comment on lines
+24
to
+26
| // Wait for 5 seconds | ||
| await aPage.waitForTimeout( 3000 ); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove arbitrary sleeps; rely on auto-wait via expect.
waitForTimeout(3000) is flaky and slows tests; the next assertion already waits.
- // Wait for 5 seconds
- await aPage.waitForTimeout( 3000 );
+ // Page content will be awaited by the next assertion📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Wait for 5 seconds | |
| await aPage.waitForTimeout( 3000 ); | |
| // Page content will be awaited by the next assertion |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/deliverytime-react.spec.ts around lines 24–26, remove the
arbitrary await aPage.waitForTimeout(3000) and rely on Playwright's auto-waiting
by awaiting the next expect assertion (e.g., await
expect(locator).toHaveText/...); if the element can be absent initially and the
assertion still flakes, replace the sleep with a targeted wait like await
locator.waitFor({ state: 'visible', timeout: 5000 }) before the expect,
otherwise simply delete the waitForTimeout call.
Comment on lines
+31
to
+39
| await aPage.locator('.css-1nxca15-UA').click(); | ||
| await aPage.getByRole('option', { name: 'Delivery' }).click(); | ||
| await aPage.getByRole('button', { name: 'Filter' }).click(); | ||
| await aPage.locator('.css-1nxca15-UA').click(); | ||
| await aPage.getByRole('option', { name: 'Store Pickup' }).click(); | ||
| await aPage.getByRole('button', { name: 'Filter' }).click(); | ||
| await aPage.locator('.css-1nxca15-UA').click(); | ||
| await aPage.getByRole('option', { name: 'All Events' }).click(); | ||
| await aPage.getByRole('button', { name: 'Filter' }).click(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hashed CSS selector with stable locator.
.css-1nxca15-UA looks generated and may change between builds. Prefer roles, labels, or data-testid.
Example (adjust to actual UI):
// Prefer one of:
await aPage.getByRole('combobox', { name: /event/i }).click();
// or add a stable test id in the app and use:
await aPage.getByTestId('dt-event-type').click();I can help add minimal data-testid hooks if you want to wire them in the UI.
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/deliverytime-react.spec.ts around lines 31 to 39, the test
uses a generated CSS selector `.css-1nxca15-UA` which is brittle; replace each
occurrence with a stable locator such as getByRole('combobox', { name: /event/i
}) or getByTestId('dt-event-type') (add a data-testid in the app if necessary),
and update the three click calls to use that stable locator before selecting the
options and clicking the Filter button.
Comment on lines
+55
to
+57
| // Wait for the page to load | ||
| await aPage.waitForTimeout(3000); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drop fixed sleep before header assertion.
The header assertion already auto-waits; remove the 3s delay.
- // Wait for the page to load
- await aPage.waitForTimeout(3000);
+ // Rely on expect auto-wait below📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Wait for the page to load | |
| await aPage.waitForTimeout(3000); | |
| // Rely on expect auto-wait below |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/deliverytime-react.spec.ts around lines 55-57, remove the
fixed 3s wait (await aPage.waitForTimeout(3000)); the header assertion already
auto-waits — delete the sleep and keep the existing header assertion (or use
Playwright's expect with toHaveText/toContainText) so the test relies on the
assertion's built-in waiting instead of a hard timeout.
Comment on lines
+76
to
+88
| await aPage.getByRole('checkbox', { name: 'Home Delivery' }).check(); | ||
| await aPage.getByRole('checkbox', { name: 'Home Delivery' }).uncheck(); | ||
| await aPage.getByRole('checkbox', { name: 'Store Pickup' }).check(); | ||
| await aPage.getByRole('checkbox', { name: 'Store Pickup' }).uncheck(); | ||
| await aPage.getByRole('spinbutton', { name: 'Delivery blocked buffer :' }).click(); | ||
| await aPage.getByRole('spinbutton', { name: 'Delivery blocked buffer :' }).fill('10'); | ||
| await aPage.getByRole('spinbutton', { name: 'Time slot :' }).click(); | ||
| await aPage.getByRole('spinbutton', { name: 'Time slot :' }).press('ControlOrMeta+a'); | ||
| await aPage.getByRole('spinbutton', { name: 'Time slot :' }).fill('60'); | ||
| await aPage.getByRole('spinbutton', { name: 'Order per slot :' }).click(); | ||
| await aPage.getByRole('spinbutton', { name: 'Order per slot :' }).press('ControlOrMeta+a'); | ||
| await aPage.getByRole('spinbutton', { name: 'Order per slot :' }).fill('1'); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add state assertions for toggles and ensure fields are cleared before typing.
- Verify check/uncheck results.
- For “Delivery blocked buffer :”, also select-all before fill (consistent with the others).
- await aPage.getByRole('checkbox', { name: 'Home Delivery' }).check();
- await aPage.getByRole('checkbox', { name: 'Home Delivery' }).uncheck();
- await aPage.getByRole('checkbox', { name: 'Store Pickup' }).check();
- await aPage.getByRole('checkbox', { name: 'Store Pickup' }).uncheck();
- await aPage.getByRole('spinbutton', { name: 'Delivery blocked buffer :' }).click();
- await aPage.getByRole('spinbutton', { name: 'Delivery blocked buffer :' }).fill('10');
+ const homeDelivery = aPage.getByRole('checkbox', { name: /Home Delivery/i });
+ const storePickup = aPage.getByRole('checkbox', { name: /Store Pickup/i });
+ await homeDelivery.check(); await expect(homeDelivery).toBeChecked();
+ await homeDelivery.uncheck();await expect(homeDelivery).not.toBeChecked();
+ await storePickup.check(); await expect(storePickup).toBeChecked();
+ await storePickup.uncheck(); await expect(storePickup).not.toBeChecked();
+ const blockedBuffer = aPage.getByRole('spinbutton', { name: /Delivery blocked buffer/i });
+ await blockedBuffer.click();
+ await blockedBuffer.press('ControlOrMeta+a');
+ await blockedBuffer.fill('10');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await aPage.getByRole('checkbox', { name: 'Home Delivery' }).check(); | |
| await aPage.getByRole('checkbox', { name: 'Home Delivery' }).uncheck(); | |
| await aPage.getByRole('checkbox', { name: 'Store Pickup' }).check(); | |
| await aPage.getByRole('checkbox', { name: 'Store Pickup' }).uncheck(); | |
| await aPage.getByRole('spinbutton', { name: 'Delivery blocked buffer :' }).click(); | |
| await aPage.getByRole('spinbutton', { name: 'Delivery blocked buffer :' }).fill('10'); | |
| await aPage.getByRole('spinbutton', { name: 'Time slot :' }).click(); | |
| await aPage.getByRole('spinbutton', { name: 'Time slot :' }).press('ControlOrMeta+a'); | |
| await aPage.getByRole('spinbutton', { name: 'Time slot :' }).fill('60'); | |
| await aPage.getByRole('spinbutton', { name: 'Order per slot :' }).click(); | |
| await aPage.getByRole('spinbutton', { name: 'Order per slot :' }).press('ControlOrMeta+a'); | |
| await aPage.getByRole('spinbutton', { name: 'Order per slot :' }).fill('1'); | |
| const homeDelivery = aPage.getByRole('checkbox', { name: /Home Delivery/i }); | |
| const storePickup = aPage.getByRole('checkbox', { name: /Store Pickup/i }); | |
| await homeDelivery.check(); await expect(homeDelivery).toBeChecked(); | |
| await homeDelivery.uncheck();await expect(homeDelivery).not.toBeChecked(); | |
| await storePickup.check(); await expect(storePickup).toBeChecked(); | |
| await storePickup.uncheck(); await expect(storePickup).not.toBeChecked(); | |
| const blockedBuffer = aPage.getByRole('spinbutton', { name: /Delivery blocked buffer/i }); | |
| await blockedBuffer.click(); | |
| await blockedBuffer.press('ControlOrMeta+a'); | |
| await blockedBuffer.fill('10'); | |
| await aPage.getByRole('spinbutton', { name: 'Time slot :' }).click(); | |
| await aPage.getByRole('spinbutton', { name: 'Time slot :' }).press('ControlOrMeta+a'); | |
| await aPage.getByRole('spinbutton', { name: 'Time slot :' }).fill('60'); | |
| await aPage.getByRole('spinbutton', { name: 'Order per slot :' }).click(); | |
| await aPage.getByRole('spinbutton', { name: 'Order per slot :' }).press('ControlOrMeta+a'); | |
| await aPage.getByRole('spinbutton', { name: 'Order per slot :' }).fill('1'); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/deliverytime-react.spec.ts around lines 76 to 88, the test
interacts with several checkboxes and spinbuttons but doesn't assert state
changes and doesn't consistently clear the "Delivery blocked buffer :" field
before filling; update the test to assert that each checkbox is checked after
.check() and unchecked after .uncheck(), and for the "Delivery blocked buffer :"
spinbutton perform a select-all (press 'ControlOrMeta+a') before filling (like
the other spinbuttons) to ensure the field is cleared; keep the existing
interactions but add these assertions and the select-all step so the test
reliably verifies state and avoids leftover input.
Comment on lines
+89
to
+94
| await aPage.getByRole('button', { name: 'Save Changes' }).click(); | ||
|
|
||
| await aPage.waitForTimeout(100); | ||
| const successMessage = aPage.locator('p:has-text("Delivery settings has been saved successfully!")'); | ||
| await expect(successMessage).toBeVisible(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert toast via shared test data; avoid fixed sleep.
Remove waitForTimeout(100) and assert the success message using the constant in testData.
- await aPage.getByRole('button', { name: 'Save Changes' }).click();
-
- await aPage.waitForTimeout(100);
- const successMessage = aPage.locator('p:has-text("Delivery settings has been saved successfully!")');
- await expect(successMessage).toBeVisible();
+ await aPage.getByRole('button', { name: /Save Changes/i }).click();
+ await expect(aPage.getByText(data.vendor.deliveryTime.saveSuccessMessage)).toBeVisible();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await aPage.getByRole('button', { name: 'Save Changes' }).click(); | |
| await aPage.waitForTimeout(100); | |
| const successMessage = aPage.locator('p:has-text("Delivery settings has been saved successfully!")'); | |
| await expect(successMessage).toBeVisible(); | |
| }); | |
| await aPage.getByRole('button', { name: /Save Changes/i }).click(); | |
| await expect(aPage.getByText(data.vendor.deliveryTime.saveSuccessMessage)).toBeVisible(); | |
| }); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/deliverytime-react.spec.ts around lines 89 to 94, remove
the hard-coded await aPage.waitForTimeout(100) and replace the literal success
message assertion with the shared test data constant: use the testData success
message string (import or reference the existing testData constant) to build the
locator/assertion (e.g., locate
p:has-text(testData.deliverySettingsSavedMessage)) and call await
expect(...).toBeVisible() so the test waits for the toast via Playwright's
built-in expect rather than a fixed sleep.
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.
All Submissions:
Related Pull Requests
Closes
Summary by CodeRabbit