fix(tooling): add-playwright-tests#420
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes introduce Playwright-based end-to-end testing to the project. A new Playwright configuration file and test suite are added, along with related dependencies and scripts in 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Shreyas281299
force-pushed
the
playwright
branch
from
76954ef to
16d3bff
Compare
Shreyas281299
force-pushed
the
playwright
branch
2 times, most recently
from
93be8ae to
9610a5d
Compare
mkesavan13
left a comment
mkesavan13
left a comment
There was a problem hiding this comment.
Looks good mostly. Just a few things
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.github/workflows/pull-request.yml (3)
175-175: Consider reducing the timeout value.Isn't 60 minutes a lot to timeout? Consider keeping it to 15 or 20 minutes instead to detect and fail stuck tests more quickly.
208-211: Verify if ACCESS_TOKEN is still required.With OAuth setup, is this ACCESS_TOKEN still needed? Also, this approach requires setting up the access token secret in the parent repo and keeping it updated regularly.
216-223: Clarify where artifacts are uploaded.Where do these artifacts get uploaded? Consider adding a comment to document the destination and retention policy of these test artifacts.
🧹 Nitpick comments (5)
packages/contact-center/cc-components/src/components/UserState/user-state.tsx (2)
108-112: Refactor dynamicdata-testidon item container
Embedding dynamic classes insidedata-testidcan break selector stability. Consider using a staticdata-testid="item-container"and assert highlight state via itsclassName.- data-testid={`item-container ${shouldHighlight ? `selected ${getIconStyle(item).class}` : ''}`} + data-testid="item-container"
108-108: Use immutable identifiers for list items
data-testid={state-item-${item.name}}may include spaces or casing changes. Usingitem.idor a slugified version ensures consistent selectors.- data-testid={`state-item-${item.name}`} + data-testid={`state-item-${item.id}`}.github/workflows/pull-request.yml (1)
182-185: Update checkout action version.The version of
actions/checkout@v3is outdated. Consider updating to the latest version.- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.7)
182-182: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
playwright/login-user-state.spec.ts (2)
11-145: Consider refactoring repeated code patterns.There's significant code duplication across the test cases, particularly for:
- Checking and filling the ACCESS_TOKEN
- Handling login/logout button state verification
- Navigation to the application URL
Consider extracting these common patterns into helper functions or using test fixtures to improve maintainability.
12-12: Use baseURL from configuration instead of hardcoded URLs.You've defined
baseURL: 'http://localhost:3000'in the Playwright config, but you're using hardcoded URLs in the tests. Use relative paths with the configured baseURL:- await page.goto('http://localhost:3000/'); + await page.goto('/');Also applies to: 56-57, 105-105
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.github/workflows/pull-request.yml(2 hunks).gitignore(1 hunks)package.json(2 hunks)packages/contact-center/cc-components/src/components/StationLogin/station-login.tsx(5 hunks)packages/contact-center/cc-components/src/components/UserState/user-state.tsx(4 hunks)packages/contact-center/station-login/eslint.config.mjs(1 hunks)packages/contact-center/station-login/package.json(2 hunks)playwright.config.ts(1 hunks)playwright/login-user-state.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/contact-center/cc-components/src/components/StationLogin/station-login.tsx (1)
Learnt from: Kesari3008
PR: webex/widgets#239
File: packages/contact-center/station-login/tests/station-login/station-login.presentational.tsx:0-0
Timestamp: 2024-11-19T13:44:14.152Z
Learning: The file `packages/contact-center/station-login/tests/station-login/station-login.presentational.tsx` is going to change completely, so writing tests for these changes are being ignored.
🧬 Code Graph Analysis (1)
packages/contact-center/cc-components/src/components/UserState/user-state.tsx (1)
packages/contact-center/cc-components/src/utils/index.ts (1)
formatTime(4-17)
🪛 actionlint (1.7.7)
.github/workflows/pull-request.yml
182-182: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (18)
packages/contact-center/station-login/eslint.config.mjs (1)
10-12: ESLint config reformat — looks good
The multi-lineignoresblock improves readability without altering behavior..gitignore (1)
50-54: Ignore Playwright test artifacts
Adding**/test-results/and**/playwright-reportensures Playwright output isn’t committed. The existingtest_results/rule remains for any legacy artifacts with underscores.packages/contact-center/cc-components/src/components/UserState/user-state.tsx (4)
86-99: Add stable test IDs to container & select
The newdata-testidattributes on the root<div>and<SelectNext>enable reliable element targeting in Playwright.
117-119: Approve icon & text test IDs
Staticdata-testidon the<Icon>and<Text>elements provides clear hooks for verifying both visuals and labels.
131-132: Approve elapsed-time test ID
The<span>for elapsed times now has a stabledata-testid="elapsed-time", facilitating accurate timing assertions.
137-137: Approve arrow-icon test ID
Addingdata-testid="select-arrow-icon"on the dropdown indicator supports end-to-end assertions on dropdown state.package.json (1)
15-15: Introduce Playwright and e2e script
Adding@playwright/test@^1.51.1and atest:e2escript integrates E2E testing into local workflows.Also applies to: 50-50
packages/contact-center/station-login/package.json (1)
37-37: Add Node types & narrow Jest patterns
Including@types/nodeand refiningjest.testMatchtotests/station-login/*.ts(x)scopes unit tests appropriately.Also applies to: 69-70
packages/contact-center/cc-components/src/components/StationLogin/station-login.tsx (1)
128-128: Well-implemented test identifiers for Playwright E2E testing.You've added appropriate
data-testidattributes to key interactive elements in the StationLogin component. These identifiers are well-named and align with the selectors used in the Playwright tests, enabling robust UI testing.Also applies to: 169-169, 175-180, 212-212, 226-226, 245-245, 249-249
.github/workflows/pull-request.yml (2)
144-144: Good implementation of conditional E2E test execution.Adding a condition to run E2E tests only when the PR has the
run_e2elabel is a good practice. This prevents unnecessary test runs and saves CI resources.
213-215: Good implementation of the Playwright test execution.The step to run Playwright tests is correctly set up using the new
test:e2escript defined in package.json.playwright.config.ts (3)
1-21: Well-configured Playwright setup with web server integration.The configuration properly loads environment variables and sets up a local development server for testing. The reuse of existing servers in local development is a good optimization.
22-37: Good testing configuration for stability.Running tests non-parallel with a single worker is appropriate for ensuring stability during initial implementation of Playwright tests. The trace retention policy is well configured to capture failures without excessive storage use.
39-57: Appropriate browser selection strategy.Starting with just Chromium and commenting out additional browsers until stability is achieved is a sensible approach. The dependencies array ensures browsers will run in sequence when enabled.
playwright/login-user-state.spec.ts (4)
3-10: Good test setup with environment validation.The
beforeAllhook correctly validates that the required ACCESS_TOKEN environment variable is present before running tests.
11-48: Well-implemented extension login test flow.The test thoroughly verifies the login flow with the Extension option, including state changes and logout functionality.
50-98: Comprehensive multi-login test across browser tabs.This test effectively validates that login state synchronizes across multiple tabs, which is a critical feature for multi-device usage scenarios.
100-145: Thorough relogin test after page refresh.The test correctly verifies that login state and preferences persist after a page refresh, which is important for maintaining user session integrity.
| @@ -0,0 +1,35 @@ | |||
| import {test as setup} from '@playwright/test'; | |||
There was a problem hiding this comment.
This is global.setup.ts, we are running this through the playwright.config(see project in the config), this will run before running any playwright test, ensuring that before running any test we have an access_token,
We would need to add 2 secrets in our actions
PLAYWRIGHT_USERNAME
PLAYWRIGHT_PASSWORD
We can add more username pass and more oauth process like this to store more access_tokens
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| setup('OAuth', async ({browser}) => { |
There was a problem hiding this comment.
Should we write a cleanup for this?
| <Option key={1} value="token">Access Token</Option> | ||
| <Option key={2} value="oauth">Login with Webex</Option> | ||
| <Option data-testid='samples:login_option_token' key={1} value="token">Access Token</Option> | ||
| <Option data-testid='samples:login_option_oauth' key={2} value="oauth">Login with Webex</Option> |
There was a problem hiding this comment.
Should we really complicate this value by having a ":"? Can we just do "_" or "-" every where?
# [1.28.0-ccwidgets.61](v1.28.0-ccwidgets.60...v1.28.0-ccwidgets.61) (2025-05-26) ### Bug Fixes * **tooling:** add-playwright-tests ([#420](#420)) ([fa52889](fa52889))
|
🎉 This PR is included in version 1.28.0-ccwidgets.61 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
5 participants
COMPLETES # https://jira-eng-sjc12.cisco.com/jira/browse/CAI-6397
This pull request addresses
by making the following changes
yarn run test:e2eto start the playwright tests in widgetsrun_e2elabel to run the e2e tests for a PRThe following scenarios were tested
Failure: https://github.com/Shreyas281299/widgets/actions/runs/15131422767
Passing: https://github.com/Shreyas281299/widgets/actions/runs/15132156111
The GAI Coding Policy And Copyright Annotation Best Practices
Checklist before merging