test(calling): add Playwright E2E base setup and SDK initialization tests#4790
test(calling): add Playwright E2E base setup and SDK initialization tests#4790eigengravy wants to merge 11 commits intowebex:calling-sdk-e2e-testsfrom
Conversation
…ests Add Playwright infrastructure for Calling SDK E2E tests against the kitchen sink sample app, including OAuth global setup, shared utilities, and INIT-001 through INIT-003 test cases for SDK initialization.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 504ec1deae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53f6565ebc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…tory Move test files, utilities, constants, and global setup into playwright/tests/calling/ for better organization. Update OAuth flow to use widgets.webex.com instead of developer portal.
rarajes2
left a comment
rarajes2
left a comment
There was a problem hiding this comment.
As discussed the structure can be something like this
playwright
- calling
- tests
- utils
- foo
- Restructure playwright/tests/calling/ → playwright/calling/{tests,utils}
- Rename global.setup.ts → oauth.setup.ts
- Rename fullSetup → initAndRegister
- Extract ServiceIndicator type to constants
- Split callingUtils into setup.ts and registration.ts
- Add inline comments for Chrome launch flags in playwright.config.ts
akulakum
left a comment
akulakum
left a comment
There was a problem hiding this comment.
PR Review
Good foundation for E2E testing infrastructure. The separation of concerns (utils, constants, setup, registration) is clean, the ServiceIndicator type is a nice pattern, and the Chrome flags are well-documented. However, there are a couple of critical issues that need to be addressed before merge.
Summary
Critical (P1): 2 issues — token handoff across Playwright projects is broken on clean runs; --disable-web-security masks real CORS bugs.
High (P2): 6 issues — unpinned dependency, overly strict SKIP_AUTH guard, phantom Contact Center project, DevTools flag in CI config, no CI integration, caret range on Playwright dep.
Medium (P3): 5 issues — unnecessary dynamic import, duplicated logic in guest test, fragile .env manipulation, missing serial mode annotation, text-based selector fragility.
See inline comments for details and recommendations.
- Remove unused baseline-browser-mapping dependency - Gate --auto-open-devtools-for-tabs behind !process.env.CI - Comment out Contact Center project placeholder - Switch to static import in setup.ts - Remove duplicate auth status check in Guest Calling test - Run SDK init tests in parallel with separate credentials - Add id to Generate Guest Token button, update selector - Set retries to 3 and workers to 3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 758b45f8f0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- SDK_INIT_TIMEOUT: 60s → 65s (RETRY_TIMER_UPPER_LIMIT + 5s) - REGISTRATION_TIMEOUT: 40s → 35s (BASE_REG_RETRY_TIMER_VAL_IN_SEC + 5s) - OPERATION_TIMEOUT: 30s → 15s (SUPPLEMENTARY_SERVICES_TIMEOUT + 5s)
eigengravy
force-pushed
the
calling-sdk-e2e-tests-sdk-init
branch
from
b403066 to
c8d0594
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8d0594bf6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
packages/@webex/contact-center/src/services/task/TaskManager.ts
Outdated
Show resolved
Hide resolved
packages/@webex/contact-center/src/services/task/TaskManager.ts
Outdated
Show resolved
Hide resolved
eigengravy
force-pushed
the
calling-sdk-e2e-tests-sdk-init
branch
from
c8d0594 to
b403066
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b403066242
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ae9081768
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Kesari3008
left a comment
Kesari3008
left a comment
There was a problem hiding this comment.
- Why are we using feature branch to merge E2E tests changes, how does this affect any working code files if we merge it next branch ?
- If we are planning to submit this in separate branch then we should be making change in pul request yml to make the pipeline run?
- Also do we have plan to add playwright.yml to run it in the pipeline. Maybe we should add that in initial PRs itself where we are setting the playwright config up
| <input id="jwt-token-for-dest" name="jwtToken" placeholder="JWT token for destination" value="" type="text" style="margin: 0.5rem 0 0.5rem 0;"> | ||
| <input id="guest-name" name="guestName" placeholder="Guest name" value="" type="text" style="margin: 0.5rem 0 0.5rem 0;"> | ||
| <button type="button" onclick="generateGuestToken()">Generate Guest Token [Prod only]</button> | ||
| <button id="generate-guest-token" type="button" onclick="generateGuestToken()">Generate Guest Token [Prod only]</button> |
There was a problem hiding this comment.
Why is this change required ?
There was a problem hiding this comment.
The id="generate-guest-token" was added to the button in the sample app HTML so the Playwright test can target it with a stable #generate-guest-token CSS selector. Previously the test used a fragile text-based selector (button:has-text("Generate Guest Token")) which would break if the button label ever changed. This was flagged by Akula in the review as a nit — adding an id makes the selector resilient to label changes.
| "@babel/types": "^7.14.9", | ||
| "@commitlint/cli": "^17.1.2", | ||
| "@commitlint/config-conventional": "^17.1.0", | ||
| "@playwright/test": "^1.58.2", |
There was a problem hiding this comment.
Do we not need the command(script) to run these tests here ?
There was a problem hiding this comment.
Done — added test:e2e:calling script: "test:e2e:calling": "yarn playwright test --project='Calling SDK E2E'"
| /** | ||
| * Navigate, init SDK, verify, and optionally set service and register line. | ||
| */ | ||
| export const initAndRegister = async ( |
There was a problem hiding this comment.
Where are we using this and when we have individually testcases calling the below methods anyway then why we need this method
navigateToCallingApp(page) anyway then
if (options.service) {
await setServiceIndicator(page, options.service);
}
await initializeCallingSDK(page, accessToken);
await verifySDKInitialized(page);
if (options.registerLine) {
await registerLine(page);
await verifyLineRegistered(page);
There was a problem hiding this comment.
initAndRegister is used in the device registration tests (next PR) where each account's beforeAll needs to navigate → init → register. Avoids repeating the same 5-step sequence in every test group.
|
|
||
| // Media | ||
| GET_MEDIA_STREAMS_BTN: '#sd-get-media-streams', | ||
| LOCAL_VIDEO: '#local-video', |
There was a problem hiding this comment.
Why have we added constants for local video and remote video, we do not have video mode supported in WebRTC ?
There was a problem hiding this comment.
Done — removed LOCAL_VIDEO and REMOTE_VIDEO selectors since calling doesn't have video.
| // Timeouts — SDK timeout + 5s buffer for network/UI overhead | ||
| export const AWAIT_TIMEOUT = 10000; // General UI interactions | ||
| export const SDK_INIT_TIMEOUT = 65000; // RETRY_TIMER_UPPER_LIMIT (60s) + 5s | ||
| export const REGISTRATION_TIMEOUT = 35000; // BASE_REG_RETRY_TIMER_VAL_IN_SEC (30s) + 5s |
There was a problem hiding this comment.
How did we come up with this timeout value ?
There was a problem hiding this comment.
Each timeout is derived from the corresponding SDK constant + 5s buffer for network/UI overhead:
SDK_INIT_TIMEOUT(65s) =RETRY_TIMER_UPPER_LIMIT(60s) + 5sREGISTRATION_TIMEOUT(35s) =BASE_REG_RETRY_TIMER_VAL_IN_SEC(30s) + 5sOPERATION_TIMEOUT(15s) =SUPPLEMENTARY_SERVICES_TIMEOUT(10s) + 5s
| }); | ||
| await expect(page.locator(SELECTORS.REGISTER_BTN)).toBeEnabled({timeout: SDK_INIT_TIMEOUT}); | ||
|
|
||
| const hasCallingClient = await page.evaluate(() => !!(window as any).callingClient); |
There was a problem hiding this comment.
We should also consider checking the config that is passed during initialization and based on that we should be checking which clients are active. There can be cases where we have CallingClient, CallHistoryClient, Voicemail etc instantiated too. I know we are focusing on CallingCleint specific tests for now but initialization should cover all scenarios or if we do not want to do that, add a TODO here to add expect statements for other clients
There was a problem hiding this comment.
Added a TODO comment in verifySDKInitialized to check which clients are active based on the initialization config. Currently we only verify callingClient exists — the TODO notes that different configs can also instantiate CallHistoryClient, Voicemail, etc. and we should add expect statements for each.
| test.describe('SDK Initialization', () => { | ||
| test.describe.configure({mode: 'parallel'}); | ||
|
|
||
| test('Normal Calling - init with calling service indicator', async ({page}) => { |
There was a problem hiding this comment.
We should also have testcase where we are passing region and country during initialization. And verify mobius servers discovered based on that. HAve helper methods created to pass those values.
There was a problem hiding this comment.
Added setRegion and setCountry helpers in setup.ts, along with REGION_INPUT/COUNTRY_INPUT selectors and REGION/COUNTRY constants. The test case itself will be added in the device registration PR where we have the TestManager for proper account isolation across parallel tests.
There was a problem hiding this comment.
Also do we have options to test from different browser, we should be providing that
There was a problem hiding this comment.
Adding config for firefox, edge and safari and ability to set broswer an env variable. Should we run tests across multiple browsers in the test command?
|
|
||
| const getToken = (envVar: string): string => { | ||
| const token = process.env[envVar]; | ||
| if (!token) { |
There was a problem hiding this comment.
We are using 3 different tokens based on different scenarios, can that cause errors here? Maybe adding test.skip guards for optional users
There was a problem hiding this comment.
I am using the 3 available accounts to run the tests (that only require one account) in parallel sets. Here the 3 SDK Init tests run in parallel and in the following Device Registration tests, I have grouped them into sets with each account mapped to a set.
| import {Page, expect} from '@playwright/test'; | ||
| import {SELECTORS, AWAIT_TIMEOUT, REGISTRATION_TIMEOUT} from './constants'; | ||
|
|
||
| export const registerLine = async (page: Page): Promise<void> => { |
There was a problem hiding this comment.
This method and below method can be combined
There was a problem hiding this comment.
Keeping them separate — in the device registration tests (next PR), verifyLineRegistered is used standalone to assert auto-re-registration after network disconnect (REG-008) where the SDK re-registers automatically without us calling registerLine.
| * - calling.register() is called automatically | ||
| * - After register, registerElm is enabled and callingClient + line are set up | ||
| */ | ||
| export const initializeCallingSDK = async (page: Page, accessToken: string): Promise<void> => { |
There was a problem hiding this comment.
Same comment here, below and this can be combined and we should also be checking that after initialization init button and token textbox getting disabled
| test.describe.configure({mode: 'parallel'}); | ||
|
|
||
| test('Normal Calling - init with calling service indicator', async ({page}) => { | ||
| await navigateToCallingApp(page); |
There was a problem hiding this comment.
Consider looking into approach of creating Page Object Model and using the page class objects to do verification directly under tests
There was a problem hiding this comment.
We're using the same utility-function pattern as the widgets repo. I did a feasibility analysis on POM and it doesn't work well with our test patterns — we rely on context.route() for network interception, browserContext.newPage() for multi-page scenarios, and shared browser context across chained tests (e.g. failover where one test's backup state feeds into the next). These are context-centric patterns that don't map cleanly to page class objects. The current utility-function approach keeps things flexible for route mocking and shared context.
The idea was to first merge everything into a feature branch and then open a PR to next with the playwright test added to the pipeline once the CI testing accounts are setup as well. Currently I am testing with the Bifrost Testing Accounts. |
Address PR review feedback: add yarn test:e2e:calling script to package.json and extract CC service domain into a named constant.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdeed354fc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Add Firefox, Edge, and Safari browser configs via PW_BROWSER env var - Split constants into separate modules: selectors.ts, timeouts.ts, index.ts - Move OAuth constants (ENV_PATH, WIDGETS_URL) from oauth.setup.ts to constants - Remove unused LOCAL_VIDEO and REMOTE_VIDEO selectors
Add setRegion/setCountry helpers, REGION_INPUT/COUNTRY_INPUT selectors, and REGION/COUNTRY constants for discovery configuration in E2E tests.
4 participants
COMPLETES https://jira-eng-gpk2.cisco.com/jira/browse/CAI-7736
This pull request addresses
Adding Playwright E2E test infrastructure for the Calling SDK kitchen sink sample app, starting with SDK initialization test cases (INIT-001 through INIT-003).
by making the following changes
playwright.config.ts) with Chrome WebRTC flags, web server setup, and project configurationplaywright/global.setup.ts) that fetches personal access tokens from developer.webex.com with parallel token fetching for caller, callee, and transfer usersplaywright/Utils/callingUtils.ts) for navigation, SDK init, and service indicator/domain selection (with commented-out future utils for registration, calls, hold, etc.)playwright/constants.ts) for element selectors and timeoutsplaywright/tests/01-sdk-initialization.spec.ts)@playwright/testdevDependency and ESLint override for playwright filesChange Type
The following scenarios were tested
callingservice indicatorcontactcenterservice indicator and service domainguestcallingservice indicatorVidcast Link: https://app.vidcast.io/share/a69273ae-fa0d-4025-a67a-55bd06c16a23
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that