Migrate e2e test suite to Playwright TypeScript#237
Open
HusneShabbir wants to merge 2 commits into
Open
Conversation
Replace the Python pytest UI test stack with Playwright TypeScript tests and update CI scripts/docs so local and CI runs execute the same test workflow. Co-authored-by: Cursor <cursoragent@cursor.com>
HusneShabbir
requested review from
JslYoon,
gabemontero,
johnmcollier,
michael-valdron and
thepetk
as code owners
July 6, 2026 08:32
PR Summary by QodoMigrate E2E suite to Playwright TypeScript (Node) and update CI/scripts
AI Description
Diagram
High-Level Assessment
Files changed (38)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
4 rules 1. Unquoted PLAYWRIGHT_EXTRA_ARGS expansion
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| KEYCLOAK_CLIENT_SECRET="$KEYCLOAK_CLIENT_SECRET" \ | ||
| PLAYWRIGHT_HEADLESS="${PLAYWRIGHT_HEADLESS:-true}" \ | ||
| uv run pytest -v "${PYTEST_EXTRA_ARGS:-}" | ||
| npx playwright test ${PLAYWRIGHT_EXTRA_ARGS:-} |
There was a problem hiding this comment.
1. Unquoted playwright_extra_args expansion 📘 Rule violation ✧ Quality
In scripts/run-tests.sh, PLAYWRIGHT_EXTRA_ARGS is expanded unquoted when invoking `npx playwright test`, which will trigger ShellCheck SC2086 (word splitting/globbing) and violates the requirement that scripts in scripts/ be ShellCheck-clean.
Agent Prompt
## Issue description
`scripts/run-tests.sh` expands `PLAYWRIGHT_EXTRA_ARGS` unquoted (`${PLAYWRIGHT_EXTRA_ARGS:-}`), which will fail ShellCheck (SC2086) and violates the requirement that all `scripts/` shell scripts are ShellCheck-clean.
## Issue Context
The script intends to allow passing multiple Playwright CLI args via an env var. ShellCheck flags unquoted expansions unless a justified disable comment is provided or the code is refactored to safely pass arguments.
## Fix Focus Areas
- scripts/run-tests.sh[92-92]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+87
to
+88
| const fixturesDir = path.join(import.meta.dirname, "../fixtures/uploads"); | ||
| const e2eRoot = path.join(import.meta.dirname, ".."); |
There was a problem hiding this comment.
2. Nonportable import.meta.dirname 🐞 Bug ☼ Reliability
The Playwright suite builds fixture paths with import.meta.dirname, which can be undefined on part of the documented Node.js >=20 support range and will crash before any tests execute. This affects both Lightspeed specs and notebook helpers that resolve fixture files.
Agent Prompt
### Issue description
The Playwright TS code uses `import.meta.dirname` to compute fixture paths. This property is not reliably available across the documented Node >=20 range, so the test process can fail during module evaluation.
### Issue Context
The repo documents Node >=20 for local runs, but the spec/helper files assume `import.meta.dirname` exists.
### Fix Focus Areas
- Replace `import.meta.dirname` usages with a portable `__dirname` derived from `import.meta.url` (e.g., `fileURLToPath` + `path.dirname`).
- tests/specs/lightspeed.spec.ts[87-88]
- tests/support/notebook-constants.ts[7-31]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+7
to
+17
| function drawerListItems(page: Page): Locator { | ||
| return historyDrawer(page).locator("li"); | ||
| } | ||
|
|
||
| export function pinnedChatItems(page: Page): Locator { | ||
| return drawerListItems(page); | ||
| } | ||
|
|
||
| export function recentChatItems(page: Page): Locator { | ||
| return drawerListItems(page); | ||
| } |
There was a problem hiding this comment.
3. Pinned chat locator unscoped 🐞 Bug ≡ Correctness
pinnedChatItems() and recentChatItems() both return all <li> elements in the drawer, so verifyChatPinned() can pass even when a chat is only present in the recent section. The context-menu helpers also filter by text without narrowing to a single element, which can trigger Playwright strict-mode failures if the same chat appears in multiple sections.
Agent Prompt
### Issue description
Chat management helpers do not distinguish pinned vs recent chat lists, which can produce false-positive pin/unpin assertions. Additionally, context-menu open helpers may act on multiple matches, causing strict-mode failures.
### Issue Context
These helpers are used by `lightspeed.spec.ts` to validate pinning persistence and unpin behavior.
### Fix Focus Areas
- Implement `pinnedChatItems()` and `recentChatItems()` to scope to their respective sections/menus under the "Pinned chats" and "Chats" headings.
- In `openChatContextMenuByName` / `openPinnedChatContextMenuByName`, narrow to a single row (e.g., `.first()` or a more specific selector) before clicking Options.
- tests/support/chat-management.ts[3-17]
- tests/support/chat-management.ts[19-45]
- tests/support/chat-management.ts[104-108]
- tests/specs/lightspeed.spec.ts[411-434]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+76
to
+82
| log "Installing Node dependencies in $TESTS_DIR..." | ||
| cd "$TESTS_DIR" | ||
| if [ -f package-lock.json ]; then | ||
| npm ci | ||
| else | ||
| npm install | ||
| fi |
There was a problem hiding this comment.
4. Duplicate npm install in ci 🐞 Bug ➹ Performance
The PR-check workflow runs npm ci in tests/ and then calls make ci-tests, but scripts/run-tests.sh unconditionally runs npm ci again, duplicating dependency installation work. This increases CI runtime and adds an extra point of failure for every PR run.
Agent Prompt
### Issue description
CI installs test dependencies twice: once in the workflow and again inside `scripts/run-tests.sh`. This is redundant work and increases CI runtime/failure surface.
### Issue Context
`make ci-tests` executes `scripts/ci-run-tests.sh` which delegates to `scripts/run-tests.sh`.
### Fix Focus Areas
- Choose a single owner for dependency installation:
- either remove the workflow `npm ci` step and keep install in `run-tests.sh` (but ensure Playwright browser install still has dependencies available), or
- add a guard/flag in `run-tests.sh` to skip install when CI already ran it (e.g., `SKIP_NPM_INSTALL=1`).
- .github/workflows/ci-pr-check.yaml[72-88]
- Makefile[9-16]
- scripts/ci-run-tests.sh[1-7]
- scripts/run-tests.sh[76-82]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Cover single-server MCP toggle/edit/tool-call behavior and make notebook/lightspeed checks resilient to real cluster state differences.
| --- | ||
|
|
||
| You are a test runner for the AI Rolling Demo E2E test suite. Tests live in `tests/` and use Playwright + pytest (Python ≥3.11, managed with `uv`). | ||
| You are a test runner for the AI Rolling Demo E2E test suite. Tests live in `tests/` and use Playwright + TypeScript (Node.js ≥20). |
Contributor
There was a problem hiding this comment.
package.json specifies Node.js 24 types as a development dependency, shouldn't this mean we'll need Node 24 or at least that?
Suggested change
| You are a test runner for the AI Rolling Demo E2E test suite. Tests live in `tests/` and use Playwright + TypeScript (Node.js ≥20). | |
| You are a test runner for the AI Rolling Demo E2E test suite. Tests live in `tests/` and use Playwright + TypeScript (Node.js ≥24). |
| if [ ! -f "$VENV_ACTIVATE" ]; then | ||
| log_fail "Virtual environment not found at $TESTS_DIR/.venv. Run 'uv sync' inside tests/ first." | ||
| if ! command -v node >/dev/null 2>&1; then | ||
| log_fail "Node.js is not installed. Install Node.js 20+ and retry." |
Contributor
There was a problem hiding this comment.
Same as previous comment:
Suggested change
| log_fail "Node.js is not installed. Install Node.js 20+ and retry." | |
| log_fail "Node.js is not installed. Install Node.js 24+ and retry." |
| ## Testing | ||
|
|
||
| Tests are in `tests/` using Playwright + pytest (Python ≥3.11, managed with `uv`). | ||
| Tests are in `tests/` using Playwright + TypeScript (Node.js ≥20). |
Contributor
There was a problem hiding this comment.
Same as previous comment:
Suggested change
| Tests are in `tests/` using Playwright + TypeScript (Node.js ≥20). | |
| Tests are in `tests/` using Playwright + TypeScript (Node.js ≥24). |
maysunfaisal
reviewed
Jul 7, 2026
| --- | ||
|
|
||
| You are a test runner for the AI Rolling Demo E2E test suite. Tests live in `tests/` and use Playwright + pytest (Python ≥3.11, managed with `uv`). | ||
| You are a test runner for the AI Rolling Demo E2E test suite. Tests live in `tests/` and use Playwright + TypeScript (Node.js ≥20). |
Contributor
There was a problem hiding this comment.
Should we merge this to development first before the main branch? cc @michael-valdron
Contributor
There was a problem hiding this comment.
Ah good catch @maysunfaisal! Yes, we should re-target this PR to development branch instead of main.
fyi @HusneShabbir
maysunfaisal
reviewed
Jul 7, 2026
| await dropdown.click(); | ||
| await expect(page.locator("body")).toMatchAriaSnapshot(` | ||
| - menu: | ||
| - menuitem "gpt-4.1-mini" |
Contributor
There was a problem hiding this comment.
This will break whenever models are added or removed from the deployment. Consider asserting that at least one model menu item is visible rather than matching an exact snapshot, or document that this test must be updated when models change.
maysunfaisal
reviewed
Jul 7, 2026
Comment on lines
+27
to
+39
| test.beforeAll(async ({ browser }) => { | ||
| test.setTimeout(12 * 60 * 1000); | ||
|
|
||
| context = await browser.newContext({ | ||
| baseURL: process.env.RHDH_BASE_URL, | ||
| permissions: ["clipboard-read", "clipboard-write"], | ||
| ignoreHTTPSErrors: true, | ||
| locale: "en-US", | ||
| timezoneId: "UTC", | ||
| userAgent: | ||
| "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 " + | ||
| "(KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36", | ||
| }); |
Contributor
There was a problem hiding this comment.
copy-pasted identically across lightspeed.spec.ts, mcp.spec.ts, and notebook.spec.ts. Consider extracting into a shared fixture.
maysunfaisal
reviewed
Jul 7, 2026
| await uploadModal.clickAddFilesForStagedCount(1); | ||
| await expect(uploadModal.dialog()).toBeHidden(); | ||
| await notebooks.expectDocumentUploadCompletes(fileName); | ||
| await page.waitForTimeout(5000); |
Contributor
There was a problem hiding this comment.
Agentic Claude:
Consider replacing with a waitFor on the expected UI state to avoid flakiness
michael-valdron
left a comment
michael-valdron
left a comment
Contributor
There was a problem hiding this comment.
@HusneShabbir Failures are happening because this is coming from a forked repository, workflows seem to only run correctly if the PR branch is in this repository.
cc @thepetk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves:
https://redhat.atlassian.net/browse/RHIDP-15130