chore: update-flakey-tests #450
Conversation
|
WalkthroughMultiple e2e test files and OIDC app pages were adjusted to improve stability: assertions were wrapped with retryable patterns, navigation/wait synchronization was made sequential, DOM-ready mounting was deferred, UI loading indicators were added, and some TypeScript typings and ESLint Playwright config were introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Page as Browser Page
participant App as Ping-AM
rect rgba(58,135,173,0.06)
Note over Test,Page: Previous flaky pattern (raced wait + click)
Test-->>Page: Promise.all( waitForURL(...), click(selector) )
Page-->>Test: navigation completed (race)
end
rect rgba(106,168,79,0.06)
Note over Test,Page: New sequential pattern (stable)
Test->>Page: click(selector)
Page->>Test: (returns)
Test->>Page: await waitForURL(...)
Page-->>Test: navigation confirmed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
View your CI Pipeline Execution ↗ for commit 5856af2
☁️ Nx Cloud last updated this comment at |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
e2e/davinci-suites/src/basic.test.ts (1)
83-83: Inconsistent retry pattern for identical assertion.Line 83 checks for the same "Complete" text visibility without the
toPass()retry wrapper that was added on line 24. For consistency and to address flakiness uniformly across the test suite, this assertion should also use the retry pattern.Apply this diff:
- await expect(page.getByText('Complete')).toBeVisible(); + await expect(async () => await expect(page.getByText('Complete')).toBeVisible()).toPass();e2e/davinci-suites/src/phone-number-field.test.ts (1)
96-96: Inconsistent retry pattern for identical assertion.Line 96 checks for "Registration Complete" heading visibility without the
toPass()retry wrapper that was added on lines 49-52 for the same assertion. For consistency and to address flakiness uniformly, this assertion should also use the retry pattern.Apply this diff:
- await expect(page.getByRole('heading', { name: 'Registration Complete' })).toBeVisible(); + await expect( + async () => + await expect(page.getByRole('heading', { name: 'Registration Complete' })).toBeVisible(), + ).toPass();e2e/oidc-suites/src/logout.spec.ts (1)
82-85: Inconsistent navigation pattern in PingOne test.Lines 82-85 still use the old
Promise.allpattern for the same post-click navigation scenario that was fixed on lines 41-48 for the PingAM test. For consistency and to address flakiness uniformly, the PingOne test should also use the sequential promise pattern.Apply this diff:
await page.getByLabel('Username').fill(pingOneUsername); await page.getByRole('textbox', { name: 'Password' }).fill(pingOnePassword); - await Promise.all([ - page.waitForURL('http://localhost:8443/ping-one/**'), - page.getByRole('button', { name: 'Sign On' }).click(), - ]); + const promise = page.waitForURL('http://localhost:8443/ping-one/**'); + await page.getByRole('button', { name: 'Sign On' }).click(); + + /** + * This block is flakey, changing to this pattern + * https://playwright.dev/docs/network#network-events + **/ + await promise; expect(page.url()).toContain('code');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/davinci-suites/src/basic.test.ts(1 hunks)e2e/davinci-suites/src/phone-number-field.test.ts(2 hunks)e2e/oidc-suites/src/logout.spec.ts(1 hunks)
⏰ 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). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (1)
e2e/oidc-suites/src/logout.spec.ts (1)
41-48: Improved navigation synchronization pattern.The change from
Promise.allto sequential promise-based waiting is a better pattern for handling post-click navigation. This ensures the click is issued before awaiting the navigation, which is more reliable in flaky network conditions.
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
flakey-tests
branch
4 times, most recently
from
e1455c5 to
124346d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
flakey-tests
branch
from
124346d to
7eff7b1
Compare
@forgerock/davinci-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
|
Deployed 37586f4 to https://ForgeRock.github.io/ping-javascript-sdk/pr-450/37586f4b8000d5882e381db663a2faf79cc8a28b branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 82.0 KB (+0.0 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Fix 'hydration' in oidc app and flakey tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
e2e/oidc-app/src/ping-am/main.ts (1)
25-30: Clean up debug artifacts.The DOMContentLoaded pattern correctly defers initialization to avoid timing issues, but the code contains debug artifacts that should be removed.
Apply this diff to clean up the code:
document.addEventListener('DOMContentLoaded', async () => { - console.log('loaded outside'); - // (async () => { await oidcApp({ config, urlParams }); - // })(); });e2e/oidc-suites/src/logout.spec.ts (1)
82-85: Consider applying the same pattern to PingOne test.The PingOne logout test still uses
Promise.allfor navigation (lines 82-85), while the PingAM test now uses the separate promise pattern (lines 41-48) to avoid race conditions. For consistency and to prevent similar flakiness, consider updating this test to match the pattern used in the PingAM test.Apply this diff:
await page.getByLabel('Username').fill(pingOneUsername); await page.getByRole('textbox', { name: 'Password' }).fill(pingOnePassword); - await Promise.all([ - page.waitForURL('http://localhost:8443/ping-one/**'), - page.getByRole('button', { name: 'Sign On' }).click(), - ]); + const promise = page.waitForURL('http://localhost:8443/ping-one/**'); + await page.getByRole('button', { name: 'Sign On' }).click(); + await promise;e2e/oidc-suites/src/login.spec.ts (1)
20-20: Remove commented code.The commented out code (previous implementation using
navigate) should be removed now that the migration topage.goto()is complete. Leaving commented code reduces readability and can cause confusion.Apply this diff across the affected lines:
- const { /* navigate, */ clickButton } = asyncEvents(page); - // await page.goto('/ping-am/'); + const { clickButton } = asyncEvents(page);And similar cleanup for lines 55, 108, 151, and 164:
- // const { navigate } = asyncEvents(page);Also applies to: 38-38, 55-55, 72-72, 90-90, 108-108, 123-123, 151-151, 164-164
e2e/oidc-suites/src/token.spec.ts (2)
188-188: Remove commented code.The commented destructuring should be removed for consistency with the rest of the file.
Apply this diff:
- // const { clickButton } = asyncEvents(page);
18-104: Consider migrating PingAM tests for consistency.The PingAM token tests (lines 18-104) still use the
navigatehelper fromasyncEvents, while the PingOne tests have been migrated to usepage.goto()with explicit loading checks. For consistency across the test suite and to apply the same flakiness improvements, consider updating the PingAM tests to match the pattern used in PingOne tests.e2e/oidc-app/src/utils/oidc-app.ts (3)
21-21: Remove commented-out code.This commented line should be deleted rather than left in the codebase.
- // const appEl = document.getElementById('app');
61-63: Clean up commented-out code and consider debug logs.The commented
window.addEventListener('load')wrapper (lines 62-63, 169) should be removed. The debugconsole.logstatements (lines 61, 66, 99) may be useful for investigating the flaky tests, but consider removing them once the tests are stable.- console.log('oidc app called'); - // window.addEventListener('load', () => { - // console.log('loaded'); const myButton = document.getElementById('login-background'); if (myButton) { - console.log('button found'); myButton.addEventListener('click', async () => { // ... rest of handler }); } else { - console.log('not found'); } // ... other code - // });Also applies to: 169-169
20-25: Consider adding type annotation to the error parameter.For consistency and type safety, add a type annotation similar to the
displayTokenResponsefunction.-function displayError(error) { +function displayError( + error: GenericError | AuthorizationError | TokenExchangeErrorResponse, +) { // const appEl = document.getElementById('app'); const errorEl = document.createElement('div'); errorEl.innerHTML = `<p><strong>Error:</strong> <span class="error">${JSON.stringify(error, null, 2)}</span></p>`; document.body.appendChild(errorEl); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
e2e/davinci-suites/src/basic.test.ts(1 hunks)e2e/davinci-suites/src/phone-number-field.test.ts(6 hunks)e2e/oidc-app/src/index.ts(1 hunks)e2e/oidc-app/src/ping-am/index.html(1 hunks)e2e/oidc-app/src/ping-am/main.ts(1 hunks)e2e/oidc-app/src/ping-one/index.html(1 hunks)e2e/oidc-app/src/utils/oidc-app.ts(3 hunks)e2e/oidc-suites/eslint.config.mjs(2 hunks)e2e/oidc-suites/src/login.spec.ts(9 hunks)e2e/oidc-suites/src/logout.spec.ts(1 hunks)e2e/oidc-suites/src/token.spec.ts(4 hunks)e2e/oidc-suites/src/utils/async-events.ts(2 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- e2e/oidc-app/src/index.ts
- e2e/oidc-app/src/ping-one/index.html
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/davinci-suites/src/basic.test.ts
- e2e/davinci-suites/src/phone-number-field.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
e2e/oidc-app/src/ping-am/main.ts (1)
e2e/oidc-app/src/utils/oidc-app.ts (1)
oidcApp(48-175)
e2e/oidc-suites/src/login.spec.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (1)
asyncEvents(9-67)
e2e/oidc-app/src/utils/oidc-app.ts (1)
packages/sdk-types/src/lib/authorize.types.ts (1)
GetAuthorizationUrlOptions(19-36)
e2e/oidc-suites/src/token.spec.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (2)
asyncEvents(9-67)clickButton(11-18)
⏰ 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). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (11)
package.json (1)
94-94: LGTM! ESLint plugin update aligns with new configuration.The version bump from ^2.0.0 to ^2.2.2 is consistent with the new Playwright ESLint configuration added in
e2e/oidc-suites/eslint.config.mjs.e2e/oidc-suites/eslint.config.mjs (1)
2-2: LGTM! Playwright ESLint integration improves test quality.Adding the Playwright plugin with
flat/recommendedconfig will help catch Playwright-specific issues and enforce best practices in test files.Also applies to: 23-31
e2e/oidc-app/src/ping-am/index.html (1)
10-18: LGTM! Loading state prevents flashes of uninitialized content.The approach of hiding
#appby default and showing a loading indicator addresses the hydration timing issues mentioned in the PR. The navigation elements outside#appensure they remain visible during initialization.e2e/oidc-suites/src/logout.spec.ts (1)
41-48: Good fix for navigation race condition.The separate promise pattern correctly addresses the timing issue by ensuring the URL wait is registered before the navigation is triggered, as documented in the Playwright network events guide.
e2e/oidc-suites/src/utils/async-events.ts (2)
7-7: LGTM! TypeScript typing improves type safety.Adding explicit
Pagetyping enables better IDE support and catches type errors at compile time, which aligns with the new Playwright ESLint configuration.Also applies to: 9-9
79-80: LGTM! More robust assertions with built-in retries.Using
toHaveTextinstead of comparingtextContentwithtoBeis more idiomatic for Playwright and provides automatic retries, which helps address the flakiness issues mentioned in the PR.e2e/oidc-suites/src/login.spec.ts (1)
22-24: LGTM! Loading state checks improve test reliability.The pattern of using
page.goto()with explicit loading state checks (await expect(page.locator('#loading')).toBeHidden()) is a good approach to ensure the app is fully initialized before test interactions. This directly addresses the hydration issues mentioned in the PR.Also applies to: 39-41, 56-58, 73-75, 91-93, 109-111, 124-126, 152-154, 165-167
e2e/oidc-suites/src/token.spec.ts (2)
108-111: LGTM! Consistent migration to explicit navigation pattern.The PingOne tests now use direct
page.goto()with loading state checks, matching the pattern adopted inlogin.spec.ts. This improves test reliability by ensuring the app is fully initialized before interactions.Also applies to: 134-137, 162-165, 188-191
117-121: LGTM! Proper handling of navigation timing.The separate promise pattern (
const promise = page.waitForURL(...); await click(); await promise;) correctly addresses race conditions, consistent with the fix inlogout.spec.ts.Also applies to: 143-147, 171-175
e2e/oidc-app/src/utils/oidc-app.ts (2)
56-59: Good defensive UI handling for test stability.The conditional visibility toggle ensures the app is revealed only after successful OIDC client initialization, which directly addresses the hydration timing issues mentioned in the PR objectives.
64-100: Improved defensive button attachment handles hydration timing.The conditional element existence check before attaching the event listener is a good defensive pattern that prevents errors when the DOM is not fully hydrated. The refactored authorization flow with consolidated error and success handling is cleaner and more maintainable.
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
Classification: code_change
The failure occurs in the test "login and get tokens" at e2e/oidc-suites/src/token.spec.ts:126:54. The test expects to find an element with ID '#accessToken-1' after clicking the "Get Tokens" button, but the element is not found within the 5-second timeout.
Analysis of the PR changes:
The PR introduced significant changes to the OIDC app's loading behavior by wrapping the app initialization in a DOMContentLoaded event listener (e2e/oidc-app/src/ping-am/main.ts:138-143 and similar changes for ping-one). Additionally, the app content is now hidden by default with a loading indicator shown initially, only revealing the app once the OIDC client successfully initializes (e2e/oidc-app/src/utils/oidc-app.ts:183-186).
Root cause:
The test is failing because the "Get Tokens" button interaction is happening, but the subsequent token display elements (#accessToken-1) are not appearing. Looking at the changes in e2e/oidc-app/src/utils/oidc-app.ts, the login-background button event listener was moved inside a conditional check (lines 212-256) and now includes additional logging and DOM ready checks. However, other button event listeners (like the one for "Get Tokens") remain at the same scope level but may be affected by timing issues related to the new DOMContentLoaded wrapper.
The test at src/token.spec.ts:447-470 was updated to include await expect(page.locator('#loading')).toBeHidden() after navigation and after the Sign On click. However, this particular test expects tokens to be displayed after clicking "Get Tokens", and the token display logic may be experiencing timing issues due to the restructured initialization flow.
The failure is directly related to the intentional code changes made to improve loading synchronization. The test needs to be updated to account for the new loading behavior, potentially adding an additional wait or using the .toPass() pattern that was applied to other tests in this PR. However, the test file token.spec.ts was only partially updated - navigation was changed and loading waits were added, but the actual token retrieval assertion was not wrapped with a retry mechanism like other flaky operations in the PR.
The fix would involve applying the same .toPass() retry pattern to the token assertions to handle the asynchronous nature of the token display after the new loading architecture was implemented.
A code change would likely not resolve this issue, so no action was taken.
🎓 To learn more about Self Healing CI, please visit nx.dev
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
This PR doesn't sit well with me. Can we discuss this a bit before continuing with this?
| await navigate('/ping-am/'); | ||
| const { /* navigate, */ clickButton } = asyncEvents(page); | ||
| // await page.goto('/ping-am/'); | ||
| await page.goto('/ping-am/'); |
There was a problem hiding this comment.
Can you explain this change?
| await page.getByRole('button', { name: 'Sign On' }).click(); | ||
|
|
||
| await expect(page.getByText('Complete')).toBeVisible(); | ||
| await expect(async () => await expect(page.getByText('Complete')).toBeVisible()).toPass(); |
There was a problem hiding this comment.
What is the functional reason for this toPass?
JIRA Ticket
N/A
Description
There's a flakeyness issue i'm just trying to figure out how to solve.
toPasswill retry a playwright assertion until it passes. https://playwright.dev/docs/test-assertions#expecttopassI don't know the root cause of the flakeyness. basically seems like we are just dealing with poor hydration with our ui's which are obviously not fine tuned apps.
Summary by CodeRabbit