feat(cc-widgets): outdial widget e2e tests#640
feat(cc-widgets): outdial widget e2e tests#640eigengravy wants to merge 7 commits intowebex:nextfrom
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
e694b97 to
6f9a30b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f9a30ba55
ℹ️ 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".
6f9a30b to
5b6eddd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b6edddcdf
ℹ️ 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".
5b6eddd to
d2f5f95
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2f5f9592d
ℹ️ 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: 43f98cde15
ℹ️ 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: 217573a34e
ℹ️ 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: 33bf3bad7c
ℹ️ 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".
| import createOutdialCallTests from '../tests/outdial-call-test.spec'; | ||
|
|
||
| test.describe('Dial Number Task Control Tests', createDialNumberTaskControlTests); | ||
| test.describe('Outdial Call Tests', createOutdialCallTests); |
There was a problem hiding this comment.
Split outdial tests from dial-number suite state
Adding createOutdialCallTests to SET_6 makes these tests run after createDialNumberTaskControlTests, but that test factory never performs a full testManager.cleanup() teardown, so the first agent session remains active in Desktop mode before outdial setup starts. In a full --project=SET_6 run, setupForOutdialDesktop then reuses the same ${projectName}_AGENT1_ACCESS_TOKEN for a second station login while the prior session is still alive, which can block the new login and fail the outdial suite before test steps begin.
Useful? React with 👍 / 👎.
| test.beforeAll(async ({browser}, testInfo) => { | ||
| const projectName = testInfo.project.name; | ||
| testManager = new TestManager(projectName); | ||
| await testManager.setupForOutdialDesktop(browser); |
There was a problem hiding this comment.
🔴 High Severity - Test crashes before skip check
The test.skip on line 22 checks for PW_DIAL_NUMBER, but setupForOutdialDesktop in this beforeAll runs before the skip check executes. If DIAL_NUMBER_LOGIN_ACCESS_TOKEN is missing, setupOutdialCustomer() will fail in beforeAll, causing the suite to crash instead of skipping gracefully.
Impact: Suite crashes with unhelpful errors instead of skipping when env vars are missing.
Suggestion: Move the env-var guard into beforeAll itself:
test.beforeAll(async ({browser}, testInfo) => {
if (\!process.env.PW_DIAL_NUMBER || \!process.env.DIAL_NUMBER_LOGIN_ACCESS_TOKEN) {
test.skip(true, 'Required env vars not set');
return;
}
const projectName = testInfo.project.name;
testManager = new TestManager(projectName);
await testManager.setupForOutdialDesktop(browser);
});There was a problem hiding this comment.
Moved test.skip to the describe level so it runs before beforeAll. This skips the entire block (including setup) when env vars are missing.
| await acceptCustomerCall(testManager.callerPage); | ||
| await waitForState(testManager.agent1Page, USER_STATES.ENGAGED); | ||
| await verifyCurrentState(testManager.agent1Page, USER_STATES.ENGAGED); | ||
| await testManager.agent1Page.waitForTimeout(3000); |
There was a problem hiding this comment.
🟡 Medium Severity - Hard-coded timeout (flaky test anti-pattern)
This waitForTimeout(3000) is an arbitrary delay that can cause flaky tests. If the system is slow, 3 seconds might not be enough; if it's fast, you're wasting time waiting unnecessarily.
Suggestion: Replace with an explicit wait on UI state:
await expect(endCallButton).toBeEnabled({timeout: 5000});
// or wait for a specific element/state that indicates readinessThere was a problem hiding this comment.
Removed the waitForTimeout(3000). The test now proceeds directly after verifying the ENGAGED state. This is consistent with how other test files handle it — they rely on waitForState for the state transition and then act immediately.
| await waitForState(testManager.agent1Page, USER_STATES.ENGAGED); | ||
| await verifyCurrentState(testManager.agent1Page, USER_STATES.ENGAGED); | ||
| await testManager.agent1Page.waitForTimeout(3000); | ||
| await testManager.agent1Page.getByTestId('call-control:end-call').first().click({timeout: 5000}); |
There was a problem hiding this comment.
🟡 Medium Severity - Inconsistent call termination pattern
This test uses an inline locator + click to end the call, while the Extension test (line 64) uses the endCallTask() utility. This inconsistency makes maintenance harder and could mask mode-specific bugs.
Suggestion: Use the endCallTask utility for consistency, or create a dedicated endOutdialCall utility in outdialUtils.ts that works for both Desktop and Extension modes.
There was a problem hiding this comment.
Switched to using endCallTask for the extension page
| await verifyCurrentState(testManager.agent1Page, USER_STATES.ENGAGED); | ||
| await testManager.agent1Page.waitForTimeout(3000); | ||
| await testManager.agent1Page.getByTestId('call-control:end-call').first().click({timeout: 5000}); | ||
| await testManager.agent1Page.waitForTimeout(2000); |
There was a problem hiding this comment.
🟡 Medium Severity - Hard-coded timeout (flaky test anti-pattern)
Another waitForTimeout(2000) before wrapup. The Extension test uses waitForWrapupAfterCallEnd utility instead, which is more deterministic.
Suggestion: Use the same pattern as the Extension test:
await waitForWrapupAfterCallEnd(testManager.agent1Page);There was a problem hiding this comment.
Replaced with waitForWrapupAfterCallEnd which uses expect.poll() with exponential backoff — same pattern as the Extension test.
|
|
||
| test.beforeAll(async ({browser}, testInfo) => { | ||
| const projectName = testInfo.project.name; | ||
| testManager = new TestManager(projectName); |
There was a problem hiding this comment.
🔴 High Severity - Test crashes before skip check
Same issue as Desktop mode (line 17) - setupForOutdialExtension in beforeAll runs before the test.skip check on line 53. If DIAL_NUMBER_LOGIN_ACCESS_TOKEN is missing, the suite will crash.
Suggestion: Apply the same fix as suggested for line 17.
There was a problem hiding this comment.
Same fix as Desktop — test.skip moved to describe level.
| await waitForState(testManager.agent1Page, USER_STATES.ENGAGED); | ||
| await verifyCurrentState(testManager.agent1Page, USER_STATES.ENGAGED); | ||
| await testManager.agent1Page.waitForTimeout(3000); | ||
| await endCallTask(testManager.agent1ExtensionPage); |
There was a problem hiding this comment.
🟡 Medium Severity - Hard-coded timeout (flaky test anti-pattern)
Same issue as Desktop mode line 29 - waitForTimeout(3000) is an arbitrary delay that can cause flaky tests.
Suggestion: Replace with an explicit wait on UI state before ending the call.
There was a problem hiding this comment.
Removed the waitForTimeout(3000), same as the Desktop fix
playwright/test-manager.ts
Outdated
| } | ||
|
|
||
| private async setupOutdialCustomer(browser: Browser): Promise<void> { | ||
| const customerToken = process.env.DIAL_NUMBER_LOGIN_ACCESS_TOKEN ?? ''; |
There was a problem hiding this comment.
🔴 High Severity - Empty token not validated
This line uses process.env.DIAL_NUMBER_LOGIN_ACCESS_TOKEN ?? '' which defaults to an empty string if the env var is undefined. When loginExtension is called with an empty string, it will fail with an unhelpful error message.
Impact: Confusing test failures when env var is not set.
Suggestion: Add validation and throw a clear error:
const customerToken = process.env.DIAL_NUMBER_LOGIN_ACCESS_TOKEN;
if (!customerToken) {
throw new Error('DIAL_NUMBER_LOGIN_ACCESS_TOKEN is required for outdial tests');
}There was a problem hiding this comment.
The describe-level test.skip now checks for DIAL_NUMBER_LOGIN_ACCESS_TOKEN before the suite runs, so setupOutdialCustomer is never called with a missing token. Leaving the ?? '' to stay consistent with the existing pattern in test-manager.ts (e.g. PW_SANDBOX_PASSWORD uses the same default).
Switched to using
const envTokens = this.getEnvTokens();
const customerToken = envTokens.dialNumberLoginAccessToken;
if (!customerToken) {
throw new Error('Environment variable DIAL_NUMBER_LOGIN_ACCESS_TOKEN is missing or empty');| }); | ||
| } | ||
|
|
||
| async setupForOutdialDesktop(browser: Browser): Promise<void> { |
There was a problem hiding this comment.
🟡 Medium Severity - No cleanup on setup failure
These methods call this.setup() followed by this.setupOutdialCustomer(). If setupOutdialCustomer fails, the agent setup from this.setup() is already done but won't be cleaned up since the test will fail in beforeAll.
Impact: Resource leaks and potential conflicts in subsequent test runs.
Suggestion: Consider wrapping in try/catch with cleanup, or document that cleanup is the caller's responsibility on setup failure.
There was a problem hiding this comment.
No other test in the codebase uses try/catch in beforeAll — the convention is to let failures propagate to the test runner. Added the if (testManager) guard in afterAll to match the pattern used across all other test files (14 instances).
| export async function acceptCustomerCall(customerPage: Page): Promise<void> { | ||
| await customerPage.bringToFront(); | ||
| await expect(customerPage.locator('#answer').first()).toBeEnabled({timeout: ACCEPT_TASK_TIMEOUT}); | ||
| await customerPage.waitForTimeout(UI_SETTLE_TIMEOUT); |
There was a problem hiding this comment.
🟡 Medium Severity - Hard-coded wait in utility function
This waitForTimeout(UI_SETTLE_TIMEOUT) is a hard-coded delay that could be flaky. If this is for UI animation/settling, consider using a more deterministic condition.
Suggestion: Replace with waitForLoadState or wait for a specific UI condition:
await customerPage.waitForLoadState('networkidle');
// or
await expect(someElement).toBeStable();There was a problem hiding this comment.
UI_SETTLE_TIMEOUT is consistent with codebase convention — waitForTimeout is used throughout the test suite for UI settling between actions.
| import createOutdialCallTests from '../tests/outdial-call-test.spec'; | ||
|
|
||
| test.describe('Dial Number Task Control Tests', createDialNumberTaskControlTests); | ||
| test.describe('Outdial Call Tests', createOutdialCallTests); |
There was a problem hiding this comment.
🟡 Medium Severity - Test isolation concern
The outdial tests now share SET_6 with dial-number task control tests. If the dial-number tests don't fully clean up their agent sessions in afterAll, the subsequent outdial setup may conflict with stale session state.
Impact: Potential test conflicts and failures.
Suggestion: Verify that dial-number-task-control-test.spec.ts has proper afterAll cleanup, or consider running outdial tests in a separate suite/set for better isolation.
There was a problem hiding this comment.
Not seeing issues with shared SET_6 currently. Both suites have afterAll cleanup that handles session teardown.
Shreyas281299
left a comment
There was a problem hiding this comment.
Review Summary: Found 2 High severity and 7 Medium severity issues. High: env-var validation problems causing unhelpful test failures. Medium: multiple hard-coded timeouts (flaky test anti-patterns), inconsistent test patterns, and test isolation concerns. Please address the High severity issues and consider fixing the Medium severity timeouts to improve test reliability.
COMPLETES CAI-7682
This pull request addresses
Adding end-to-end Playwright tests for the outdial call workflow across two agent login modes (Desktop and Extension).
by making the following changes
The following scenarios were tested
The GAI Coding Policy And Copyright Annotation Best Practices