test(calling): added playwright tests for call settings#5012
test(calling): added playwright tests for call settings#5012vivekv1504 wants to merge 5 commits into
Conversation
![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: 1d43739d40
ℹ️ 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".
| // Single-user Call Settings tests | ||
| { | ||
| name: 'SET_CALL_SETTINGS - PROD', | ||
| dependencies: ['OAuth - PROD'], |
There was a problem hiding this comment.
SET_CALL_SETTINGS uses USER_1, USER_2, and USER_3, but this project only depends on OAuth, so Playwright can run it in parallel with the registration projects and SET_CALL_TRANSFER_CONSULT, which use the same accounts. test-data.ts documents that the same Webex account must never be active in two sessions simultaneously and that dependencies are how shared accounts are ordered; with this dependency, these new tests can deregister or mutate settings while the other projects are running, making the E2E suite flaky.
Useful? React with 👍 / 👎.
| // Restore | ||
| if (originalChecked) { | ||
| await setCallForwardAlways(page, true, cfDestination); |
There was a problem hiding this comment.
When CF Always was already enabled before the test, the cleanup re-enables it with cfDestination (USER_2) instead of the destination that was loaded from the account. On real PROD/INT test accounts this leaves the user's call-forwarding target permanently changed after the test; capture the original destination/ring values before overwriting them and restore those exact values.
Useful? React with 👍 / 👎.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8cf4e3bb1
ℹ️ 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".
| testSuite: 'set-call-settings.spec.ts', | ||
| // Single-user Contacts supplementary service E2E tests |
There was a problem hiding this comment.
SET_CALL_SETTINGS never closes before the SET_CONTACTS key starts, so USER_SETS is syntactically malformed and importing packages/calling/playwright.config.ts fails before Playwright can discover or run any suite. Add the missing }, after this entry so the existing SET_CONTACTS project remains a sibling instead of being parsed inside it.
Useful? React with 👍 / 👎.
| // USER_4's number is a valid system number with no active registration here, | ||
| // so calls forwarded to it go to voicemail rather than looping back. | ||
| cfDestination = getPhoneNumber('USER_4'); |
There was a problem hiding this comment.
In PROD runs, this suite forwards live calls to USER_4, but playwright.config.ts lets SET_CALL_SETTINGS - PROD and SET_CALL - PROD both depend only on OAuth while SET_CALL actively registers USER_4/USER_5. When those projects overlap, the forwarded calls can ring USER_4's active SET_CALL session instead of going to voicemail, breaking the assumption in the comment and making both suites flaky; either declare/serialize this USER_4 usage or use a truly unused destination.
Useful? React with 👍 / 👎.
| await loadSettings(calleePage); | ||
| await setCallForwardAlways(calleePage, false); | ||
| await setCallForwardBusy(calleePage, false); | ||
| await setCallForwardNoAnswer(calleePage, false); | ||
| await setCallForwardNotReachable(calleePage, false); | ||
| await ensureDndState(calleePage, 'DND Disabled'); | ||
| await setVoicemailSendAllCalls(calleePage, false); | ||
| await setVoicemailSendBusyCalls(calleePage, false); | ||
| await setVoicemailSendUnansweredCalls(calleePage, false); |
There was a problem hiding this comment.
When these live-call tests start on an account that already has DND, call forwarding, or voicemail forwarding enabled, the setup unconditionally disables every setting and the later cleanup/finally blocks leave them disabled rather than restoring the original values. Because these run against shared PROD/INT test accounts, a passing or failed run can permanently change the account baseline and affect later suites; snapshot the loaded settings before this reset and restore them in afterAll.
Useful? React with 👍 / 👎.
eigengravy
left a comment
eigengravy
left a comment
There was a problem hiding this comment.
E2E test review — there's a parse error that blocks the entire suite, an account isolation violation, and several test logic gaps. Details inline.
| SET_CALL_SETTINGS: { | ||
| name: 'SET_CALL_SETTINGS', | ||
| accounts: ['USER_3', 'USER_2', 'USER_1'], | ||
| testSuite: 'set-call-settings.spec.ts', |
There was a problem hiding this comment.
CRITICAL: Missing closing }, — this file won't parse
The SET_CALL_SETTINGS object is missing its closing brace+comma. Playwright test discovery will fail immediately with a syntax error.
SET_CALL_SETTINGS: {
name: 'SET_CALL_SETTINGS',
accounts: ['USER_3', 'USER_2', 'USER_1'],
testSuite: 'set-call-settings.spec.ts',
+ },
// Single-user Contacts supplementary service E2E tests
SET_CONTACTS: {There was a problem hiding this comment.
Done chnages applied ,rebase the branch and appiled the changes
| use: browserOptions[PW_BROWSER], | ||
| }, | ||
| { | ||
| name: 'SET_CALL_SETTINGS - INT', |
There was a problem hiding this comment.
Account isolation violation: SET_CALL_SETTINGS shares USER_1/2/3 with registration tests
SET_CALL_SETTINGS uses accounts USER_1, USER_2, and USER_3 but only depends on OAuth - PROD. Since SET_REGISTRATION_1/2/3 also depend only on OAuth and use these same accounts, Playwright can schedule them concurrently — violating the constraint at the top of test-data.ts:
the same Webex account must NEVER be active in two browser sessions simultaneously
This needs dependencies: ['SET_REGISTRATION_1 - PROD', 'SET_REGISTRATION_2 - PROD', 'SET_REGISTRATION_3 - PROD'] at minimum, or should wait until after SET_CALL (which already serializes through the registration projects).
There was a problem hiding this comment.
added dependencies on SET_REGISTRATION_1/2/3 and SET_CALL_TRANSFER_CONSULT for both PROD and INT so SET_CALL_SETTINGS won’t run concurrently with other suites using USER_1/2/3.
| // Allow the CF setting to propagate to the server before placing the call. | ||
| await calleePage.waitForTimeout(5000); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Test logic gap: no caller-side verification that the call was actually placed
CS-CALL-101 (and CS-CALL-102 below) only asserts the callee's answer button stays disabled. But if makeCall() silently fails (network error, invalid number, SDK exception), no call is created and the callee never rings — the test still passes.
You need a caller-side assertion to confirm the outbound call was actually established. For example, wait for the caller's call-state indicator to show an active/ringing call before starting the observation window on the callee side.
There was a problem hiding this comment.
Added waitForCallerOutboundCall() after makeCall() in CS-CALL-101 and CS-CALL-102. It waits for the caller’s SDK to report an active outbound call via getActiveCalls() before asserting the callee doesn’t ring, so the test fails if the dial never happened.
|
|
||
| // Reload and verify the setting was persisted on the server. | ||
| await loadSettings(calleePage); | ||
| await expect(calleePage.locator(CALLING_SELECTORS.CF_BUSY_CB)).toBeChecked({ |
There was a problem hiding this comment.
Incomplete test: CS-CALL-103 never actually tests CF Busy forwarding
The test name says "CF When Busy" but it only verifies:
- The setting persists after reload
- The first call rings normally
It never:
- Answers the first call (to make the callee busy)
- Places a second call from
secondCallerPage(USER_1) - Verifies the second call gets forwarded instead of ringing the callee
As written, this test passes even if CF Busy forwarding is completely broken. The title should either be renamed to reflect what it actually tests ("CF Busy setting persists and doesn't affect non-busy calls"), or the test should be completed to exercise the actual busy-forward path.
There was a problem hiding this comment.
added waitForCallerOutboundCall() on CS-CALL-101/102/104/105/106; implemented CS-CALL-103 (answer first call, second call while busy, call-waiting-aware); hardened CS-CALL-104/106 with setting reload/verify. Full SET_CALL_SETTINGS - PROD suite passes locally.
|
|
||
| // Enable the checkbox so the email input is accessible. | ||
| if (!originalChecked) { | ||
| await page.locator(CALLING_SELECTORS.VM_NOTIF_EMAIL_CB).check({timeout: AWAIT_TIMEOUT}); |
There was a problem hiding this comment.
Incomplete restoration: if checkbox was originally ON, the test email is left in the account
The restoration block only unchecks if the checkbox was originally off:
if (!originalChecked) {
await page.locator(...).uncheck(...);
}
await saveVoicemailSettings(page);If the checkbox was already enabled, the test overwrites the real email with autotest-notification@example.com and never restores it. The account is left with a test email address as the configured notification destination.
Fix: capture originalEmail before the test, and always restore both the checkbox state and the email value:
const originalEmail = await page.locator(CALLING_SELECTORS.VM_NOTIF_EMAIL_ID).inputValue();
// ... test ...
// Restore:
if (originalChecked) {
await page.locator(CALLING_SELECTORS.VM_NOTIF_EMAIL_ID).fill(originalEmail || '');
} else {
await page.locator(CALLING_SELECTORS.VM_NOTIF_EMAIL_CB).uncheck(...);
}Same issue exists in CS-VM-015 below.
There was a problem hiding this comment.
restoration now captures originalEmail via inputValue() before the test overwrites it. If the checkbox was originally on, we restore the prior email and save; if it was off, we uncheck and save. Applied the same fix to CS-VM-015.
vivekv1504
force-pushed
the
callingSDK-callsettings
branch
from
f8cf4e3 to
e68b9a7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e68b9a7cc4
ℹ️ 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".
| const testEmail = 'autotest-notification@example.com'; | ||
| await page.locator(CALLING_SELECTORS.VM_NOTIF_EMAIL_ID).fill(testEmail, {force: true}); | ||
| await saveVoicemailSettings(page); |
There was a problem hiding this comment.
When the account already has email notifications enabled, this overwrites the saved destination with autotest-notification@example.com and the cleanup only preserves the checkbox state, so shared PROD/INT test accounts are left with a different voicemail notification address after the suite. Snapshot the original email value before filling the test address and restore it when originalChecked is true; the email-copy test below has the same issue.
Useful? React with 👍 / 👎.
| const targetRings = '4'; | ||
| await page.locator(CALLING_SELECTORS.VM_UNANSWERED_RINGS).fill(targetRings, {force: true}); | ||
| await saveVoicemailSettings(page); |
There was a problem hiding this comment.
When sendUnansweredCalls is already enabled on the shared account, this changes numberOfRings to 4 and never restores the previous value because the cleanup only unchecks the box when it was originally off. A passing run can therefore permanently alter the voicemail timeout for later suites or manual use; capture the original ring count and write it back when the setting was originally enabled.
Useful? React with 👍 / 👎.
Shreyas281299
left a comment
Shreyas281299
left a comment
There was a problem hiding this comment.
Submitting accidental empty pending review.
| // 3-user transfer tests — waits for 2-user (shared USER_4+USER_5) | ||
| // { | ||
| // name: 'SET_3USER - PROD', | ||
| // dependencies: ['SET_2USER - PROD'], | ||
| // testDir: './playwright/suites', | ||
| // testMatch: USER_SETS.SET_3USER.testSuite, | ||
| // use: browserOptions[PW_BROWSER], | ||
| // }, | ||
| // { | ||
| // name: 'SET_3USER - INT', | ||
| // dependencies: ['SET_2USER - INT'], | ||
| // testDir: './playwright/suites', | ||
| // testMatch: USER_SETS.SET_3USER.testSuite, | ||
| // use: {...browserOptions[PW_BROWSER], testEnv: 'int'} as any, | ||
| // }, |
There was a problem hiding this comment.
Please remove these commented lines
There was a problem hiding this comment.
Done! Removed commented lines
Shreyas281299
left a comment
Shreyas281299
left a comment
There was a problem hiding this comment.
Posting inline findings from /tmp/pr5012-review.md.
| }, | ||
| // Single-user Call Settings tests | ||
| { | ||
| name: 'SET_CALL_SETTINGS - PROD', |
There was a problem hiding this comment.
SET_CALL_SETTINGS currently reuses USER_1/USER_2/USER_3, which are also used by SET_REGISTRATION_1/2/3. Since these projects only depend on OAuth and run in parallel, this violates the single-active-session rule in test-data and creates account-collision flakiness. Could we assign a dedicated user set to SET_CALL_SETTINGS (or add explicit project dependencies) so those sessions never overlap?
There was a problem hiding this comment.
SET_CALL_SETTINGS currently shares USER_1/2/3 with SET_REGISTRATION_* and SET_CALL_TRANSFER_CONSULT while only depending on OAuth, which breaks the single-active-session rule in test-data.ts. I’ll fix this in a follow-up by [adding project dependencies | assigning a dedicated user set] so those sessions never overlap.
| // Allow the setting to propagate before placing the call. | ||
| await calleePage.waitForTimeout(5000); | ||
|
|
||
| try { |
There was a problem hiding this comment.
cfDestination = getPhoneNumber('USER_4') can collide with SET_CALL - PROD, where USER_4 is actively registered in parallel. That means forwarded calls from CS-CALL-101 may ring in the SET_CALL suite and perturb both projects. Can we route CF tests to a destination number not owned by any concurrently registered test account?
| const dest = page.locator(CALLING_SELECTORS.CF_ALWAYS_DEST); | ||
| const isChecked = await cb.isChecked(); | ||
|
|
||
| if (enable && !isChecked) { |
There was a problem hiding this comment.
In setCallForwardAlways (and similarly at the Busy/NoAnswer/NotReachable helpers), when enable === true and the toggle is already checked, we skip filling destination and still save. That can silently persist a stale number from previous state. Consider adding an enable && isChecked && destination branch to always refresh the destination before saveCfSettings.
There was a problem hiding this comment.
Fixed — added enable && isChecked && destination handling to all CF helpers so the destination is always refreshed before save when enabling with an explicit number.
| test('CS-VM-012: Voicemail — MWI notification badge toggle saves and reloads correctly', async () => { | ||
| await loadSettings(page); | ||
| const original = await page.locator(CALLING_SELECTORS.VM_MWI_CB).isChecked(); | ||
|
|
There was a problem hiding this comment.
page.unroute('**/features/doNotDisturb*', routeHandler) is currently outside a finally block. If an assertion throws before the end, the route stub can leak and affect retries/subsequent tests in the same page context. Could we wrap the route setup in try/finally and always unroute in finally?
There was a problem hiding this comment.
Wrapped the DND route stub in try/finally and pass the same routeHandler to unroute so the stub is always removed, even when assertions fail.
|
|
||
| if (original) { | ||
| await expect(page.locator(CALLING_SELECTORS.VM_UNANSWERED_CB)).not.toBeChecked({ | ||
| timeout: OPERATION_TIMEOUT, |
There was a problem hiding this comment.
expect(cfaDataElem).not.toBeEmpty() is too weak here because both success and error paths populate the element (callSetting or error), so this can pass on regressions. Can we assert a semantic value instead (e.g., expected destination substring or parsed JSON fields) to prove the call-forward update actually succeeded?
There was a problem hiding this comment.
Thanks — agreed that not.toBeEmpty() was too weak since both success (callSetting) and error paths populate #callforwardalways-data.
Updated CS-CFA-007 to assert on the network response instead: we wait for a successful GET callForwarding, then verify the response body includes callForwarding.always with a boolean enabled field. This ensures the lookup used valid API data rather than only checking that the element had any text (which could be an error string).
| // 4. Enter password and click Sign In | ||
| await page.getByPlaceholder('Password').fill(password, {timeout: 15000}); | ||
| await page | ||
| .locator('input#IDToken2[name="IDToken2"][type="password"]') |
There was a problem hiding this comment.
Switching to a strict input#IDToken2[name="IDToken2"][type="password"] selector improves specificity but removes the previous placeholder-based fallback. Since IdP attribute churn is common, can we keep this selector as primary and fall back to getByPlaceholder('Password') if it doesn't resolve?
There was a problem hiding this comment.
restored the fallback. Password entry now uses input#IDToken2[name="IDToken2"][type="password"] as the primary locator and falls back to getByPlaceholder('Password') via .or() if the strict selector doesn’t resolve. This keeps specificity when the IdP markup is stable while still tolerating attribute churn.
| CF_NO_ANSWER_DEST: '#notAnsweredDest', | ||
| CF_NOT_REACHABLE_CB: '#notReachableCb', | ||
| CF_NOT_REACHABLE_DEST: '#notReachableDest', | ||
| CF_DIRECTORY_NUMBER: '#directoryNumber', |
There was a problem hiding this comment.
div#incomingsection #answer relies on the container tag name to disambiguate duplicate incomingsection IDs. If markup changes (e.g., fieldset -> div), this may become ambiguous and fail in strict mode. Would it be safer to fix duplicate IDs in sample HTML and keep selectors semantically unique?
There was a problem hiding this comment.
Addressed by removing duplicate IDs from the sample HTML and standardizing on INCOMING_ANSWER_BTN: '#incomingsection #answer'. No tag-based disambiguation needed anymore.
| await setCallForwardNoAnswer(calleePage, true, cfDestination); | ||
| // Allow the setting to propagate before placing the call. | ||
| await calleePage.waitForTimeout(5000); | ||
|
|
There was a problem hiding this comment.
secondCallerPage appears to be fully initialized but unused in callSettingsCallTests, while still adding setup cost and an extra registered account session. Is this context still required? If not, removing it may both reduce runtime and eliminate one source of account collision.
| use: {...browserOptions[PW_BROWSER], testEnv: 'int'} as any, | ||
| }, | ||
|
|
||
| // 3-user transfer tests — waits for 2-user (shared USER_4+USER_5) |
There was a problem hiding this comment.
Noting the commented SET_3USER block references identifiers/projects that don't exist (USER_SETS.SET_3USER, SET_2USER - PROD/INT). Could we either remove this stale block or update it to valid identifiers to avoid confusion for future maintainers?
There was a problem hiding this comment.
Already removed the stale commented SET_3USER / SET_2USER block in the latest push — it was leftover from an older naming scheme. The live config now only uses SET_CALL and SET_CALL_TRANSFER_CONSULT
| }); | ||
| }; | ||
|
|
||
| export const answerCall = async (page: Page): Promise<void> => { |
There was a problem hiding this comment.
waitForIncomingCall and answerCall now rely on different incoming-answer selectors (INCOMING_CALL_ANSWER_BTN vs INCOMING_ANSWER_BTN). That split can cause partial breakages if one locator changes independently. Can we standardize both flows on a single selector constant/invariant?
There was a problem hiding this comment.
Standardized on a single INCOMING_ANSWER_BTN constant. Both waitForIncomingCall and answerCall use it, and call-settings.ts was updated. Removed the duplicate incoming-call markup from the sample HTML so the selector is #incomingsection #answer without tag-based disambiguation.
… and improved email handling
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3e809af98
ℹ️ 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".
| // Wait for both forms to be populated from the server before returning. | ||
| await Promise.all([cfResponsePromise, vmResponsePromise]); |
There was a problem hiding this comment.
CS-CALL-103 reads CALL_WAITING_BTN.innerText() immediately after loadSettings(), but this helper only waits for call-forwarding/voicemail responses and DND text before returning. If the call-waiting GET is slower than the DND request, the button can still say Fetching Call Waiting Status, causing an account with call waiting enabled to be treated as disabled and sending the test down the wrong branch/flaking; include the call-waiting response or final button text in the load barrier.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f5ffbb22e
ℹ️ 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".
| .poll( | ||
| async () => calleePage.locator(CALLING_SELECTORS.INCOMING_ANSWER_BTN).isDisabled(), | ||
| {timeout: NO_INCOMING_CALL_TIMEOUT, intervals: [1000]} | ||
| ) | ||
| .toBe(true); |
There was a problem hiding this comment.
When call waiting is disabled, this expect.poll(...).toBe(true) succeeds on the first poll because the answer button starts disabled, so the test stops immediately instead of verifying that the second incoming call never appears during NO_INCOMING_CALL_TIMEOUT. If CF Busy regresses and the callee rings a few seconds later, this branch still passes; wait the full observation window (as the other no-ring tests do) before asserting the button remains disabled.
Useful? React with 👍 / 👎.
3 participants
COMPLETES #< https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7878 >
This pull request addresses
The Calling SDK CallSettingsClient APIs (Do Not Disturb, Call Forwarding, Voicemail, and Call Waiting) did not have end-to-end Playwright coverage against the calling sample app. Without automated tests, regressions in settings fetch/save, UI wiring, or live call behavior (e.g. DND blocking incoming calls, call forwarding routing) could go unnoticed.
This PR adds a dedicated SET_CALL_SETTINGS Playwright project and a full E2E suite that exercises these flows through the sample app UI and validates real call behavior with multiple registered users.
by making the following changes
Playwright test infrastructure
SET_CALL_SETTINGSuser set intest-data.tswith three accounts:USER_3— settings owner / calleeUSER_2— primary callerUSER_1— second caller (for multi-user scenarios)SET_CALL_SETTINGS - PRODandSET_CALL_SETTINGS - INTprojects inplaywright.config.ts(with OAuth dependencies).playwright/suites/set-call-settings.spec.ts.Reusable test utilities (
utils/call-settings.ts)loadSettings()— clicks “Get Settings” and waits for bothcallForwardingandvoicemailGET responses plus DND UI settle (avoids saving CF/VM before server state is loaded).clickDnd(),ensureDndState()with PUT wait +expect.poll()to handle eventual consistency between write and read.setCallForwardAlways/Busy/NoAnswer/NotReachable()andsaveCfSettings()with PUT response waits.setVoicemailSendAllCalls/BusyCalls/UnansweredCalls()andsaveVoicemailSettings().Selectors (
constants/selectors.ts)#callForwardFormand#voicemailFormto avoid duplicate ID collisions in the sample app.UI tests (
callSettingsTests) — single user, serialEach live-call test resets settings in
beforeAll/finallyblocks and usesINCOMING_CALL_ANSWER_BTNto assert whetherline:incoming_callfired.Reliability fixes
Sample app
docs/samples/calling/index.html: corrected voicemail “When not answered” rings input ID (vmNotAnsweredRings).Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.