fix(tooling): remove wdio & upgrade meeting widget to playwright#628
fix(tooling): remove wdio & upgrade meeting widget to playwright#628Riteshfyi wants to merge 28 commits intowebex:meetings-upgradefrom
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52c36927e5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
packages/@webex/widgets/package.json
Outdated
| "release:dry-run": "semantic-release --dry-run", | ||
| "start": "npm run demo:serve", | ||
| "test:e2e": "npm run demo:build && wdio wdio.conf.js", | ||
| "test:e2e": "npm run demo:build && yarn playwright test --project='Meetings Setup' --project='Meetings Widget Test'", |
There was a problem hiding this comment.
Use the configured setup project name in test:e2e
test:e2e invokes Playwright with --project='Meetings Setup', but the setup project defined in playwright.config.ts is named OAuth: Get Meeting Access Token (line 76). Since Playwright resolves --project by exact project name, this command fails before tests run, which means the e2e_test_meetings workflow path (.github/workflows/pull-request.yml, Test E2E) cannot execute the migrated meetings suite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good Review Codex, it was a silly mistake.
| this.unmuteAudioBtn = this.controls.getByRole('button', {name: 'Unmute'}); | ||
| this.muteVideoBtn = this.controls.getByRole('button', {name: 'Stop video'}); | ||
| this.unmuteVideoBtn = this.controls.getByRole('button', {name: 'Start video'}); | ||
| this.joinMeetingBtn = this.controls.getByRole('button', {name: /^(Muted, video off|Unmuted, video on)$/}); |
There was a problem hiding this comment.
Match the join button’s actual accessible name
The new locator only matches a button named /^(Muted, video off|Unmuted, video on)$/, but the meeting widget code still identifies the pre-join control as button[aria-label="Join meeting"] (packages/@webex/widgets/src/widgets/WebexMeetings/WebexMeetings.jsx), and the prior E2E test also targeted “Join meeting”; in that pre-join state joinMeetingBtn.click() in both suites cannot resolve an element, so the tests fail before joining.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
After leaving & joining again, the name changes
Shreyas281299
left a comment
There was a problem hiding this comment.
Thanks for this upgrade. Changes look good. Lets just ensure that the checks are passing.
And what about the credentials?
|
Will Move these Changes to a feature branch, before we migrate the meeting widgets to latest version of our Webex |
packages/@webex/widgets/package.json
Outdated
| "release:dry-run": "semantic-release --dry-run", | ||
| "start": "npm run demo:serve", | ||
| "test:e2e": "npm run demo:build && wdio wdio.conf.js", | ||
| "test:e2e": "npm run demo:build && yarn playwright test --config=../../../playwright.config.ts --project='Meetings Widget Test'", |
There was a problem hiding this comment.
- Why are we using both npm and yarn ? Let's use a single package manager
- Do we have to provide the config path ? Is there a way we can use the default path and keep the command to minimum ?
- Can we also rename the folder from
widgetstomeetings-widgetsor justmeetings? NOT IN THE SCOPE OF THIS PR
There was a problem hiding this comment.
I have done the changes for point 1 & 2. will have to see if there are any consequence before we change the name.
| @@ -0,0 +1,31 @@ | |||
| import {Page, Locator, expect} from '@playwright/test'; | |||
|
|
|||
| const TEST_URL = 'https://localhost:9000/'; | |||
There was a problem hiding this comment.
- Should we move this to a constant or config file ?
- Will it work in the pipeline ? Maybe this value can come from env variable
There was a problem hiding this comment.
- Moved to constants
- Yeah, this will work. i think env will be an overkill here.
| import { test, expect } from '@playwright/test'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import { SamplesPage } from './pages/SamplesPage'; | ||
| import { MeetingWidgetPage } from './pages/MeetingWidgetPage'; | ||
| import dotenv from 'dotenv'; |
There was a problem hiding this comment.
nitpick: Let's re-arrange the imports. Please check other files for reference
| // Re-read .env fresh so we pick up the token written by Meetings Setup | ||
| const envPath = path.resolve(__dirname, '../../.env'); | ||
| const envConfig = dotenv.parse(fs.readFileSync(envPath)); | ||
| for (const key in envConfig) { | ||
| process.env[key] = envConfig[key]; | ||
| } |
There was a problem hiding this comment.
Why do we need to do this? We are calling the dotenv.config() at the top. Please verify if it's require
There was a problem hiding this comment.
Playwright test files were reading older environment variable values. For example, after OAuth, the tests were reading an empty string instead of the newly written access token. To address this issue, this change has been made.
There was a problem hiding this comment.
we have to update the process with the new variables, because the meetings-setup has updated the token in the .env, therefore we have to update the variables cached in the process.
I tried with upgrading the dot-env to version 18 & using override : true, it was working directly. but too much logs are being printed by dot-env, in the latest version. so decide to move with the less hectic solution
| test.describe('Meeting Widget', () => { | ||
| let samplesPage: SamplesPage; | ||
| let meetingPage: MeetingWidgetPage; | ||
| let accessToken: string; |
There was a problem hiding this comment.
Are we changing the value of accessToken anywhere? If not, this can be const and value can be assigned here itself from the env variable
There was a problem hiding this comment.
Updated, since access token & meeting destination are only being used once, they directly initialized as const in the test itself now.
playwright/meetings.setup.ts
Outdated
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
nitpick: remove unnecessary newlines.
Applicable everywhere in this file
playwright/meetings.setup.ts
Outdated
| const envPath = path.resolve(__dirname, '../.env'); | ||
| let envContent = ''; | ||
| if (fs.existsSync(envPath)) { | ||
| envContent = fs.readFileSync(envPath, 'utf8'); | ||
| } | ||
|
|
||
|
|
||
| const tokenPattern = /^PW_MEETING_ACCESS_TOKEN=.*$/m; | ||
| if (tokenPattern.test(envContent)) { | ||
| envContent = envContent.replace(tokenPattern, `PW_MEETING_ACCESS_TOKEN=${accessToken}`); | ||
| } else { | ||
| if (!envContent.endsWith('\n') && envContent.length > 0) envContent += '\n'; | ||
| envContent += `PW_MEETING_ACCESS_TOKEN=${accessToken}\n`; | ||
| } | ||
|
|
||
| fs.writeFileSync(envPath, envContent, 'utf8'); | ||
| console.log('Access token written to .env'); | ||
| }); |
There was a problem hiding this comment.
Why do we have to do all this if we are using dotenv package ?
There was a problem hiding this comment.
dotenv doesn't have the feature option to update env file, it is read only.
so to update or write the access token, we are handling it using this way.
| const e2eConfig = [ | ||
| `--disable-site-isolation-trials`, | ||
| `--disable-web-security`, | ||
| `--no-sandbox`, | ||
| `--disable-features=WebRtcHideLocalIpsWithMdns`, | ||
| `--allow-file-access-from-files`, | ||
| `--use-fake-ui-for-media-stream`, | ||
| `--use-fake-device-for-media-stream`, | ||
| `--use-file-for-fake-audio-capture=${dummyAudioPath}`, | ||
| `--disable-extensions`, | ||
| `--disable-plugins`, | ||
| `--window-size=1280,720`, | ||
| ]; |
There was a problem hiding this comment.
Let's add a comment at the end of each config for what they are being used
playwright.config.ts
Outdated
| webServer: [ | ||
| { | ||
| command: 'yarn workspace samples-cc-react-app serve', | ||
| url: 'http://localhost:3000', |
There was a problem hiding this comment.
This can come from constant or cofing file.
Applicable everywhere in this file
playwright.config.ts
Outdated
| }, | ||
| webServer: [ | ||
| { | ||
| command: 'yarn workspace samples-cc-react-app serve', |
There was a problem hiding this comment.
Let's ensure we are using either npm or yarn throughout the codebase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb168355e5
ℹ️ 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".
| this.unmuteAudioBtn = this.controls.getByRole('button', { name: 'Unmute' }); | ||
| this.muteVideoBtn = this.controls.getByRole('button', { name: 'Stop video' }); | ||
| this.unmuteVideoBtn = this.controls.getByRole('button', { name: 'Start video' }); | ||
| this.joinMeetingBtn = this.controls.getByRole('button', { name: /^(Muted, video off|Unmuted, video on)$/ }); |
There was a problem hiding this comment.
Include "Join meeting" in join button locator
The new join-button locator only accepts Muted, video off or Unmuted, video on, but the meeting widget still treats the pre-join control as button[aria-label="Join meeting"] (see WebexMeetings focus logic), so on the initial join flow joinMeetingBtn.click() can fail to resolve any element before the test even enters the meeting. This makes the migrated Playwright suite flaky/failing whenever the control still exposes the standard "Join meeting" accessible name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31ce16db2b
ℹ️ 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/widgets/package.json
Outdated
| "release:dry-run": "semantic-release --dry-run", | ||
| "start": "npm run demo:serve", | ||
| "test:e2e": "npm run demo:build && wdio wdio.conf.js", | ||
| "test:e2e": "npm run demo:build && cd ../../.. && yarn run test:e2e:meetings", |
There was a problem hiding this comment.
Install Playwright deps before running meetings E2E
This script now runs the meetings suite through Playwright, but the e2e_test_meetings workflow path (.github/workflows/pull-request.yml, step Test E2E at line 193) invokes it without the npx playwright install --with-deps step that the CC Playwright job uses at lines 220-221. On fresh GitHub runners (empty Playwright browser cache), the setup project will fail before tests because required browser binaries/system deps are missing, so this change breaks the meetings E2E pipeline unless that install step is added.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Noted, will raise separate PR for this.
|
NOTE : before merging this PR, we have to create a feature branch, there we will update the .yml & the webex resolution for testing the tests on CI before merging. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76ded35323
ℹ️ 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".
playwright.config.ts
Outdated
|
|
||
| const projects = testScope === 'cc' ? ccProjects | ||
| : testScope === 'meetings' ? meetingsProjects | ||
| : [...ccProjects, ...meetingsProjects]; |
There was a problem hiding this comment.
Scope default Playwright projects to avoid cross-suite runs
The fallback branch here runs ccProjects and meetingsProjects whenever TEST_SCOPE is unset, but the CC workflow still invokes unscoped yarn run test:e2e (.github/workflows/pull-request.yml, e2e_test_cc_widgets), so CC CI now executes meetings setup/specs too. That couples CC validation to meeting OAuth/token flows and duplicates coverage already provided by e2e_test_meetings, which can block CC PRs on unrelated meetings instability.
Useful? React with 👍 / 👎.
packages/@webex/widgets/package.json
Outdated
| "release:dry-run": "semantic-release --dry-run", | ||
| "start": "npm run demo:serve", | ||
| "test:e2e": "npm run demo:build && wdio wdio.conf.js", | ||
| "test:e2e": "npm run demo:build && cd ../../.. && yarn run test:e2e:meetings", |
There was a problem hiding this comment.
Build widget dist before running meetings E2E
This script now launches the meetings Playwright suite but only runs demo:build first, which produces the demo bundle rather than dist/webexWidgets.mjs; however the meetings web server (samples-meeting-app) imports @webex/widgets from the package entry in dist/. On a clean checkout (no prebuilt dist/), yarn workspace @webex/widgets run test:e2e fails during server startup before tests execute. The pre-step should build the package output (build:src) or otherwise guarantee dist/ exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dd1386250
ℹ️ 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".
playwright.config.ts
Outdated
| const projects = testScope === 'cc' ? ccProjects | ||
| : testScope === 'meetings' ? meetingsProjects | ||
| : [...ccProjects, ...meetingsProjects]; |
There was a problem hiding this comment.
Limit default project list to CC suites
The fallback branch now runs [..., ...meetingsProjects] whenever TEST_SCOPE is unset, so the existing CC workflow path (e2e_test_cc_widgets still calls yarn run test:e2e without TEST_SCOPE) will execute meetings tests too. This makes meetings failures block the CC job despite the separate meetings job being continue-on-error, and it duplicates E2E runtime on every labeled PR.
Useful? React with 👍 / 👎.
playwright.config.ts
Outdated
| webServer: [ | ||
| { | ||
| command: 'yarn workspace samples-cc-react-app serve', | ||
| url: 'http://localhost:3000', | ||
| reuseExistingServer: !process.env.CI, |
There was a problem hiding this comment.
Scope web servers to the selected test suite
webServer is now hard-coded to start both sample apps for every run, even when TEST_SCOPE targets only one suite. This couples CC-only and meetings-only runs to unrelated app startup health, so a failure in one sample app can abort the other suite before tests begin. Build the webServer list from TEST_SCOPE the same way projects is scoped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69ad9362bc
ℹ️ 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".
playwright.config.ts
Outdated
| const projects = testScope === 'cc' ? ccProjects | ||
| : testScope === 'meetings' ? meetingsProjects | ||
| : [...ccProjects, ...meetingsProjects]; |
There was a problem hiding this comment.
Restrict default project selection to avoid cross-suite CI runs
The new projects selector runs both CC and meetings suites whenever TEST_SCOPE is unset, and the e2e_test_cc_widgets workflow still invokes yarn run test:e2e without setting that variable. In practice this makes the CC job execute meetings OAuth/setup tests too, so unrelated meetings login/secrets instability can now fail the CC pipeline and the meetings suite gets run twice (also in e2e_test_meetings), which is a regression in CI isolation and reliability.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yeah. the workflow is updated in my PR. when this one is migrated it will be easy to just move the changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87de470403
ℹ️ 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 projects = testScope === 'cc' ? ccProjects | ||
| : testScope === 'meetings' ? meetingsProjects | ||
| : [...ccProjects, ...meetingsProjects]; |
There was a problem hiding this comment.
Avoid running both setup projects in the default test scope
When TEST_SCOPE is unset (the current yarn run test:e2e path used by the CC workflow), this ternary selects both CC and meetings projects, so Playwright can execute playwright/global.setup.ts and playwright/meetings.setup.ts in the same run; both setup files do read-modify-write updates to the same ../.env file (UpdateENVWithUserSets/OAuth token writes in global.setup.ts and token write in meetings.setup.ts), which creates a last-writer-wins race that can drop variables like PW_MEETING_ACCESS_TOKEN and make the suite flaky.
Useful? React with 👍 / 👎.
COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-772628
This pull request addresses
Removes Wdio E2E Tests & moves to playwright for meetings widget.
by making the following changes
converting previous tests to typescrip & updating them for palywright
VIDCAST
https://app.vidcast.io/share/efc89d9a-211e-4130-a602-e326ac9f3b43
Change Type
The following scenarios were tested
The GAI Coding Policy And Copyright Annotation Best Practices
Checklist before merging
Make sure to have followed the contributing guidelines before submitting.