feat(cc-widgets): Add multi-party conference E2E suites for SET_7 and SET_8#624
feat(cc-widgets): Add multi-party conference E2E suites for SET_7 and SET_8#624rsarika wants to merge 15 commits intowebex:nextfrom
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
…nto multi-party-conference-e2e-tests # Conflicts: # playwright/ai-docs/AGENTS.md
…nto multi-party-conference-e2e-tests
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
akulakum
left a comment
There was a problem hiding this comment.
PR Review: feat(cc-widgets): Add multi-party conference E2E suites for SET_7 and SET_8
Summary
Good test coverage for multi-party conference scenarios. The PR adds comprehensive E2E tests for MPC, transfer, and switch flows with proper retry patterns and cleanup handling.
Critical Issues (Must Fix)
-
Missing
global.setup.tsupdates -setupForConferenceDesktopexpectsAGENT3_ACCESS_TOKENandAGENT4_ACCESS_TOKEN, butglobal.setup.tsonly handles AGENT1/AGENT2 OAuth flows. SET_7/SET_8 tests will fail at runtime. -
Missing
PAGE_TYPESupdates -constants.tsneedsAGENT3andAGENT4page types for consistency with TestManager changes.
Medium Issues
-
skipOAuthshould be environment-driven - Currently hardcoded; should useprocess.env.PW_SKIP_OAUTH -
Excessive
waitForTimeoutusage - ~15+ occurrences of hardcoded waits; prefer explicit condition waits for reliability -
Helper functions should be extracted -
setAgentState,ensureAgentsIdle,submitWrapupIfVisible,clickMergeWhenReadycould be reusable utilities inUtils/conferenceUtils.ts
Minor Issues
-
Factory pattern deviation - New parameterized factory differs from existing convention (functional but inconsistent)
-
5-minute test timeout - Consider per-test timeouts instead of blanket 300s
-
Missing env var validation in
getAgentNames()
Documentation Issues
-
Set-to-suite mapping outdated -
playwright/ai-docs/AGENTS.mdandai-docs/templates/playwright/00-master.mddon't include SET_7/SET_8 mappings -
Missing
setupForConferenceDesktopin TestManager methods list in docs
Positive Observations
✅ Good retry patterns in createInboundAndAccept and consultAgentWithRetry
✅ Proper error handling with .catch(() => {})
✅ Parallel context creation for performance
✅ Explicit merge-button readiness validation (addresses flakiness mentioned in PR description)
✅ Clear test naming convention (CTS-MPC-01, CTS-TC-01, etc.)
✅ Well-structured Playwright template hierarchy in ai-docs/templates/playwright/
| dotenv.config({path: path.resolve(__dirname, '.env')}); | ||
|
|
||
| const dummyAudioPath = path.resolve(__dirname, './playwright/wav/dummyAudio.wav'); | ||
| const skipOAuth = false; // Set to true to skip OAuth tests and dependencies, useful for local development without OAuth setup |
There was a problem hiding this comment.
Suggestion: Make skipOAuth environment-driven instead of hardcoded.
const skipOAuth = process.env.PW_SKIP_OAUTH === 'true';This allows toggling without code changes during local development.
| ENTRY_POINT: env.PW_ENTRY_POINT6, | ||
| TEST_SUITE: 'dial-number-tests.spec.ts', | ||
| }, | ||
| SET_7: { |
There was a problem hiding this comment.
🔴 Critical: global.setup.ts only handles AGENT1 and AGENT2 OAuth token acquisition. SET_7 and SET_8 define AGENT3 and AGENT4, but there's no corresponding update to global.setup.ts to acquire their tokens.
Tests will fail at runtime with:
Missing required conference access tokens: agent3AccessToken, agent4AccessToken
Please update global.setup.ts to iterate over all agents defined in each USER_SET (not just AGENT1/AGENT2).
| public agent2Page: Page; | ||
| public agent2Context: BrowserContext; | ||
|
|
||
| // Agent 3 main widget page (Conference suites) |
There was a problem hiding this comment.
🔴 Critical: PAGE_TYPES in constants.ts is not updated to include AGENT3 and AGENT4. This breaks consistency with the existing page type system.
Please add to constants.ts:
export const PAGE_TYPES = {
AGENT1: 'agent1',
AGENT2: 'agent2',
AGENT3: 'agent3', // Add
AGENT4: 'agent4', // Add
CALLER: 'caller',
// ... rest
};| this.consoleMessages = []; | ||
| this.setupPageConsoleLogging(this.agent1Page, true); | ||
| this.setupPageConsoleLogging(this.agent2Page, true); | ||
| this.setupPageConsoleLogging(this.agent3Page, true); |
There was a problem hiding this comment.
Clarification needed: The caller page uses agent4AccessToken. The inherited comment on line 73 says "Agent 2 for making calls" which is now misleading for conference flows. Consider updating the comment or documenting why agent4's token is used for the caller in conference scenarios.
|
|
||
| export default function createConferenceTransferSwitchTests(group: ConferenceSuiteGroup = 'all') { | ||
| let testManager: TestManager; | ||
| test.describe.configure({timeout: 300000}); |
There was a problem hiding this comment.
Suggestion: 5-minute timeout is quite long. Consider using per-test timeouts for specific slow tests rather than a blanket timeout for the entire suite.
// Instead of suite-wide:
test.describe.configure({timeout: 300000});
// Consider per-test for slow tests:
test('CTS-MPC-04 should support 4-party conference', async () => {
test.setTimeout(180000); // 3 min for this specific test
// ...
});| const runSwitch = group === 'all' || group === 'switch' || group === 'transfer-switch'; | ||
|
|
||
| const getAgentNames = () => ({ | ||
| agent2Name: process.env[`${testManager.projectName}_AGENT2_NAME`]!, |
There was a problem hiding this comment.
Issue: Non-null assertions (!) are used without validation. If env vars are missing, tests fail with unclear errors.
Suggestion: Add validation:
const getAgentNames = () => {
const agent2Name = process.env[`${testManager.projectName}_AGENT2_NAME`];
const agent3Name = process.env[`${testManager.projectName}_AGENT3_NAME`];
if (!agent2Name || !agent3Name) {
throw new Error(`Missing agent name env vars for ${testManager.projectName}`);
}
return { agent2Name, agent3Name };
};| await acceptIncomingTask(targetPage, TASK_TYPES.CALL, ACCEPT_TASK_TIMEOUT); | ||
| await waitForState(targetPage, USER_STATES.ENGAGED); | ||
| await verifyCurrentState(targetPage, USER_STATES.ENGAGED); | ||
| return; |
There was a problem hiding this comment.
Anti-pattern: Hardcoded waitForTimeout calls cause flaky tests. Found ~15+ occurrences in this file.
Suggestion: Replace with explicit condition waits:
// Instead of:
await testManager.callerPage.waitForTimeout(2000);
// Use:
await expect(testManager.callerPage.getByTestId('expected-element')).toBeVisible();
// or
await waitForState(page, expectedState);This applies to multiple locations in this file (lines 70, 77, 170, etc.).
|
|
||
| export type ConferenceSuiteGroup = 'all' | 'mpc' | 'transfer-switch' | 'mpc-transfer' | 'switch'; | ||
|
|
||
| async function setAgentState(page: Page, state: string): Promise<void> { |
There was a problem hiding this comment.
Suggestion: These helper functions (setAgentState, ensureAgentsIdle, submitWrapupIfVisible, clickMergeWhenReady, etc.) are generic and reusable.
Consider extracting them to Utils/conferenceUtils.ts for consistency with existing utility patterns and reusability in future conference tests.
// playwright/Utils/conferenceUtils.ts
export async function setAgentState(page: Page, state: string): Promise<void> { ... }
export async function ensureAgentsIdle(pages: Page[]): Promise<void> { ... }
export async function submitWrapupIfVisible(page: Page): Promise<void> { ... }
export async function clickMergeWhenReady(ownerPage: Page): Promise<void> { ... }| } finally { | ||
| await cleanupAllAgents(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Minor: The afterAll explicitly calls stationLogout for all agents, then calls testManager.cleanup() which also attempts logout. This causes redundant operations.
Suggestion: Either rely on testManager.cleanup() alone:
test.afterAll(async () => {
await cleanupAllAgents();
await testManager.cleanup(); // This already handles logout
});| import {test} from '@playwright/test'; | ||
| import createConferenceTransferSwitchTests from '../tests/conference-transfer-switch-test.spec'; | ||
|
|
||
| test.describe('Conference MPC Tests', () => createConferenceTransferSwitchTests('mpc')); |
There was a problem hiding this comment.
Note: This parameterized factory pattern differs from existing convention where factories are parameterless:
// Existing pattern:
test.describe('Advanced Task Controls Tests', createAdvancedTaskControlsTests);
// This PR:
test.describe('Conference MPC Tests', () => createConferenceTransferSwitchTests('mpc'));This is functional but inconsistent with existing patterns. Consider either:
- Splitting into separate factory files to match existing patterns, OR
- Documenting this as an intentional evolution for parameterized suites
…rty-conference-e2e-tests
COMPLETES #https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7322
This pull request addresses
Missing and uneven multi-party conference E2E coverage for the conference matrix scenarios, where tests were not logically split for parallel execution by set, and conference merge actions could intermittently fail due to missing button readiness checks.
by making the following changes
playwright/tests/conference-transfer-switch-test.spec.ts.SET_7: MPC scenarios (CTS-MPC-01..16)SET_8: Transfer + Switch scenarios (CTS-TC-01..05,CTS-SW-01..05)playwright/suites/following existing structure:conference-mpc-transfer-tests.spec.tsconference-switch-tests.spec.tsplaywright/test-data.tsforSET_7andSET_8.playwright/test-manager.tsfor 4-agent conference flows.skip/fixmeper agreed scope (TC14,TC20,TC17-19).The following scenarios were tested
yarn test:e2e --project=SET_7yarn test:e2e --project=SET_8yarn test:e2e --project=SET_7 --project=SET_8Result: