refactor(e2e): one user per worker and fix failing tests#9005
Draft
kiran-redhat wants to merge 26 commits intomainfrom
Draft
refactor(e2e): one user per worker and fix failing tests#9005kiran-redhat wants to merge 26 commits intomainfrom
kiran-redhat wants to merge 26 commits intomainfrom
Conversation
Reuse onboarding-authenticated browser context across suites and split the slowest discover specs into smaller files so they can execute in parallel workers with better throughput. Made-with: Cursor
…-level tests Replaced direct navigation to the root URL with a dedicated method to navigate to the Discover Journeys page, ensuring the "Create Custom Journey" button is enabled upon loading. This change enhances test reliability and clarity.
- navigateToMediaScreen: wait for URL change after each Next click before checking isVisible(), preventing overshooting past the media screen due to client-side navigation race; also bail early if we've left /customize - waitForAutoSubmitError: replace brittle p.MuiFormHelperText-root.Mui-error selector with aria-invalid="true" on the input element - waitForAutoSubmit / getYouTubeHelperText: replace .MuiFormHelperText-root with p (MUI always renders helper text in a <p>) - registerNewAccount: use TermsAndConditionsNextButton data-testid instead of generic button[type="button"] to avoid ambiguous matches; add waitForURL to confirm navigation away from Terms before calling waitUntilDiscoverPageLoaded - gotoDiscoverJourneysPage: if Create Custom Journey is not enabled within 30s (lastActiveTeamId race between updateLastActiveTeamId mutation and SSR page load), reload to trigger fresh SSR with committed team data Made-with: Cursor
Replaced the registration process with a login method using pre-created user accounts across various journey-related test files. This change enhances test efficiency by eliminating the need for account creation during test execution, allowing for faster and more reliable tests.
Updated journey-related test files to utilize pre-created user accounts for login, replacing the previous registration process. This change improves test execution speed and reliability by removing the need for dynamic account creation during tests.
- Worker-scoped registration with activeTeamId merged into storage state - Stabilize discover, teams, profile, and journey editor navigation - YouTube customization: detect missing media step and skip when LD flag off Made-with: Cursor
…d With Me selector Replaced the previous button check with a combobox check for the Shared With Me option to ensure the Discover page is fully loaded. Adjusted timeout handling for improved reliability during page load and reloading scenarios.
…ions Updated the registration process to include explicit checks for page navigation and visibility, ensuring that each step is validated before proceeding. Introduced dedicated methods for asserting the current page context, improving test reliability and clarity. This refactor also includes error handling to provide better feedback on failures during the registration process.
… URL matching Removed hardcoded URL regex from multiple files and centralized it in a new constants file for better maintainability. Updated journey-level actions and page classes to utilize the new regex for improved navigation handling. Enhanced timeout management for page transitions to ensure stability during tests.
Updated the image upload method to wait for the Custom tab panel to render before interaction, improving reliability. Modified the backToHome method to use Promise.all for better synchronization during navigation, ensuring the URL is fully loaded before proceeding. Increased timeout for visibility checks in the registration flow to accommodate potential delays in rendering.
…e2e-one-user-per-worker
kiran-redhat
requested review from
mikeallisonJS,
sharon-elsa-mathew1995 and
tataihono
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a worker-scoped Playwright auth fixture that registers per-worker accounts and writes per-worker storage-state files; migrates many specs to reuse worker storage state and shared contexts/pages (serial suites); adds multiple new discover/journey/card-level E2E specs; tightens onboarding, media, navigation synchronization, timeouts, and click-with-navigation patterns in page objects; updates CI artifact uploads and concurrency; introduces stricter e2e rules doc. Changes
Sequence DiagramsequenceDiagram
participant Worker as Playwright Worker
participant Fixture as workerAuth Fixture
participant Browser as Headless Browser
participant App as Web App
participant Storage as Temp File
Worker->>Fixture: initialize worker-scoped fixtures
Fixture->>Browser: browser.newContext() & newPage()
Browser->>App: navigate → registration/onboarding flows
App-->>Browser: onboarding UI & sessionStorage (team id)
Fixture->>Browser: complete registration & capture sessionStorage
Fixture->>Browser: context.storageState()
Browser->>Storage: write worker storage-state JSON (inject sessionStorage)
Fixture-->>Worker: return workerEmail & workerStorageState path
loop For each spec on worker
Worker->>Browser: newContext({ storageState: workerStorageState })
Browser->>Browser: create Page(s) per test (or sharedContext)
Browser->>App: run tests using pre-authenticated pages
Browser->>Browser: close Page(s)
end
Worker->>Browser: close shared resources after suite
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit a646780
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit f6c2701
☁️ Nx Cloud last updated this comment at |
Contributor
|
The latest updates on your projects.
|
![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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/journeys-admin-e2e/src/pages/journey-level-actions-page.ts (1)
393-395:⚠️ Potential issue | 🟠 MajorRemove the
sleephelper and replace its usage with soft waits.The method at lines 393-395 implements a hard wait using
setTimeout, which violates E2E testing guidelines. Hard waits are forbidden — all waiting must use Playwright's built-in auto-waiting viaexpect()assertions or methods with built-in waiting.The method has one active usage at
apps/journeys-admin-e2e/src/e2e/discover/journey-level-actions-discover.spec.ts:110(await journeyLevelActions.sleep(2000)), which appears between a save operation and a subsequent click. Remove the method definition and replace the usage with a proper soft wait pattern (e.g., waiting for the expected page state, redirect, or element visibility after the save completes).Method to remove
- async sleep(ms): Promise<Promise<void>> { - return await new Promise((resolve) => setTimeout(resolve, ms)) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/journey-level-actions-page.ts` around lines 393 - 395, Remove the hard-wait helper sleep (async sleep(ms)) from journey-level-actions-page and replace its single usage in the spec (await journeyLevelActions.sleep(2000)) with a Playwright soft-wait: after the save action, wait for a determinative UI/state change instead of sleeping — e.g., await expect(saveButton).toBeEnabled()/toHaveCount()/toBeVisible(), await expect(successToast).toBeVisible(), await page.waitForResponse(r => r.url().includes('/save') && r.status() === 200), or await page.waitForNavigation() depending on the app behavior; update the spec to use one of these built-in waits so the test waits for the save to complete rather than sleeping.apps/journeys-admin-e2e/src/pages/journey-page.ts (1)
321-340:⚠️ Potential issue | 🟠 MajorThese 90s click timeouts can stretch one editor failure into minutes.
Line 321 and Line 340 now sit inside a retry loop, so a missing/intercepted typography target can burn through multiple 90s action waits before the test finally fails. The timeout guideline also requires an inline justification whenever we go past the default.
As per coding guidelines:
apps/**/*e2e*/**/*.ts: "Wait timeouts must be short and justified. The default timeout (30s) is sufficient for stable app states. Only exceed it for known slow operations..." and "When a timeout exceeds the default, add an inline comment explaining why."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/journey-page.ts` around lines 321 - 340, The retry loop around the journey card editor uses ninetySecondsTimeout for click waits (in the frameLocator chain and the .click call) which can multiply long waits; change those waits to a short, bounded timeout (e.g., 3–5s or the 30s default) so each retry fails fast, and if a longer timeout is truly required replace ninetySecondsTimeout with a named, justified constant and add an inline comment explaining why it must exceed the default; update references to journeyCardFrame, the locator for 'h3[data-testid="JourneysTypography"] textarea' and the .click call to use the new shorter timeout (or the new justified constant) and move expensive waits outside the retry loop where possible.
🧹 Nitpick comments (9)
apps/journeys-admin-e2e/src/pages/card-level-actions.ts (1)
236-240: Prefer a user-facing locator for the Custom upload panel.This wait improves the timing, but it still hard-codes
data-testid="ImageUpload"in a newly touched E2E path. If this panel has a visible label/button, anchor the wait to that instead so the page object follows the same contract as the new repo rule.As per coding guidelines, "Reference elements by the label the user sees in the UI using getByRole(), getByPlaceholder(), and similar accessibility queries — avoid data-testid selectors, CSS class selectors, and implementation-detail locators."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/card-level-actions.ts` around lines 236 - 240, Replace the hard-coded data-testid locator used to wait for the Custom upload panel in card-level-actions (currently this.page.locator('div[data-testid="ImageUpload"]')) with an accessibility-first query that targets the user-facing label or control for that panel (for example use getByRole/getByText/getByLabelText for the visible "Custom" tab/panel or its upload button), and use that locator in the await expect(...).toBeVisible({ timeout: sixtySecondsTimeout }) call so the page object follows the repo rule of referencing UI-visible labels rather than data-testid/implementation selectors.apps/journeys-admin-e2e/src/e2e/discover/journey-sorting.spec.ts (2)
49-54: Minor: Redundant page close inafterAll.Since
afterEachalready closesonboardingPageand sets it toundefined, line 50 is redundant.♻️ Suggested simplification
test.afterAll(async () => { - if (onboardingPage != null) await onboardingPage.close() if (sharedContext != null) await sharedContext.close() - onboardingPage = undefined sharedContext = undefined })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/e2e/discover/journey-sorting.spec.ts` around lines 49 - 54, In test.afterAll, remove the redundant call that closes onboardingPage (the if (onboardingPage != null) await onboardingPage.close()) because onboardingPage is already closed and cleared in afterEach; keep only the cleanup for sharedContext (if (sharedContext != null) await sharedContext.close()) and then set onboardingPage = undefined and sharedContext = undefined to ensure both variables are cleared.
7-14: Inconsistent variable naming: usesharedPagefor consistency.Other specs in this PR use
sharedPagefor the module-level page variable. UsingonboardingPagehere creates inconsistency and potential confusion.♻️ Suggested rename for consistency
-let onboardingPage: Page | undefined +let sharedPage: Page | undefined let sharedContext: BrowserContext | undefined const getSharedPage = (): Page => { - if (onboardingPage == null) + if (sharedPage == null) throw new Error('Shared authenticated page was not initialized') - return onboardingPage + return sharedPage }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/e2e/discover/journey-sorting.spec.ts` around lines 7 - 14, Rename the module-level variable onboardingPage to sharedPage and update all references (including inside getSharedPage) so getSharedPage returns sharedPage; ensure the variable declaration signature (let onboardingPage: Page | undefined) is updated to let sharedPage: Page | undefined and the error message in getSharedPage remains appropriate, and verify sharedContext usage remains unchanged.apps/journeys-admin-e2e/src/e2e/profile/profile.spec.ts (1)
36-48: Minor: Redundant page close inafterAll.Since
afterEachalready closescurrentPageand sets it toundefined, the check inafterAllat line 44 is redundant and will never execute. Consider simplifying:♻️ Suggested simplification
test.afterAll(async () => { - if (currentPage != null) await currentPage.close() if (sharedContext != null) await sharedContext.close() - currentPage = undefined sharedContext = undefined })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/e2e/profile/profile.spec.ts` around lines 36 - 48, The afterAll block contains a redundant check that attempts to close currentPage even though test.afterEach already closes and sets currentPage to undefined; update test.afterAll (the function named in the diff) to remove the "if (currentPage != null) await currentPage.close()" check and simply close sharedContext if present and then set currentPage = undefined and sharedContext = undefined to keep cleanup concise and deterministic.apps/journeys-admin-e2e/src/e2e/discover/members.spec.ts (1)
46-50: Remove empty destructuring parameter.The empty
{}destructuring is unnecessary since the test doesn't use any fixtures.♻️ Suggested fix
- test('Add a member through member option in discover page', async ({}) => { + test('Add a member through member option in discover page', async () => { const page = getSharedPage()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/e2e/discover/members.spec.ts` around lines 46 - 50, The test callback includes an unnecessary empty destructured parameter; change the test declaration from test('Add a member...', async ({}) => { ... }) to test('Add a member...', async () => { ... }) so the async function takes no arguments—leave the body using getSharedPage() and TeamsPage.verifyMemberAddedViaMemberOptionOfThreeDotOptions() unchanged and run lint/tests to confirm no other fixture usage is expected.apps/journeys-admin-e2e/src/pages/teams-page.ts (1)
146-154: Navigation synchronization pattern looks good.The
Promise.allpattern correctly coordinates the click with URL navigation. The 90s timeout is at the hard limit but justified for cold SSR scenarios.Consider using an accessibility query instead of
data-testidfor the button:♻️ Suggested refactor using getByRole
async clickCreateJourneyBtn() { await Promise.all([ this.page.waitForURL(journeyEditorUrlRegex, { timeout: ninetySecondsTimeout, waitUntil: 'commit' }), - this.page.locator('button[data-testid="AddJourneyButton"]').click() + this.page.getByRole('button', { name: /add journey/i }).click() ]) }As per coding guidelines: "Reference elements by the label the user sees in the UI using getByRole(), getByPlaceholder(), and similar accessibility queries — avoid data-testid selectors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/teams-page.ts` around lines 146 - 154, In clickCreateJourneyBtn the locator uses a data-testid selector; change it to an accessibility query so tests reference the UI label instead—replace this.page.locator('button[data-testid="AddJourneyButton"]') with an accessible query (e.g., getByRole('button', { name: /Add Journey/i } ) or a suitable visible label) while keeping the same Promise.all navigation pattern and ninetySecondsTimeout; ensure the role/label matches what the user sees so navigation still waits for journeyEditorUrlRegex as before.apps/journeys-admin-e2e/src/e2e/discover/templated-journey.spec.ts (1)
41-46: Remove empty destructuring parameter.♻️ Suggested fix
- test('Assert that See all link & See all templates button have a href to /templates', async ({}) => { + test('Assert that See all link & See all templates button have a href to /templates', async () => { const page = getSharedPage()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/e2e/discover/templated-journey.spec.ts` around lines 41 - 46, The test declaration uses an unnecessary empty destructured parameter "async ({})" — change the test's async parameter to a plain empty parameter "async ()" in the test that calls getSharedPage(), new JourneyPage(page), verifySeeLinkHrefAttributeBesideUseTemplate and verifySeeAllTemplateBelowUseTemplate so you remove the unused destructuring and avoid linter/errors from an unused parameter.apps/journeys-admin-e2e/src/e2e/customization/youtube-video.spec.ts (1)
83-89: Prefer accessibility query over CSS class selector.The assertion uses
.MuiFilledInput-rootwhich is an implementation detail. Consider asserting on user-visible attributes instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/e2e/customization/youtube-video.spec.ts` around lines 83 - 89, The test currently asserts styling via the implementation class .MuiFilledInput-root; update the assertion to use an accessibility-focused query/attribute instead: locate the input using field.getByRole('textbox') (already present) and assert a user-visible attribute that indicates the filled variant—for example check placeholder/aria-label/text content or value (e.g. expect(field.getByRole('textbox')).toHaveAttribute('placeholder', '...') or toHaveValue(...)) rather than using locator('.MuiFilledInput-root'); modify the test in the 'should use filled variant styling on YouTube input' test to replace the CSS selector assertion with the appropriate accessible-attribute assertion on the textbox.apps/journeys-admin-e2e/src/e2e/discover/card-level-actions-footer.spec.ts (1)
30-35: Drop the extra root navigation frombeforeEach.
clickCreateCustomJourney()already owns the Discover navigation and editor URL wait. Keepingawait page.goto('/')here adds an extra, unvalidated route change before setup and can reintroduce the root-route timing path this PR is trying to remove.♻️ Suggested cleanup
test.beforeEach(async () => { const page = getSharedPage() const cardLevelActionPage = new CardLevelActionPage(page) const journeyPage = new JourneyPage(page) - await page.goto('/') await journeyPage.clickCreateCustomJourney() await cardLevelActionPage.waitUntilJourneyCardLoaded() await cardLevelActionPage.clickOnJourneyCard()Based on learnings: "Write automation the way a tester would exercise the product: follow real end-user flows in order, with explicit checks that each step and each screen match expectations. Avoid loose branching, silent skips, or merged paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/e2e/discover/card-level-actions-footer.spec.ts` around lines 30 - 35, The beforeEach setup currently performs an extra root navigation (await page.goto('/')) before delegating navigation to JourneyPage.clickCreateCustomJourney(), which duplicates routing and can reintroduce the root-route timing issue; remove the page.goto('/') call from the test.beforeEach so the setup only constructs getSharedPage(), CardLevelActionPage, JourneyPage and then calls await journeyPage.clickCreateCustomJourney() (leaving getSharedPage, CardLevelActionPage, JourneyPage and clickCreateCustomJourney as the referenced symbols).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/journeys-admin-e2e/README.md`:
- Line 45: Add a language identifier (e.g., text) to the fenced code blocks in
README.md that are currently just triple backticks (the blocks flagged by
markdownlint MD040 at the locations noted), so change ``` to ```text for both
occurrences (including the one referenced at line 102) to satisfy markdownlint
without altering content.
In `@apps/journeys-admin-e2e/src/e2e-constants.ts`:
- Around line 1-5: The regex journeyEditorUrlRegex currently matches any path
that starts with /journeys/:id or /journeys/:id/edit and can falsely succeed for
longer routes; update journeyEditorUrlRegex to require the editor route to be
the end of the path while still allowing optional query or hash by anchoring the
pattern so it only matches when the path ends or is followed by ? or # (i.e.,
change the pattern used by journeyEditorUrlRegex to assert end-of-path with
optional query/hash).
In `@apps/journeys-admin-e2e/src/e2e/customization/youtube-video.spec.ts`:
- Around line 16-27: Remove the try/catch that conditionally skips the test in
the test.beforeEach hook: delete the catch block that checks for
MediaCustomizeStepUnavailableError and the test.skip(true, ...) call so that
calls to CustomizationMediaPage.navigateToMediaScreen (invoked after
navigateToCustomize with TEMPLATE_ID) will surface errors instead of hiding
them; leave the test.beforeEach to simply instantiate CustomizationMediaPage,
call navigateToCustomize(TEMPLATE_ID) and await navigateToMediaScreen(), and if
this flow legitimately shouldn't reach a media step use a different TEMPLATE_ID
or separate spec rather than conditional skipping.
In
`@apps/journeys-admin-e2e/src/e2e/discover/journey-level-actions-discover.spec.ts`:
- Around line 94-117: The test uses a 120s timeout and a hard sleep which
violate E2E guardrails; change test.setTimeout(120000) to at most 90000 (or
remove it to use the default) and remove the hard wait await
journeyLevelActions.sleep(2000); instead, wait for the UI to reflect the saved
language using a soft wait — for example call a helper that waits for the
language value to appear (e.g. await
journeyLevelActions.waitForLanguageSaved('Adi') or await
journeyPage.waitForSelector(...language-display-locator...,'visible') or await
page.waitForSelector(selectorForLanguageValue('Adi')) — then continue with the
existing clickThreeDotOptionsOfJourneyCreationPage and
verifySelectedLanguageInLanguagePopup calls so the test no longer relies on
fixed sleeps or >90s timeout.
- Around line 119-133: The test clicks "Copy Link" but never verifies the
result; update the spec after clickThreeDotOptionsOfJourneyCreationPage('Copy
Link') to assert the copy actually occurred — either check the clipboard
contains the journey URL or assert the copy-success snackbar/toast is visible
and contains the expected message. Use existing helpers similar to the share
specs (e.g., reading navigator.clipboard or the app's snackbar selector) and add
the assertion immediately after clickThreeDotOptionsOfJourneyCreationPage; keep
all setup calls (getSharedPage, getSharedContext,
JourneyLevelActions.setBrowserContext, JourneyPage.createAndVerifyCustomJourney,
JourneyPage.getJourneyName, JourneyLevelActions.selectCreatedJourney,
JourneyPage.clickThreeDotBtnOfCustomJourney) unchanged.
In
`@apps/journeys-admin-e2e/src/e2e/discover/journey-level-actions-share.spec.ts`:
- Around line 85-88: The test uses journeyLevelActions.verifySnackBarMsg('Link
copied') which mismatches the casing used elsewhere; update the expected
snackbar text to 'Link Copied' in the call to verifySnackBarMsg so it matches
other share-copy assertions (locate the invocation in the sequence around
journeyPage.editUrlAndSave(), journeyPage.clickCopyIconInShareDialog(),
verifySnackBarMsg(...), and journeyPage.verifyUpdatedUrlSlugIsLoaded(urlSlug)
and replace the lowercase string with the capitalized version).
In `@apps/journeys-admin-e2e/src/e2e/discover/teams.spec.ts`:
- Around line 29-48: The suite currently reuses sharedPage across tests
(initialized in test.beforeAll and torn down in test.afterAll), causing UI-state
leakage; change to keep the worker-scoped sharedContext but create a fresh Page
for each test by moving page creation into test.beforeEach (use
sharedContext.newPage()) and close that page in test.afterEach, update
getSharedPage() to return the per-test page instance instead of sharedPage, and
remove/adjust the now-unnecessary page creation/closure in
test.beforeAll/test.afterAll so each test runs with a clean Page.
In `@apps/journeys-admin-e2e/src/fixtures/workerAuth.ts`:
- Around line 83-108: The code mutates Playwright's typed rawState.origins
causing TS2339; instead create a new storage-state object: map rawState.origins
to a new array (e.g., newOrigins) where for each origin you spread the existing
origin and set sessionStorage: [{ name: SS_KEY, value: teamId }] when teamId is
provided and not '__shared__', and if rawState.origins is empty push a
constructed origin object ({ origin: appOrigin, localStorage: [],
sessionStorage: [...] }) using page.evaluate to get appOrigin; finally write
promises.writeFile(storagePath, JSON.stringify({ ...rawState, origins:
newOrigins })) so you never assign to the Playwright-typed origin objects
directly.
In `@apps/journeys-admin-e2e/src/pages/customization-media-page.ts`:
- Around line 87-93: waitForAutoSubmit() currently waits on a static helper
paragraph and page.load state which can be true before the auto-submit finishes;
change it to wait for a post-submit signal instead (e.g., the success/error
helper text, the preview/render element, or the normalized input value) so the
function only resolves after auto-submit completes. Specifically, in
waitForAutoSubmit replace the generic
this.page.getByTestId('VideosSection-youtube-input').locator('p') +
page.waitForLoadState('load') checks with one of: waiting for the specific
success/error helper text via getByText or a dedicated test id, waiting for the
preview/render container to become visible, or polling the input's value
(getByTestId('VideosSection-youtube-input') element) until it equals the
expected normalized value; use page.waitForSelector or
page.waitForFunction/page.waitForResponse as appropriate to ensure the submit
finished before returning.
- Around line 17-23: The helper navigateToMediaScreen (and getScreenFromUrl)
currently branches on DOM/URL state and swallows URL-parse failures, creating
multiple hidden E2E flows; make it deterministic by splitting responsibilities:
(1) change getScreenFromUrl to surface parse errors (remove the silent try/catch
so it throws on invalid URLs) or return a non-ambiguous value so callers fail
fast, and (2) replace the branching navigateToMediaScreen with two explicit
helpers (e.g., navigateToMediaScreenWhenPresent and
navigateToMediaScreenWhenMissing) or remove the conditional logic so a single
helper follows one predetermined path and any unexpected state triggers an
assertion/failure; update tests/fixtures to call the appropriate helper for each
scenario rather than relying on runtime branching.
In `@apps/journeys-admin-e2e/src/pages/journey-page.ts`:
- Around line 275-280: The current try/catch around the createBtn readiness
silently recovers by calling this.page.reload(), masking real failures in
gotoDiscoverJourneysPage(); remove the try/catch and reload branch and instead
assert deterministically (for example replace the whole block with a single
assertion like expect(createBtn).toBeEnabled({ timeout: 90000 })) so failures
surface loudly; update any references in gotoDiscoverJourneysPage()/createBtn
usage accordingly and do not introduce new conditional or recovery branches.
In `@apps/journeys-admin-e2e/src/pages/login-page.ts`:
- Around line 33-44: The readiness check currently uses sharedWithMeSelect
(getByRole('combobox').filter({ hasText: 'Shared With Me' })) which fails when
workerAuth preserves a different active team; change the check to wait for a
discover-page signal that is valid for both states instead of a specific label —
for example, wait for the TeamSelect control without filtering by text (a
team-select/getByRole('combobox') locator), or wait for a discover-specific
element (e.g., a discover-page test-id or heading) to be visible or for the
discover page URL, replacing the try/catch reload branch; update locators that
reference sharedWithMeSelect and ensure workerAuth path is covered by the new
wait.
In `@apps/journeys-admin-e2e/src/pages/register-Page.ts`:
- Around line 153-164: The current verifyTermsAcceptedAndPersisted method
incorrectly treats "any URL except terms-and-conditions" as success and uses a
120000ms wait; change it to wait explicitly for the expected post-terms Discover
screen (e.g., wait for the known Discover page URL or a stable Discover page
element by using waitUntilDiscoverPageLoaded or a specific locator) instead of
the negative predicate in page.waitForURL, keep the secondary assertion await
expect(this.page).not.toHaveURL(/terms-and-conditions/) and reduce the explicit
timeout to <=90000ms (prefer using the existing thirtySecondsTimeout or another
justified short timeout) so the function only proceeds when the real post-terms
target is reached.
---
Outside diff comments:
In `@apps/journeys-admin-e2e/src/pages/journey-level-actions-page.ts`:
- Around line 393-395: Remove the hard-wait helper sleep (async sleep(ms)) from
journey-level-actions-page and replace its single usage in the spec (await
journeyLevelActions.sleep(2000)) with a Playwright soft-wait: after the save
action, wait for a determinative UI/state change instead of sleeping — e.g.,
await expect(saveButton).toBeEnabled()/toHaveCount()/toBeVisible(), await
expect(successToast).toBeVisible(), await page.waitForResponse(r =>
r.url().includes('/save') && r.status() === 200), or await
page.waitForNavigation() depending on the app behavior; update the spec to use
one of these built-in waits so the test waits for the save to complete rather
than sleeping.
In `@apps/journeys-admin-e2e/src/pages/journey-page.ts`:
- Around line 321-340: The retry loop around the journey card editor uses
ninetySecondsTimeout for click waits (in the frameLocator chain and the .click
call) which can multiply long waits; change those waits to a short, bounded
timeout (e.g., 3–5s or the 30s default) so each retry fails fast, and if a
longer timeout is truly required replace ninetySecondsTimeout with a named,
justified constant and add an inline comment explaining why it must exceed the
default; update references to journeyCardFrame, the locator for
'h3[data-testid="JourneysTypography"] textarea' and the .click call to use the
new shorter timeout (or the new justified constant) and move expensive waits
outside the retry loop where possible.
---
Nitpick comments:
In `@apps/journeys-admin-e2e/src/e2e/customization/youtube-video.spec.ts`:
- Around line 83-89: The test currently asserts styling via the implementation
class .MuiFilledInput-root; update the assertion to use an accessibility-focused
query/attribute instead: locate the input using field.getByRole('textbox')
(already present) and assert a user-visible attribute that indicates the filled
variant—for example check placeholder/aria-label/text content or value (e.g.
expect(field.getByRole('textbox')).toHaveAttribute('placeholder', '...') or
toHaveValue(...)) rather than using locator('.MuiFilledInput-root'); modify the
test in the 'should use filled variant styling on YouTube input' test to replace
the CSS selector assertion with the appropriate accessible-attribute assertion
on the textbox.
In `@apps/journeys-admin-e2e/src/e2e/discover/card-level-actions-footer.spec.ts`:
- Around line 30-35: The beforeEach setup currently performs an extra root
navigation (await page.goto('/')) before delegating navigation to
JourneyPage.clickCreateCustomJourney(), which duplicates routing and can
reintroduce the root-route timing issue; remove the page.goto('/') call from the
test.beforeEach so the setup only constructs getSharedPage(),
CardLevelActionPage, JourneyPage and then calls await
journeyPage.clickCreateCustomJourney() (leaving getSharedPage,
CardLevelActionPage, JourneyPage and clickCreateCustomJourney as the referenced
symbols).
In `@apps/journeys-admin-e2e/src/e2e/discover/journey-sorting.spec.ts`:
- Around line 49-54: In test.afterAll, remove the redundant call that closes
onboardingPage (the if (onboardingPage != null) await onboardingPage.close())
because onboardingPage is already closed and cleared in afterEach; keep only the
cleanup for sharedContext (if (sharedContext != null) await
sharedContext.close()) and then set onboardingPage = undefined and sharedContext
= undefined to ensure both variables are cleared.
- Around line 7-14: Rename the module-level variable onboardingPage to
sharedPage and update all references (including inside getSharedPage) so
getSharedPage returns sharedPage; ensure the variable declaration signature (let
onboardingPage: Page | undefined) is updated to let sharedPage: Page | undefined
and the error message in getSharedPage remains appropriate, and verify
sharedContext usage remains unchanged.
In `@apps/journeys-admin-e2e/src/e2e/discover/members.spec.ts`:
- Around line 46-50: The test callback includes an unnecessary empty
destructured parameter; change the test declaration from test('Add a member...',
async ({}) => { ... }) to test('Add a member...', async () => { ... }) so the
async function takes no arguments—leave the body using getSharedPage() and
TeamsPage.verifyMemberAddedViaMemberOptionOfThreeDotOptions() unchanged and run
lint/tests to confirm no other fixture usage is expected.
In `@apps/journeys-admin-e2e/src/e2e/discover/templated-journey.spec.ts`:
- Around line 41-46: The test declaration uses an unnecessary empty destructured
parameter "async ({})" — change the test's async parameter to a plain empty
parameter "async ()" in the test that calls getSharedPage(), new
JourneyPage(page), verifySeeLinkHrefAttributeBesideUseTemplate and
verifySeeAllTemplateBelowUseTemplate so you remove the unused destructuring and
avoid linter/errors from an unused parameter.
In `@apps/journeys-admin-e2e/src/e2e/profile/profile.spec.ts`:
- Around line 36-48: The afterAll block contains a redundant check that attempts
to close currentPage even though test.afterEach already closes and sets
currentPage to undefined; update test.afterAll (the function named in the diff)
to remove the "if (currentPage != null) await currentPage.close()" check and
simply close sharedContext if present and then set currentPage = undefined and
sharedContext = undefined to keep cleanup concise and deterministic.
In `@apps/journeys-admin-e2e/src/pages/card-level-actions.ts`:
- Around line 236-240: Replace the hard-coded data-testid locator used to wait
for the Custom upload panel in card-level-actions (currently
this.page.locator('div[data-testid="ImageUpload"]')) with an accessibility-first
query that targets the user-facing label or control for that panel (for example
use getByRole/getByText/getByLabelText for the visible "Custom" tab/panel or its
upload button), and use that locator in the await expect(...).toBeVisible({
timeout: sixtySecondsTimeout }) call so the page object follows the repo rule of
referencing UI-visible labels rather than data-testid/implementation selectors.
In `@apps/journeys-admin-e2e/src/pages/teams-page.ts`:
- Around line 146-154: In clickCreateJourneyBtn the locator uses a data-testid
selector; change it to an accessibility query so tests reference the UI label
instead—replace this.page.locator('button[data-testid="AddJourneyButton"]') with
an accessible query (e.g., getByRole('button', { name: /Add Journey/i } ) or a
suitable visible label) while keeping the same Promise.all navigation pattern
and ninetySecondsTimeout; ensure the role/label matches what the user sees so
navigation still waits for journeyEditorUrlRegex as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fbd684a8-276e-4759-b09d-6584b3e38b99
📒 Files selected for processing (27)
.cursor/rules/e2e-testing.mdc.github/workflows/e2e-tests.ymlapps/journeys-admin-e2e/README.mdapps/journeys-admin-e2e/playwright.config.tsapps/journeys-admin-e2e/src/e2e-constants.tsapps/journeys-admin-e2e/src/e2e/customization/youtube-video.spec.tsapps/journeys-admin-e2e/src/e2e/discover/active-archived-trash.spec.tsapps/journeys-admin-e2e/src/e2e/discover/analytics.spec.tsapps/journeys-admin-e2e/src/e2e/discover/card-level-actions-footer.spec.tsapps/journeys-admin-e2e/src/e2e/discover/card-level-actions.spec.tsapps/journeys-admin-e2e/src/e2e/discover/custom-journey.spec.tsapps/journeys-admin-e2e/src/e2e/discover/journey-level-actions-discover.spec.tsapps/journeys-admin-e2e/src/e2e/discover/journey-level-actions-share.spec.tsapps/journeys-admin-e2e/src/e2e/discover/journey-level-actions.spec.tsapps/journeys-admin-e2e/src/e2e/discover/journey-sorting.spec.tsapps/journeys-admin-e2e/src/e2e/discover/members.spec.tsapps/journeys-admin-e2e/src/e2e/discover/teams.spec.tsapps/journeys-admin-e2e/src/e2e/discover/templated-journey.spec.tsapps/journeys-admin-e2e/src/e2e/profile/profile.spec.tsapps/journeys-admin-e2e/src/fixtures/workerAuth.tsapps/journeys-admin-e2e/src/pages/card-level-actions.tsapps/journeys-admin-e2e/src/pages/customization-media-page.tsapps/journeys-admin-e2e/src/pages/journey-level-actions-page.tsapps/journeys-admin-e2e/src/pages/journey-page.tsapps/journeys-admin-e2e/src/pages/login-page.tsapps/journeys-admin-e2e/src/pages/register-Page.tsapps/journeys-admin-e2e/src/pages/teams-page.ts
💤 Files with no reviewable changes (1)
- .github/workflows/e2e-tests.yml
kiran-redhat
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/journeys-admin-e2e/src/pages/register-Page.ts (1)
156-161:⚠️ Potential issue | 🟠 MajorMake post-terms navigation strict and keep timeout within repo limits.
Line 158–160 currently treats “any URL except terms” as success and uses
120000ms. That can pass on the wrong destination and exceeds the 90s ceiling.Proposed fix
async verifyTermsAcceptedAndPersisted() { - // Wait for navigation away from Terms before checking the Discover page. - await this.page.waitForURL( - (url) => !url.toString().includes('terms-and-conditions'), - { timeout: 120000 } - ) + // 90s: cold SSR + initial auth data load can delay first Discover render. + // Wait for an explicit Discover signal instead of "not terms". await this.waitUntilDiscoverPageLoaded() await this.page.reload({ waitUntil: 'domcontentloaded' }) await expect(this.page).not.toHaveURL(/terms-and-conditions/, { timeout: thirtySecondsTimeout }) }As per coding guidelines, "
apps/**/*e2e*/**/*.ts: Validate the destination screen before acting..." and "Hard limit is 90s (90000ms)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/register-Page.ts` around lines 156 - 161, In verifyTermsAcceptedAndPersisted, replace the loose waitForURL predicate that only asserts the URL does not include 'terms-and-conditions' with a strict positive check for the expected post-terms destination (e.g., assert the URL includes the Discover route segment such as 'discover' or the exact expected path) and reduce the timeout to the repo hard limit (<=90000ms, e.g., 90000). Update the waitForURL call in that method to wait for the specific destination string and set timeout: 90000.
🧹 Nitpick comments (1)
apps/journeys-admin-e2e/src/pages/register-Page.ts (1)
195-197: Prefer user-visible role query for the Terms “Next” button.Using
data-testidhere couples the test to implementation details; this should use the user-visible label.Proposed refactor
- const nextButton = termsRoot.locator( - 'button[data-testid="TermsAndConditionsNextButton"]' - ) + const nextButton = termsRoot.getByRole('button', { name: 'Next' })As per coding guidelines, "
apps/**/*e2e*/**/*.ts: Reference elements by the label the user sees in the UI ... Avoid data-testid, CSS class names, or implementation-detail selectors."Also applies to: 333-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/register-Page.ts` around lines 195 - 197, The test is selecting the Terms "Next" button using an implementation-only selector; update the locator to query by the user-visible role/label instead: replace the locator call that assigns nextButton (and the similar locator at the other occurrence around lines 333-335) to use a role/label-based query on termsRoot (e.g., use getByRole / locator with role: 'button' and name: 'Next' or the exact visible text) so the test targets the button by what the user sees rather than data-testid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/journeys-admin-e2e/src/pages/register-Page.ts`:
- Around line 149-154: The helper completeCreateYourWorkspaceStep asserts the
wrong next page after clickCreateBtn: it calls assertOnTermsAndConditionsPage
but should validate the workspace-completion destination; update
completeCreateYourWorkspaceStep to call the appropriate post-creation assertion
(e.g., assertOnWorkspaceCreatedPage / assertOnWorkspaceCompletionPage /
assertOnWorkspaceDashboardPage) instead of assertOnTermsAndConditionsPage and
ensure the destination is validated before any further actions; keep the
preceding calls to assertOnCreateYourWorkspacePage, entetTeamName, and
clickCreateBtn unchanged.
- Around line 137-139: Add a short inline comment above the await
expect(this.page).toHaveURL(/terms-and-conditions/, { timeout: 90000 }) call
explaining why a 90s timeout is needed (follow the project's existing comment
pattern), e.g., note that first-load can be slow due to cold Vercel SSR or
initial Apollo queries on daily-e2e runs; place the comment immediately above
the toHaveURL call so the intent is clear when reading register-Page.ts and
references to this.page/toHaveURL are obvious.
---
Duplicate comments:
In `@apps/journeys-admin-e2e/src/pages/register-Page.ts`:
- Around line 156-161: In verifyTermsAcceptedAndPersisted, replace the loose
waitForURL predicate that only asserts the URL does not include
'terms-and-conditions' with a strict positive check for the expected post-terms
destination (e.g., assert the URL includes the Discover route segment such as
'discover' or the exact expected path) and reduce the timeout to the repo hard
limit (<=90000ms, e.g., 90000). Update the waitForURL call in that method to
wait for the specific destination string and set timeout: 90000.
---
Nitpick comments:
In `@apps/journeys-admin-e2e/src/pages/register-Page.ts`:
- Around line 195-197: The test is selecting the Terms "Next" button using an
implementation-only selector; update the locator to query by the user-visible
role/label instead: replace the locator call that assigns nextButton (and the
similar locator at the other occurrence around lines 333-335) to use a
role/label-based query on termsRoot (e.g., use getByRole / locator with role:
'button' and name: 'Next' or the exact visible text) so the test targets the
button by what the user sees rather than data-testid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 106d5cd5-ef7e-4878-b3a2-a6fc48e4d7db
📒 Files selected for processing (1)
apps/journeys-admin-e2e/src/pages/register-Page.ts
…olation and clarity - Refactored login tests to utilize a fresh storage state for each session, ensuring no shared state between tests. - Updated the `LoginPage` class to include a new method for signing in with email and password, improving code clarity. - Added a new test for handling user onboarding that stops at the Terms and Conditions page, verifying the correct flow for users who leave before acceptance. - Enhanced the `Register` class with a method for registering accounts through OTP, stopping at the Terms page for better test coverage. - Improved comments throughout the code to clarify the purpose and flow of tests and methods.
kiran-redhat
changed the title
Contributor
|
The latest updates on your projects.
|
… and assertions - Added detailed guidelines for handling `sessionStorage` and `storageState` in Playwright tests to ensure accurate session management. - Updated regex for journey editor routes to prevent false matches on longer paths. - Implemented best practices for closing `BrowserContext`s to prevent resource leaks during tests. - Enhanced assertions for post-onboarding and post-terms scenarios to align with actual application behavior. - Improved visibility checks and error handling in various test cases, ensuring robustness and clarity in test outcomes. - Refactored multiple test files to utilize a shared context for better isolation and reliability.
…e2e-one-user-per-worker
- Updated various dependencies to their latest versions for better performance and security. - Refactored project structure to enhance maintainability and clarity. - Removed deprecated files and streamlined the build process.
kiran-redhat
removed request for
mikeallisonJS,
sharon-elsa-mathew1995 and
tataihono
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Journeys-Admin tests passing:
Journeys tests passing:
Summary by CodeRabbit