include tests/e2e in pnpm workspace for shared tooling#1084
include tests/e2e in pnpm workspace for shared tooling#1084NilukaSripalim wants to merge 14 commits intoasgardeo:mainfrom
Conversation
NilukaSripalim
requested review from
DonOmalVindula and
jeradrutnam
as code owners
There was a problem hiding this comment.
Pull request overview
This PR integrates the tests/e2e folder into the existing pnpm workspace to leverage shared tooling such as ESLint configurations, Prettier settings, and common dependencies from the @thunder packages. This consolidation helps maintain consistency across the project and avoids duplicate tool configurations.
Changes:
- Added
tests/e2eto the pnpm workspace configuration - Updated e2e test dependencies to use workspace packages and catalog versions
- Replaced custom ESLint rules with shared Thunder ESLint plugin configuration
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/pnpm-workspace.yaml | Adds tests/e2e to the workspace using a relative path |
| tests/e2e/package.json | Replaces hardcoded versions with workspace dependencies and catalog references |
| tests/e2e/eslint.config.mjs | Imports and uses shared Thunder ESLint plugin, removing duplicate rule definitions |
tests/e2e/eslint.config.mjs
Outdated
| import tsParser from "@typescript-eslint/parser"; | ||
|
|
||
| export default [ | ||
| ...thunderPlugin.configs.typescript, |
There was a problem hiding this comment.
The Thunder ESLint plugin does not export a typescript config. Based on the plugin's source code, only base and react configs are available. Since this is a test project without React components, use ...thunderPlugin.configs.base instead.
| ...thunderPlugin.configs.typescript, | |
| ...thunderPlugin.configs.base, |
tests/e2e/eslint.config.mjs
Outdated
| plugins: { | ||
| "@typescript-eslint": typescriptEslint, | ||
| playwright: playwright, | ||
| }, |
There was a problem hiding this comment.
The @typescript-eslint plugin has been removed from the plugins object, but the languageOptions still reference the TypeScript parser. Since the Thunder base config already includes TypeScript support, verify that the parser and parserOptions configuration from lines 15-21 is necessary or if it can be simplified to avoid conflicting with the shared config.
NilukaSripalim
added
the
skip-changelog
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1084 +/- ##
==========================================
- Coverage 86.78% 86.76% -0.02%
==========================================
Files 526 526
Lines 35078 35078
Branches 1611 1611
==========================================
- Hits 30441 30436 -5
- Misses 2930 2936 +6
+ Partials 1707 1706 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
.github/workflows/pr-builder.yml
Outdated
| - name: � Install pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: '9.x' | ||
| run_install: false | ||
|
|
||
| - name: �📥 Download Built Distribution |
There was a problem hiding this comment.
The emoji character appears corrupted or not properly encoded. Should be a package/box emoji like 📦.
| - name: � Install pnpm | |
| uses: pnpm/action-setup@v4 | |
| with: | |
| version: '9.x' | |
| run_install: false | |
| - name: �📥 Download Built Distribution | |
| - name: 📦 Install pnpm | |
| uses: pnpm/action-setup@v4 | |
| with: | |
| version: '9.x' | |
| run_install: false | |
| - name: 📦📥 Download Built Distribution |
.github/workflows/pr-builder.yml
Outdated
| - name: � Install pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: '9.x' | ||
| run_install: false | ||
|
|
||
| - name: �📥 Download Built Distribution |
There was a problem hiding this comment.
The emoji character appears corrupted or not properly encoded. Should be a single emoji.
| - name: � Install pnpm | |
| uses: pnpm/action-setup@v4 | |
| with: | |
| version: '9.x' | |
| run_install: false | |
| - name: �📥 Download Built Distribution | |
| - name: 📦 Install pnpm | |
| uses: pnpm/action-setup@v4 | |
| with: | |
| version: '9.x' | |
| run_install: false | |
| - name: 📥 Download Built Distribution |
tests/e2e/package.json
Outdated
| "@typescript-eslint/eslint-plugin": "8.0.0", | ||
| "@typescript-eslint/parser": "8.0.0", |
There was a problem hiding this comment.
These TypeScript-ESLint dependencies appear to be redundant since the workspace package @thunder/eslint-plugin-thunder already includes TypeScript-ESLint support in its configs.base. Consider removing these dependencies as they're already provided transitively through the shared plugin.
| "@typescript-eslint/eslint-plugin": "8.0.0", | |
| "@typescript-eslint/parser": "8.0.0", |
tests/e2e/package.json
Outdated
| "@thunder/eslint-plugin-thunder": "workspace:^", | ||
| "@thunder/prettier-config": "workspace:^", |
There was a problem hiding this comment.
The package.json now references the shared @thunder/prettier-config package, but the existing .prettierrc.json file should be replaced with a prettier.config.js file that imports this shared config (similar to how frontend/apps/thunder-gate/prettier.config.js uses it). The .prettierrc.json file uses different configuration values (e.g., singleQuote: false vs true, arrowParens: avoid vs always) which will cause inconsistency.
tests/e2e/eslint.config.mjs
Outdated
| @@ -1,42 +1,17 @@ | |||
| import typescriptEslint from "@typescript-eslint/eslint-plugin"; | |||
| import thunderPlugin from "@thunder/eslint-plugin-thunder"; | |||
There was a problem hiding this comment.
The new eslint.config.mjs migrates to use the shared Thunder ESLint plugin, but the old .eslintrc.json file still exists in the tests/e2e directory. This should be removed to avoid configuration conflicts and confusion about which ESLint config is active.
tests/e2e/package.json
Outdated
| "@thunder/eslint-plugin-thunder": "workspace:^", | ||
| "@thunder/prettier-config": "workspace:^", |
There was a problem hiding this comment.
Since the project is now using pnpm workspace and the package-lock.json was generated by npm, the package-lock.json file in tests/e2e directory should be removed and replaced with pnpm-lock.yaml at the workspace root level (frontend/pnpm-lock.yaml) after running 'pnpm install'.
.github/workflows/pr-builder.yml
Outdated
| env: | ||
| NODE_PATH: ../../node_modules | ||
|
|
||
| - name: Run Playwright tests | ||
| - name: 🎭 Run Playwright Tests | ||
| run: npx playwright test | ||
| working-directory: tests/e2e | ||
| env: | ||
| NODE_PATH: ../../node_modules |
There was a problem hiding this comment.
The NODE_PATH environment variable may not be necessary since tests/e2e is now part of the pnpm workspace. pnpm manages node_modules resolution through symlinks automatically. Verify if NODE_PATH is actually required for proper module resolution, or if it can be removed for cleaner configuration.
.github/workflows/pr-builder.yml
Outdated
| - name: � Install pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: '9.x' | ||
| run_install: false | ||
|
|
||
| - name: �📥 Download Built Distribution |
There was a problem hiding this comment.
The step name contains a corrupted/invisible character. The emoji should be properly displayed. Check the encoding of this line.
| - name: � Install pnpm | |
| uses: pnpm/action-setup@v4 | |
| with: | |
| version: '9.x' | |
| run_install: false | |
| - name: �📥 Download Built Distribution | |
| - name: 📦 Install pnpm | |
| uses: pnpm/action-setup@v4 | |
| with: | |
| version: '9.x' | |
| run_install: false | |
| - name: 📦 Download Built Distribution |
.github/workflows/pr-builder.yml
Outdated
| - name: � Install pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: '9.x' | ||
| run_install: false | ||
|
|
||
| - name: �📥 Download Built Distribution |
There was a problem hiding this comment.
The step name contains corrupted/invisible characters at the beginning. The emoji should be properly displayed. Check the encoding of this line.
| - name: � Install pnpm | |
| uses: pnpm/action-setup@v4 | |
| with: | |
| version: '9.x' | |
| run_install: false | |
| - name: �📥 Download Built Distribution | |
| - name: 📦 Install pnpm | |
| uses: pnpm/action-setup@v4 | |
| with: | |
| version: '9.x' | |
| run_install: false | |
| - name: 📥 Download Built Distribution |
tests/e2e/eslint.config.mjs
Outdated
| @@ -1,42 +1,17 @@ | |||
| import typescriptEslint from "@typescript-eslint/eslint-plugin"; | |||
| import thunderPlugin from "@thunder/eslint-plugin-thunder"; | |||
There was a problem hiding this comment.
The old .eslintrc.json file should be removed since the configuration has been migrated to eslint.config.mjs. Having both configuration files can cause confusion and conflicts.
tests/e2e/eslint.config.mjs
Outdated
| @@ -1,42 +1,17 @@ | |||
| import typescriptEslint from "@typescript-eslint/eslint-plugin"; | |||
| import thunderPlugin from "@thunder/eslint-plugin-thunder"; | |||
There was a problem hiding this comment.
The .prettierrc.json file should be removed or replaced with a prettier.config.js that imports the shared @thunder/prettier-config package. Having a local .prettierrc.json file will override the shared configuration and defeat the purpose of centralizing tooling configuration. Create a prettier.config.js file similar to the one in frontend/prettier.config.js that imports from "@thunder/prettier-config".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- tests/e2e/package-lock.json: Language not supported
- tests/e2e/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
.github/workflows/pr-builder.yml:1
- The step name contains a corrupted emoji character '�' before 'Install pnpm'. This should be cleaned up.
# This workflow will build the project on pull requests with tests
| "node", | ||
| "@playwright/test" | ||
| ], | ||
| "moduleResolution": "node", |
There was a problem hiding this comment.
The change from moduleResolution: 'bundler' to 'node' may cause module resolution issues if the E2E tests were previously relying on bundler-specific resolution patterns. This change should be verified with the actual test execution to ensure all imports still resolve correctly.
| "moduleResolution": "node", | |
| "moduleResolution": "bundler", |
|
|
||
|
|
There was a problem hiding this comment.
An extra blank line was added without apparent purpose. While this doesn't affect functionality, it seems unintentional and should likely be removed to keep the file clean. Additionally, this file is in the frontend/ directory but the PR description mentions adding tests/e2e to the workspace - there's no visible change here that adds tests/e2e to any workspace configuration.
| - ../tests/e2e |
.github/workflows/pr-builder.yml
Outdated
| - name: ⚙️ Set up Node.js 24.x | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '24.x' | ||
|
|
There was a problem hiding this comment.
Node.js is being set up twice in this workflow - once at line 305 with 'lts/*' and again here with '24.x'. This duplication is unnecessary and could cause confusion. Consider removing one of these steps or clarifying why different Node.js versions are needed at different stages.
| - name: ⚙️ Set up Node.js 24.x | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: '24.x' |
| run: | | ||
| cd frontend | ||
| pnpm install --frozen-lockfile | ||
|
|
There was a problem hiding this comment.
The workflow installs dependencies from the frontend directory (which includes tests/e2e through the workspace), but then runs Playwright commands directly in tests/e2e. This creates an implicit dependency on the workspace installation happening in frontend/. Consider adding a comment explaining this workspace relationship, or verify that the working-directory for Playwright commands can access the node_modules from the workspace root.
| run: | | |
| cd frontend | |
| pnpm install --frozen-lockfile | |
| run: | | |
| # Note: ./frontend is the pnpm workspace root. Running `pnpm install` | |
| # here installs dependencies for all workspace packages, including | |
| # the tests/e2e package used by the Playwright steps below. | |
| cd frontend | |
| pnpm install --frozen-lockfile | |
| # Note: tests/e2e is part of the pnpm workspace rooted at ./frontend, | |
| # so these Playwright commands rely on node_modules installed in the | |
| # previous "Install E2E Dependencies" step. |
| "express": "^5.2.1", | ||
| "prettier": "3.4.2", | ||
| "typescript": "5.7.3" | ||
| "prettier": "3.6.2", |
There was a problem hiding this comment.
Prettier has been downgraded from 3.4.2 to 3.6.2, but version 3.6.2 is actually newer than 3.4.2. However, this creates a potential version mismatch if the workspace root or other packages use a different Prettier version. Since one goal of this PR is to share tooling across the workspace, consider removing this devDependency entirely and relying on the workspace's shared Prettier installation for consistency.
| "prettier": "3.6.2", |
|
With #1086, this flow has been updated in a more efficient way. |
2 participants
Purpose
This PR adds the tests/e2e folder to the existing pnpm workspace so it can reuse shared tools such as linters, Prettier, and other common dependencies. This helps keep tooling consistent across the project and avoids managing duplicate configurations.
🔧 Summary of Breaking Changes
Describe what is changing
💥 Impact
What will break? Who is affected?
🔄 Migration Guide
How should users update their code/configuration to adapt to the breaking changes? Include examples if helpful
-->
Approach
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks