Skip to content

refactor(lightspeed): use lightspeed-playwright-e2e for shared e2e code#2833

Closed
HusneShabbir wants to merge 1 commit intoredhat-developer:mainfrom
HusneShabbir:refactor/lightspeed-e2e-package
Closed

refactor(lightspeed): use lightspeed-playwright-e2e for shared e2e code#2833
HusneShabbir wants to merge 1 commit intoredhat-developer:mainfrom
HusneShabbir:refactor/lightspeed-e2e-package

Conversation

@HusneShabbir
Copy link
Copy Markdown
Contributor

Move specs to import helpers, mocks, and fixtures from the published Playwright package. Keep plugin-sourced translations in e2e-tests via getTranslations. Remove duplicated local e2e utilities and upload fixtures now supplied by the package dependency.

Move specs to import helpers, mocks, and fixtures from the published
Playwright package. Keep plugin-sourced translations in e2e-tests via
getTranslations. Remove duplicated local e2e utilities and upload
fixtures now supplied by the package dependency.

Made-with: Cursor
@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge Bot commented Apr 20, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. CWD-based fixture path 🐞 Bug ☼ Reliability
Description
lightspeedE2eUploadFixturePath() hard-codes a process.cwd()/node_modules/.../dist/... layout to
locate upload fixtures, so the “Multiple file upload” test will fail with file-not-found when
Playwright is run from a different working directory or when node_modules is not rooted at
process.cwd() (common in monorepos/hoisting). This contradicts the Playwright config intent to use
absolute paths that work from any cwd.
Code

workspaces/lightspeed/e2e-tests/lightspeed.ui.test.ts[R50-61]

+function lightspeedE2eUploadFixturePath(locale: string, n: 1 | 2): string {
+  return path.join(
+    process.cwd(),
+    'node_modules',
+    '@red-hat-developer-hub',
+    'lightspeed-playwright-e2e',
+    'dist',
+    'fixtures',
+    'uploads',
+    `${locale}.upload${n}.json`,
+  );
+}
Relevance

⭐⭐⭐ High

Playwright config explicitly anchors paths with __dirname for cwd-independence;
process.cwd()+node_modules layout is brittle.

PR-#2626
PR-#2809

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test constructs fixture paths using process.cwd() and a hard-coded internal directory
structure under node_modules/.../dist/fixtures/uploads, then passes those paths into
uploadFiles(). Separately, the Playwright config explicitly calls out using absolute paths “to
work from any cwd” and uses __dirname to anchor paths, showing the repo expects cwd-independence;
this new helper is not cwd-independent. The workspace package.json also indicates this code lives
under a larger monorepo directory (repository.directory) and runs installs from a parent folder
(postinstall), reinforcing that process.cwd() is not a reliable anchor for where node_modules
(and thus fixtures) live.

e2e-tests/lightspeed.ui.test.ts[50-60]
e2e-tests/lightspeed.ui.test.ts[234-237]
playwright.config.ts[24-41]
package.json[36-48]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`lightspeedE2eUploadFixturePath()` is anchored to `process.cwd()` and assumes a specific on-disk `node_modules/@red-hat-developer-hub/lightspeed-playwright-e2e/dist/...` layout. This makes the e2e test brittle and can produce file-not-found errors when Playwright is invoked from a different cwd or when the package is installed/hoisted differently.

### Issue Context
The Playwright config already anchors paths using `__dirname` explicitly “to work from any cwd”. Fixture resolution should follow the same principle.

### Fix Focus Areas
- e2e-tests/lightspeed.ui.test.ts[50-61]
- e2e-tests/lightspeed.ui.test.ts[234-237]

### Suggested fix
Replace the `process.cwd() + node_modules + ...` join with Node resolution:
- Use `path.dirname(require.resolve('@red-hat-developer-hub/lightspeed-playwright-e2e/package.json'))` to get the installed package root, then `path.join(pkgRoot, 'dist', 'fixtures', 'uploads', ...)`.
- Alternatively, resolve the fixture file directly with `require.resolve(...)` (if the fixture files are shipped) and use that resolved absolute path.

This keeps fixture lookup robust regardless of cwd and monorepo node_modules layout.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@sonarqubecloud
Copy link
Copy Markdown

@rhdh-qodo-merge
Copy link
Copy Markdown

Review Summary by Qodo

Refactor e2e tests to use lightspeed-playwright-e2e package

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Migrate e2e tests to use shared lightspeed-playwright-e2e package
• Remove duplicated local e2e utilities, fixtures, and page objects
• Keep plugin-sourced translations via getTranslations function
• Update test imports to use published package exports
• Remove local upload fixture files (now in package)
Diagram
flowchart LR
  A["Local e2e code<br/>fixtures, pages, utils"] -->|"Migrate to"| B["lightspeed-playwright-e2e<br/>published package"]
  C["Plugin translations<br/>getTranslations"] -->|"Keep local"| D["e2e-tests<br/>translations.ts"]
  B -->|"Import from"| E["Test files<br/>conversation, mcp, ui"]
Loading

Grey Divider

File Changes

1. workspaces/lightspeed/e2e-tests/fixtures/mcpServerMocks.ts Refactoring +0/-202

Remove MCP server mock utilities

workspaces/lightspeed/e2e-tests/fixtures/mcpServerMocks.ts


2. workspaces/lightspeed/e2e-tests/fixtures/responses.ts Refactoring +0/-221

Remove response fixtures and mock data

workspaces/lightspeed/e2e-tests/fixtures/responses.ts


3. workspaces/lightspeed/e2e-tests/lightspeed.conversation.test.ts Refactoring +48/-58

Update imports to use package exports

workspaces/lightspeed/e2e-tests/lightspeed.conversation.test.ts


View more (26)
4. workspaces/lightspeed/e2e-tests/lightspeed.mcp.test.ts Refactoring +24/-31

Update imports to use package exports

workspaces/lightspeed/e2e-tests/lightspeed.mcp.test.ts


5. workspaces/lightspeed/e2e-tests/lightspeed.ui.test.ts Refactoring +38/-24

Update imports and fixture path resolution

workspaces/lightspeed/e2e-tests/lightspeed.ui.test.ts


6. workspaces/lightspeed/e2e-tests/pages/LightspeedPage.ts Refactoring +0/-457

Remove page object definitions

workspaces/lightspeed/e2e-tests/pages/LightspeedPage.ts


7. workspaces/lightspeed/e2e-tests/pages/McpConfigureTokenPage.ts Refactoring +0/-118

Remove MCP configuration page class

workspaces/lightspeed/e2e-tests/pages/McpConfigureTokenPage.ts


8. workspaces/lightspeed/e2e-tests/utils/accessibility.ts Refactoring +0/-41

Remove accessibility test utilities

workspaces/lightspeed/e2e-tests/utils/accessibility.ts


9. workspaces/lightspeed/e2e-tests/utils/chatManagement.ts Refactoring +0/-460

Remove chat management helper functions

workspaces/lightspeed/e2e-tests/utils/chatManagement.ts


10. workspaces/lightspeed/e2e-tests/utils/devMode.ts Refactoring +0/-286

Remove dev mode mock utilities

workspaces/lightspeed/e2e-tests/utils/devMode.ts


11. workspaces/lightspeed/e2e-tests/utils/fileUpload.ts Refactoring +0/-132

Remove file upload test utilities

workspaces/lightspeed/e2e-tests/utils/fileUpload.ts


12. workspaces/lightspeed/e2e-tests/utils/lightspeedE2eSetup.ts Refactoring +0/-79

Remove e2e bootstrap setup function

workspaces/lightspeed/e2e-tests/utils/lightspeedE2eSetup.ts


13. workspaces/lightspeed/e2e-tests/utils/localeSkip.ts Refactoring +0/-45

Remove locale skip utilities

workspaces/lightspeed/e2e-tests/utils/localeSkip.ts


14. workspaces/lightspeed/e2e-tests/utils/sidebar.ts Refactoring +0/-101

Remove sidebar interaction utilities

workspaces/lightspeed/e2e-tests/utils/sidebar.ts


15. workspaces/lightspeed/e2e-tests/utils/testHelper.ts Refactoring +0/-139

Remove test helper functions

workspaces/lightspeed/e2e-tests/utils/testHelper.ts


16. workspaces/lightspeed/e2e-tests/utils/translations.ts Refactoring +12/-49

Keep local translations, remove helper functions

workspaces/lightspeed/e2e-tests/utils/translations.ts


17. workspaces/lightspeed/e2e-tests/fixtures/uploads/de.upload1.json Refactoring +0/-4

Remove German upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/de.upload1.json


18. workspaces/lightspeed/e2e-tests/fixtures/uploads/de.upload2.json Refactoring +0/-4

Remove German upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/de.upload2.json


19. workspaces/lightspeed/e2e-tests/fixtures/uploads/en.upload1.json Refactoring +0/-4

Remove English upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/en.upload1.json


20. workspaces/lightspeed/e2e-tests/fixtures/uploads/en.upload2.json Refactoring +0/-4

Remove English upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/en.upload2.json


21. workspaces/lightspeed/e2e-tests/fixtures/uploads/es.upload1.json Refactoring +0/-4

Remove Spanish upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/es.upload1.json


22. workspaces/lightspeed/e2e-tests/fixtures/uploads/es.upload2.json Refactoring +0/-4

Remove Spanish upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/es.upload2.json


23. workspaces/lightspeed/e2e-tests/fixtures/uploads/fr.upload1.json Refactoring +0/-4

Remove French upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/fr.upload1.json


24. workspaces/lightspeed/e2e-tests/fixtures/uploads/fr.upload2.json Refactoring +0/-4

Remove French upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/fr.upload2.json


25. workspaces/lightspeed/e2e-tests/fixtures/uploads/it.upload1.json Refactoring +0/-4

Remove Italian upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/it.upload1.json


26. workspaces/lightspeed/e2e-tests/fixtures/uploads/it.upload2.json Refactoring +0/-4

Remove Italian upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/it.upload2.json


27. workspaces/lightspeed/e2e-tests/fixtures/uploads/ja.upload1.json Refactoring +0/-4

Remove Japanese upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/ja.upload1.json


28. workspaces/lightspeed/e2e-tests/fixtures/uploads/ja.upload2.json Refactoring +0/-4

Remove Japanese upload fixture file

workspaces/lightspeed/e2e-tests/fixtures/uploads/ja.upload2.json


29. workspaces/lightspeed/package.json Dependencies +2/-0

Add lightspeed-playwright-e2e package dependency

workspaces/lightspeed/package.json


Grey Divider

Qodo Logo

@rhdh-qodo-merge rhdh-qodo-merge Bot added enhancement New feature or request Tests labels Apr 20, 2026
@gabemontero
Copy link
Copy Markdown
Contributor

@thepetk @Jdubrick @yangcao77 @maysunfaisal @HusneShabbir - IIRC there has been discussions amongst at least some of you all wrt lightspeed e2e's

I'm curious if those conversations included more reuse between what is here and in https://github.com/redhat-developer/lightspeed-playwright-e2e vs. what we have in https://github.com/redhat-ai-dev/ai-rolling-demo-gitops/tree/main/tests

I do realize there is minimally a typescript vs. python difference ..... so maybe that means say replacing the python version in the rolling demo with the typescript version elsewhere

Anyway just curious from a due diligence perspective on some of the details that you all got into

thanks

@thepetk
Copy link
Copy Markdown
Contributor

thepetk commented Apr 20, 2026

@thepetk @Jdubrick @yangcao77 @maysunfaisal @HusneShabbir - IIRC there has been discussions amongst at least some of you all wrt lightspeed e2e's

I'm curious if those conversations included more reuse between what is here and in https://github.com/redhat-developer/lightspeed-playwright-e2e vs. what we have in https://github.com/redhat-ai-dev/ai-rolling-demo-gitops/tree/main/tests

I do realize there is minimally a typescript vs. python difference ..... so maybe that means say replacing the python version in the rolling demo with the typescript version elsewhere

Anyway just curious from a due diligence perspective on some of the details that you all got into

thanks

@gabemontero we have a sync tomorrow with @HusneShabbir & @jrichter1 to see how we can integrate the e2e tests here into the rolling demo suite! We'll follow up on our next weekly sync so everyone is up-to-speed!

@gabemontero
Copy link
Copy Markdown
Contributor

@thepetk @Jdubrick @yangcao77 @maysunfaisal @HusneShabbir - IIRC there has been discussions amongst at least some of you all wrt lightspeed e2e's
I'm curious if those conversations included more reuse between what is here and in https://github.com/redhat-developer/lightspeed-playwright-e2e vs. what we have in https://github.com/redhat-ai-dev/ai-rolling-demo-gitops/tree/main/tests
I do realize there is minimally a typescript vs. python difference ..... so maybe that means say replacing the python version in the rolling demo with the typescript version elsewhere
Anyway just curious from a due diligence perspective on some of the details that you all got into
thanks

@gabemontero we have a sync tomorrow with @HusneShabbir & @jrichter1 to see how we can integrate the e2e tests here into the rolling demo suite! We'll follow up on our next weekly sync so everyone is up-to-speed!

perfect thanks @thepetk

@HusneShabbir HusneShabbir requested a review from jrichter1 April 21, 2026 06:12
@HusneShabbir
Copy link
Copy Markdown
Contributor Author

Closing this PR based on the discussion with @jrichter1 and @thepetk. We agreed on starting with a simpler approach and considering shared code at a later stage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants