Add initial Playwright E2E tests for Client module#3355
Open
Tushar-Jawale wants to merge 1 commit intoopenMF:devfrom
Open
Add initial Playwright E2E tests for Client module#3355Tushar-Jawale wants to merge 1 commit intoopenMF:devfrom
Tushar-Jawale wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Playwright Client Page playwright/pages/client.page.ts |
Adds ClientPage (extends BasePage) with url = '/#/clients', ~18 locator getters (search, table, form fields, headings, paginator) and ~10 async methods (navigateToClients, waitForLoad, searchClient, openCreateClientForm, fillClientForm, submitClientForm, openClientProfile, and visibility assertions). Uses explicit waits and network-idle fallbacks. |
Playwright Tests playwright/tests/client.spec.ts |
Adds E2E test suite (skipped on CI) with beforeEach login and three tests: client list visibility, open-create-client form (with URL assertion), and open client profile via search + assertions. Relies on ClientPage methods and explicit waits. |
Sequence Diagram(s)
sequenceDiagram
participant TestRunner as Test Runner
participant ClientPage as ClientPage
participant Browser as Browser
participant Backend as Backend
TestRunner->>ClientPage: navigateToClients()
ClientPage->>Browser: goto("/#/clients")
Browser->>Backend: load app + API requests
Backend-->>Browser: app bundle + data
Browser-->>ClientPage: page loaded / network idle
TestRunner->>ClientPage: openCreateClientForm()
ClientPage->>Browser: click(createClientButton)
Browser-->>ClientPage: create form visible
TestRunner->>ClientPage: fillClientForm(first,last)
ClientPage->>Browser: fill inputs
TestRunner->>ClientPage: submitClientForm()
ClientPage->>Browser: click(submit)
Browser->>Backend: POST /clients
Backend-->>Browser: created + redirect
Browser-->>ClientPage: client detail loaded
TestRunner->>ClientPage: assertClientDetailVisible()
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
- alberto-art3ch
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title directly and accurately describes the main objective: adding initial Playwright E2E tests for the Client module, which matches both files added (ClientPage and client.spec.ts). |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📝 Coding Plan
- Generate coding plan for human review comments
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
playwright/tests/client.spec.ts (1)
26-27: Worth planning a backend-enabled CI job for this suite.Because the whole suite is skipped whenever
CIis set, these tests won’t catch regressions in the main pipeline. A dedicated E2E job with a seeded Fineract backend would make this coverage actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/tests/client.spec.ts` around lines 26 - 27, The tests are currently always skipped in CI because of the test.skip(!!process.env.CI, 'Requires Fineract backend') guard; instead add a dedicated CI E2E job that provisions/seed a Fineract backend and sets a specific enabling env (e.g., FINERACT_URL or RUN_FINERACT_E2E=true), then change the guard to skip only when that env is absent (for example, replace test.skip(!!process.env.CI, ...) with test.skip(!process.env.FINERACT_URL, 'Requires Fineract backend') or similar) so the suite runs in the new backend-enabled job while remaining skipped in normal CI runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playwright/pages/client.page.ts`:
- Around line 52-55: The locator firstClientNameCell is brittle because it
selects the first td by position; update it to target the actual navigable name
cell (e.g., the td that carries the routerLink or the cell containing the
display name) so it won’t break if columns shift. Modify the getter
firstClientNameCell to select the table cell with the [routerLink] attribute or
use a content-based locator for the displayName, and update openClientProfile()
to use that locator instead of relying on positional td selection.
- Around line 130-137: Modify waitForLoad() to wait for the page's searchInput
element to be visible instead of silently swallowing a networkidle timeout:
after waiting for 'domcontentloaded', catch networkidle timeout but then await
this.searchInput.waitFor({ state: 'visible', timeout: X }) to ensure the UI is
ready. In searchClient(), after triggering the search, replace the swallowed
timeout behavior with an explicit wait for either clientRows or
noClientFoundMessage to be visible (use something like Promise.race between
this.clientRows.waitFor(...) and this.noClientFoundMessage.waitFor(...)) so the
method only returns once results or the empty state are present. Keep the
pattern used in openClientProfile() (waiting for navigation and a specific
heading) as the model for these waits.
In `@playwright/tests/client.spec.ts`:
- Around line 56-65: Make the test self-contained by seeding a known test client
during Arrange (use your API helper or a backend fixture call) before calling
clientPage.navigateToClients(); then Act by calling
clientPage.searchClient('UniqueTestClientName') (use that seeded name) instead
of an empty search and remove the fixed await page.waitForTimeout(3000); finally
Assert by replacing brittle sleeps with state-based waits — e.g., wait for
clientPage.clientRows.first() toBeVisible and verify the results actually
populated (toHaveCount or expect(...).toBeVisible) before calling
clientPage.openClientProfile(), then call clientPage.assertClientDetailVisible()
to complete the assertion; reference functions: clientPage.navigateToClients,
clientPage.searchClient, clientPage.clientRows, clientPage.openClientProfile,
clientPage.assertClientDetailVisible.
---
Nitpick comments:
In `@playwright/tests/client.spec.ts`:
- Around line 26-27: The tests are currently always skipped in CI because of the
test.skip(!!process.env.CI, 'Requires Fineract backend') guard; instead add a
dedicated CI E2E job that provisions/seed a Fineract backend and sets a specific
enabling env (e.g., FINERACT_URL or RUN_FINERACT_E2E=true), then change the
guard to skip only when that env is absent (for example, replace
test.skip(!!process.env.CI, ...) with test.skip(!process.env.FINERACT_URL,
'Requires Fineract backend') or similar) so the suite runs in the new
backend-enabled job while remaining skipped in normal CI runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfad6d72-0f5d-4578-9ca8-bd1c887bb3e0
📒 Files selected for processing (2)
playwright/pages/client.page.tsplaywright/tests/client.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
playwright/pages/client.page.ts (1)
178-185: Consider waiting forlastNameInputvisibility for robustness.The method waits for
firstNameInputbut notlastNameInputbefore interacting with it. While Angular Material forms typically render all fields synchronously, explicitly waiting forlastNameInputwould make the test more resilient to future form restructuring or lazy-loaded form sections.♻️ Optional improvement
async fillClientForm(firstName: string, lastName: string): Promise<void> { await this.firstNameInput.waitFor({ state: 'visible', timeout: 10000 }); await this.firstNameInput.clear(); await this.firstNameInput.fill(firstName); + await this.lastNameInput.waitFor({ state: 'visible', timeout: 10000 }); await this.lastNameInput.clear(); await this.lastNameInput.fill(lastName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/pages/client.page.ts` around lines 178 - 185, In fillClientForm, add an explicit wait for lastNameInput visibility before interacting with it: after awaiting firstNameInput.waitFor(...) in the fillClientForm method, call await this.lastNameInput.waitFor({ state: 'visible', timeout: 10000 }) (or the same timeout used for firstNameInput) so the subsequent this.lastNameInput.clear() and this.lastNameInput.fill(lastName) operate on a visible element; this uses the existing firstNameInput and lastNameInput selectors in the fillClientForm function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@playwright/pages/client.page.ts`:
- Around line 178-185: In fillClientForm, add an explicit wait for lastNameInput
visibility before interacting with it: after awaiting
firstNameInput.waitFor(...) in the fillClientForm method, call await
this.lastNameInput.waitFor({ state: 'visible', timeout: 10000 }) (or the same
timeout used for firstNameInput) so the subsequent this.lastNameInput.clear()
and this.lastNameInput.fill(lastName) operate on a visible element; this uses
the existing firstNameInput and lastNameInput selectors in the fillClientForm
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdc0a8be-431d-4786-8924-e42b4d5b1b8d
📒 Files selected for processing (2)
playwright/pages/client.page.tsplaywright/tests/client.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright/tests/client.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
playwright/pages/client.page.ts (1)
118-124: UseBasePage.navigate()to avoid duplicating navigation logic.
navigateToClients()repeats the samegoto + waitForLoadflow already centralized inplaywright/pages/BasePage.ts(Line 46-53). Reusing it keeps timeout behavior and load policy consistent.♻️ Suggested refactor
async navigateToClients(): Promise<void> { - await this.page.goto(this.url, { - waitUntil: 'load', - timeout: 60000 - }); - await this.waitForLoad(); + await this.navigate(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/pages/client.page.ts` around lines 118 - 124, The navigateToClients method duplicates the goto + waitForLoad logic already implemented in BasePage.navigate; update navigateToClients to call the shared navigation method instead of repeating page.goto and waitForLoad. Replace the body of navigateToClients to simply await this.navigate() (or super.navigate() if needed based on inheritance) so the timeout and load policy from BasePage.navigate are reused; keep the method signature navigateToClients(): Promise<void> intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playwright/pages/client.page.ts`:
- Around line 145-158: searchClient currently races against
clientRows.first().waitFor which can be satisfied by pre-search rows; capture
the pre-search state and await a real change: before filling searchInput read
the current count (this.clientRows.count()) into a variable, then after pressing
Enter replace the Promise.race with logic that waits either for
noClientFoundMessage to become visible or for this.clientRows.count() to differ
from the captured previousCount (with the existing timeouts). Reference
searchClient, this.clientRows, this.noClientFoundMessage and this.searchInput
when locating where to add the previousCount capture and the updated wait.
---
Nitpick comments:
In `@playwright/pages/client.page.ts`:
- Around line 118-124: The navigateToClients method duplicates the goto +
waitForLoad logic already implemented in BasePage.navigate; update
navigateToClients to call the shared navigation method instead of repeating
page.goto and waitForLoad. Replace the body of navigateToClients to simply await
this.navigate() (or super.navigate() if needed based on inheritance) so the
timeout and load policy from BasePage.navigate are reused; keep the method
signature navigateToClients(): Promise<void> intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69648d08-82ea-4306-a85d-6b5839b15d7e
📒 Files selected for processing (1)
playwright/pages/client.page.ts
Comment on lines
+145
to
+158
| async searchClient(name: string): Promise<void> { | ||
| await this.searchInput.click(); | ||
| await this.searchInput.fill(name); | ||
| await this.searchInput.press('Enter'); | ||
| try { | ||
| await this.page.waitForLoadState('networkidle', { timeout: 10000 }); | ||
| } catch (e) { | ||
| console.log('Network idle timeout after search - proceeding anyway'); | ||
| } | ||
| // Wait for observable results: either rows appear or "no client found" message | ||
| await Promise.race([ | ||
| this.clientRows.first().waitFor({ state: 'visible', timeout: 15000 }), | ||
| this.noClientFoundMessage.waitFor({ state: 'visible', timeout: 15000 }) | ||
| ]); |
There was a problem hiding this comment.
searchClient() can return before filtered results are actually ready.
clientRows.first().waitFor({ state: 'visible' }) may already be true from the pre-search table state, so Promise.race can resolve too early and downstream clicks can target stale rows.
🔧 Suggested fix
async searchClient(name: string): Promise<void> {
await this.searchInput.click();
await this.searchInput.fill(name);
await this.searchInput.press('Enter');
try {
await this.page.waitForLoadState('networkidle', { timeout: 10000 });
} catch (e) {
console.log('Network idle timeout after search - proceeding anyway');
}
- // Wait for observable results: either rows appear or "no client found" message
+ const matchingClientNameCell = this.page
+ .locator('table.bordered-table tbody tr td.mat-column-displayName')
+ .filter({ hasText: name })
+ .first();
+
+ // Wait for concrete search outcome: matching row or empty-state message
await Promise.race([
- this.clientRows.first().waitFor({ state: 'visible', timeout: 15000 }),
- this.noClientFoundMessage.waitFor({ state: 'visible', timeout: 15000 })
+ matchingClientNameCell.waitFor({ state: 'visible', timeout: 15000 }),
+ this.page.getByText(/No client was found/i).waitFor({ state: 'visible', timeout: 15000 })
]);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/pages/client.page.ts` around lines 145 - 158, searchClient
currently races against clientRows.first().waitFor which can be satisfied by
pre-search rows; capture the pre-search state and await a real change: before
filling searchInput read the current count (this.clientRows.count()) into a
variable, then after pressing Enter replace the Promise.race with logic that
waits either for noClientFoundMessage to become visible or for
this.clientRows.count() to differ from the captured previousCount (with the
existing timeouts). Reference searchClient, this.clientRows,
this.noClientFoundMessage and this.searchInput when locating where to add the
previousCount capture and the updated wait.
Tushar-Jawale
force-pushed
the
playwright-client-module-tests
branch
from
85e1af4 to
00c4999
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
playwright/pages/client.page.ts (1)
145-158:⚠️ Potential issue | 🟠 Major
searchClient()can still resolve on pre-search state.Line 156 waits on
this.clientRows.first()visibility, which is often already true before pressing Enter. The method can return early and downstream actions can use stale rows. This was flagged earlier and remains in the current code.Suggested fix
async searchClient(name: string): Promise<void> { + const previousCount = await this.clientRows.count(); + const previousFirstName = (await this.firstClientNameCell.textContent())?.trim() ?? ''; + await this.searchInput.click(); await this.searchInput.fill(name); await this.searchInput.press('Enter'); @@ - // Wait for observable results: either rows appear or "no client found" message - await Promise.race([ - this.clientRows.first().waitFor({ state: 'visible', timeout: 15000 }), - this.noClientFoundMessage.waitFor({ state: 'visible', timeout: 15000 }) - ]); + // Wait for a real post-search state change or empty-state message + await expect + .poll(async () => { + if (await this.noClientFoundMessage.isVisible()) return true; + const currentCount = await this.clientRows.count(); + if (currentCount !== previousCount) return true; + if (currentCount === 0) return false; + const currentFirstName = (await this.firstClientNameCell.textContent())?.trim() ?? ''; + return currentFirstName !== previousFirstName; + }, { timeout: 15000 }) + .toBe(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/pages/client.page.ts` around lines 145 - 158, searchClient currently races against an already-visible row (this.clientRows.first()) so it can return the pre-search state; fix by capturing the pre-search snapshot and then waiting for a change after submitting: record the current first-row identifier/text or row count before this.searchInput.press('Enter'), then after pressing Enter wait (via this.page.waitForFunction or a looped wait) until either this.noClientFoundMessage is visible or the first row's identifier/text or the row count has changed from the snapshot (with the same total timeout). Update the logic in searchClient to use that change-detection instead of blindly waiting for this.clientRows.first() visibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playwright/pages/client.page.ts`:
- Around line 132-136: The try/catch around this.page.waitForLoadState currently
swallows all exceptions; change it to only suppress Playwright timeout errors
and rethrow everything else: import or reference Playwright's TimeoutError
(e.g., playwright.errors.TimeoutError or TimeoutError) and in the catch check if
the thrown error is an instance of (or has name === 'TimeoutError') before
logging and continuing; if not, rethrow the error. Apply the same change to the
other catch block around the same waitForLoadState call so only TimeoutError is
swallowed and other Playwright failures propagate.
---
Duplicate comments:
In `@playwright/pages/client.page.ts`:
- Around line 145-158: searchClient currently races against an already-visible
row (this.clientRows.first()) so it can return the pre-search state; fix by
capturing the pre-search snapshot and then waiting for a change after
submitting: record the current first-row identifier/text or row count before
this.searchInput.press('Enter'), then after pressing Enter wait (via
this.page.waitForFunction or a looped wait) until either
this.noClientFoundMessage is visible or the first row's identifier/text or the
row count has changed from the snapshot (with the same total timeout). Update
the logic in searchClient to use that change-detection instead of blindly
waiting for this.clientRows.first() visibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: debeabeb-e02f-4bba-b737-24d5f12971fc
📒 Files selected for processing (2)
playwright/pages/client.page.tsplaywright/tests/client.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright/tests/client.spec.ts
Comment on lines
+132
to
+136
| try { | ||
| await this.page.waitForLoadState('networkidle', { timeout: 15000 }); | ||
| } catch (e) { | ||
| console.log('Network idle timeout on clients page - proceeding anyway'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the actual file content at the specified lines
cat -n playwright/pages/client.page.ts | sed -n '125,160p'Repository: openMF/web-app
Length of output: 1610
🏁 Script executed:
# Check the imports at the top of the file
head -20 playwright/pages/client.page.tsRepository: openMF/web-app
Length of output: 696
🌐 Web query:
Playwright test errors.TimeoutError how to catch timeout errors
💡 Result:
To catch Playwright timeout errors, you need to distinguish operation timeouts (catchable) from test runner timeouts (not really catchable).
1) Catch operation timeouts (click / waitFor / goto / assertions, etc.)
These throw playwright.errors.TimeoutError, which you can catch with try/catch and instanceof:
import playwright from 'playwright';
try {
await page.getByText('Foo').click({ timeout: 1000 });
} catch (e) {
if (e instanceof playwright.errors.TimeoutError) {
// handle timeout (fallback, retry, log, etc.)
return;
}
throw e; // not a timeout, rethrow
}This is the pattern shown in the official TimeoutError docs. [1]
2) “Test timeout of 30000ms exceeded” is a test-level timeout
If the entire test hits the Playwright Test timeout (default 30s), the runner fails the test; you generally should increase/adjust the test timeout instead of trying to catch it:
test.setTimeout(120_000)(per-test)timeout: 120_000(inplaywright.config.ts)
Playwright documents these test/expect/action timeout layers separately. [2]
Sources:
[1] Playwright API: TimeoutError (and instanceof playwright.errors.TimeoutError example) (playwright.dev)
[2] Playwright Test: Timeouts (test timeout vs expect/action/navigation timeouts) (playwright.dev)
Citations:
🏁 Script executed:
# Check if errors is used elsewhere in the codebase with `@playwright/test`
rg "from '@playwright/test'" -A 3 playwright/Repository: openMF/web-app
Length of output: 1775
🏁 Script executed:
# Look for TimeoutError usage pattern in existing code
rg "TimeoutError|errors\." playwright/ -B 2 -A 2Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Check playwright.config.ts to understand the test setup
cat playwright.config.ts 2>/dev/null || echo "File not found"Repository: openMF/web-app
Length of output: 2853
🌐 Web query:
@playwright/test errors.TimeoutError import
💡 Result:
TimeoutError is not exported from @playwright/test, so import { errors.TimeoutError } from '@playwright/test' won’t work.
Use one of these instead:
1) In Playwright Test (recommended): use the built-in playwright fixture
import { test } from '@playwright/test';
test('handles timeout', async ({ page, playwright }) => {
try {
await page.locator('text=Foo').click({ timeout: 100 });
} catch (e) {
if (e instanceof playwright.errors.TimeoutError) {
// handle timeout
}
}
});This is the pattern shown in the official docs (playwright.errors.TimeoutError). [1]
2) Import from the Playwright library package (not @playwright/test)
CommonJS
const playwright = require('playwright');
if (e instanceof playwright.errors.TimeoutError) { /* ... */ }[1]
If you paste the exact import line and the error message you’re getting, I can tell you which of the two fits your setup (ESM vs CJS, TS config, etc.).
Sources: [1]
🏁 Script executed:
# Check package.json for playwright dependency
cat package.json | grep -A 5 -B 5 "playwright"Repository: openMF/web-app
Length of output: 1101
🏁 Script executed:
# Check if there are other error handling patterns in the codebase
rg "instanceof.*Error" playwright/ -B 2 -A 2Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Look at BasePage to understand the inheritance pattern
cat -n playwright/pages/BasePage.ts | head -40Repository: openMF/web-app
Length of output: 1375
🏁 Script executed:
# Check if playwright package (not `@playwright/test`) is available
cat package.json | grep -i "\"playwright\""Repository: openMF/web-app
Length of output: 94
🏁 Script executed:
# Search for any playwright import from the main package (not `@playwright/test`)
rg "from 'playwright'" . --type tsRepository: openMF/web-app
Length of output: 40
🌐 Web query:
Playwright page object error handling TimeoutError import from playwright package
💡 Result:
JavaScript / TypeScript (Playwright “library” or @playwright/test)
TimeoutError isn’t imported as a top-level named export. Catch it via playwright.errors.TimeoutError (or errors.TimeoutError). (playwright.dev)
import * as playwright from 'playwright'; // or: const playwright = require('playwright');
try {
await page.locator('text=Foo').click({ timeout: 100 });
} catch (e) {
if (e instanceof playwright.errors.TimeoutError) {
// handle timeout
} else {
throw e;
}
}Alternative:
import { errors } from 'playwright';
try { /* ... */ }
catch (e) {
if (e instanceof errors.TimeoutError) { /* ... */ }
}Python (if that’s what you meant)
from playwright.sync_api import TimeoutError
# (or from playwright.async_api import TimeoutError)(Then except TimeoutError:.) (stackoverflow.com)
Citations:
- 1: https://playwright.dev/docs/api/class-timeouterror?utm_source=openai
- 2: https://stackoverflow.com/questions/66490575/how-can-i-handle-the-wait-for-selector-functions-timeouterror-in-playwright-pyt?utm_source=openai
Catch only timeout errors; rethrow other Playwright failures.
Lines 134–135 and 151–152 catch all exceptions and proceed, which masks real failures (page closed, navigation aborted, context errors, etc.). Only swallow TimeoutError and rethrow everything else.
Suggested fix
+import { errors } from 'playwright';
import { Locator, expect } from '@playwright/test';
@@
} catch (e) {
+ if (!(e instanceof errors.TimeoutError)) throw e;
console.log('Network idle timeout on clients page - proceeding anyway');
}
@@
} catch (e) {
+ if (!(e instanceof errors.TimeoutError)) throw e;
console.log('Network idle timeout after search - proceeding anyway');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/pages/client.page.ts` around lines 132 - 136, The try/catch around
this.page.waitForLoadState currently swallows all exceptions; change it to only
suppress Playwright timeout errors and rethrow everything else: import or
reference Playwright's TimeoutError (e.g., playwright.errors.TimeoutError or
TimeoutError) and in the catch check if the thrown error is an instance of (or
has name === 'TimeoutError') before logging and continuing; if not, rethrow the
error. Apply the same change to the other catch block around the same
waitForLoadState call so only TimeoutError is swallowed and other Playwright
failures propagate.
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
2 participants
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.
This PR introduces initial Playwright E2E test coverage for the Client module.
Changes included:
ClientPagepage object following the existing Page Object Model structure used by LoginPage.client.spec.ts.Current test coverage includes:
The implementation follows the existing Playwright architecture and locator strategy used in the login tests.
Jira Ticket: WEB-838
Summary by CodeRabbit