chrome only #99
chrome only #99
Conversation
|
Warning Rate limit exceeded@shashwatahalder01 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 33 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces modifications to the end-to-end (E2E) and API testing workflow. Changes span across four files: the GitHub Actions workflow configuration, Playwright test configuration, an admin login test specification, and the package.json scripts. The primary alterations involve simplifying the testing setup by removing specific environment preparation steps and dependencies, broadening the browser installation command, and focusing on a single admin login test case. These changes significantly alter the functional flow of the testing process. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/e2e_api_tests.yml(2 hunks)tests/pw/e2e.config.ts(1 hunks)tests/pw/tests/e2e/admin.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e_api tests
🔇 Additional comments (1)
.github/workflows/e2e_api_tests.yml (1)
99-104: Verify Chrome-only test coverage is sufficient.Running only Chrome tests without cross-browser testing could miss browser-specific issues. Consider maintaining a minimal set of tests for other browsers.
| @@ -146,7 +146,7 @@ export default defineConfig({ | |||
| name: 'e2e_tests', | |||
| testMatch: /.*\.spec\.ts/, | |||
| /* whether not to run setup tests before running actual tests */ | |||
| dependencies: NO_SETUP ? [] : ['e2e_setup'], | |||
| // dependencies: NO_SETUP ? [] : ['e2e_setup'], | |||
There was a problem hiding this comment.
💡 Codebase verification
Keep the e2e_setup dependency to maintain test reliability
The e2e_setup dependency is part of a carefully designed test initialization chain:
- The setup files (_env.setup.ts, _site.setup.ts, etc.) contain crucial environment initialization
- Other test projects maintain their setup dependencies using NO_SETUP flag
- Breaking this chain by commenting out the dependency could lead to inconsistent test environments
🔗 Analysis chain
Verify test dependencies before removing setup phase.
Commenting out the e2e_setup dependency could lead to test failures if the environment is not properly initialized. Consider keeping the dependency and using the existing NO_SETUP environment variable instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any tests depend on environment setup
# Look for setup-dependent operations in test files
# Search for environment-dependent operations
rg -l "beforeAll|beforeEach" tests/e2e/
Length of output: 92
Script:
#!/bin/bash
# First, find test files to understand the directory structure
fd -e ts -e js test
# Then look for setup patterns in the test files
rg -l "beforeAll|beforeEach|setup" tests/pw/
# Check the content of the config file for context
cat tests/pw/e2e.config.ts
Length of output: 12654
| @@ -3,7 +3,7 @@ import { LoginPage } from '@pages/loginPage'; | |||
| import { data } from '@utils/testData'; | |||
|
|
|||
| test.describe('Admin functionality test', () => { | |||
| test('admin can login', { tag: ['@lite', '@admin'] }, async ({ page }) => { | |||
| test.only('admin can login', { tag: ['@lite', '@admin'] }, async ({ page }) => { | |||
There was a problem hiding this comment.
Remove .only to enable all tests.
Using test.only will skip other important tests in the suite, including the logout test. This appears to be a debugging artifact and should be removed before merging.
- test.only('admin can login', { tag: ['@lite', '@admin'] }, async ({ page }) => {
+ test('admin can login', { tag: ['@lite', '@admin'] }, async ({ page }) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.only('admin can login', { tag: ['@lite', '@admin'] }, async ({ page }) => { | |
| test('admin can login', { tag: ['@lite', '@admin'] }, async ({ page }) => { |
.github/workflows/e2e_api_tests.yml
Outdated
| # Prepare test summary | ||
| - name: Prepare test summary | ||
| id: prepare-test-summary | ||
| uses: actions/github-script@v7 | ||
| if: always() && steps.clone-dokan-lite.outcome == 'success' | ||
| with: | ||
| result-encoding: string | ||
| script: | | ||
| const script = require("./tests/pw/utils/gitTestSummary.ts") | ||
| return await script({github, context, core}) | ||
|
|
||
| # # Find PR comment | ||
| # - name: Find PR comment by github-actions[bot] | ||
| # id: find-comment | ||
| # if: github.event_name == 'pull_request' | ||
| # uses: peter-evans/find-comment@v3 | ||
| # # Prepare test summary | ||
| # - name: Prepare test summary | ||
| # id: prepare-test-summary | ||
| # uses: actions/github-script@v7 | ||
| # if: always() && steps.clone-dokan-lite.outcome == 'success' | ||
| # with: | ||
| # issue-number: ${{ github.event.pull_request.number }} | ||
| # comment-author: 'github-actions[bot]' | ||
| # body-includes: Tests Summary | ||
|
|
||
| # # Post test summary as PR comment | ||
| # - name: Create or update PR comment | ||
| # if: github.event_name == 'pull_request' | ||
| # uses: peter-evans/create-or-update-comment@v4 | ||
| # with: | ||
| # comment-id: ${{ steps.find-comment.outputs.comment-id }} | ||
| # issue-number: ${{ github.event.pull_request.number }} | ||
| # body: ${{ steps.prepare-test-summary.outputs.result }} | ||
| # reactions: hooray | ||
| # edit-mode: replace | ||
|
|
||
| # Backup Database | ||
| - name: Backup Database | ||
| if: always() && steps.wp-env.outcome == 'success' | ||
| working-directory: tests/pw | ||
| run: | | ||
| npm run wp-env run tests-cli wp db export wp-data/db.sql | ||
| # result-encoding: string | ||
| # script: | | ||
| # const script = require("./tests/pw/utils/gitTestSummary.ts") | ||
| # return await script({github, context, core}) | ||
|
|
||
| # # # Find PR comment | ||
| # # - name: Find PR comment by github-actions[bot] | ||
| # # id: find-comment | ||
| # # if: github.event_name == 'pull_request' | ||
| # # uses: peter-evans/find-comment@v3 | ||
| # # with: | ||
| # # issue-number: ${{ github.event.pull_request.number }} | ||
| # # comment-author: 'github-actions[bot]' | ||
| # # body-includes: Tests Summary | ||
|
|
||
| # # # Post test summary as PR comment | ||
| # # - name: Create or update PR comment | ||
| # # if: github.event_name == 'pull_request' | ||
| # # uses: peter-evans/create-or-update-comment@v4 | ||
| # # with: | ||
| # # comment-id: ${{ steps.find-comment.outputs.comment-id }} | ||
| # # issue-number: ${{ github.event.pull_request.number }} | ||
| # # body: ${{ steps.prepare-test-summary.outputs.result }} | ||
| # # reactions: hooray | ||
| # # edit-mode: replace | ||
|
|
||
| # # Backup Database | ||
| # - name: Backup Database | ||
| # if: always() && steps.wp-env.outcome == 'success' | ||
| # working-directory: tests/pw | ||
| # run: | | ||
| # npm run wp-env run tests-cli wp db export wp-data/db.sql | ||
|
|
||
| # Upload artifacts | ||
| - name: Archive test artifacts (screenshots, HTML snapshots, Reports) | ||
| if: always() && steps.debug-log.outcome == 'success' | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: test-artifact | ||
| path: | | ||
| tests/pw/wp-data | ||
| tests/pw/playwright | ||
| tests/pw/playwright-report | ||
| if-no-files-found: ignore | ||
| retention-days: 30 | ||
| # # Upload artifacts | ||
| # - name: Archive test artifacts (screenshots, HTML snapshots, Reports) | ||
| # if: always() && steps.debug-log.outcome == 'success' | ||
| # uses: actions/upload-artifact@v4 | ||
| # with: | ||
| # name: test-artifact | ||
| # path: | | ||
| # tests/pw/wp-data | ||
| # tests/pw/playwright | ||
| # tests/pw/playwright-report | ||
| # if-no-files-found: ignore | ||
| # retention-days: 30 |
There was a problem hiding this comment.
Maintain test coverage and reporting.
Removing test coverage collection, API tests, and artifacts upload significantly reduces visibility into test results and makes debugging harder. Consider:
- Keeping coverage reporting for Chrome tests
- Maintaining API tests as they are browser-independent
- Preserving test artifacts for debugging
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/e2e_api_tests.yml (3)
106-145:⚠️ Potential issueRestore critical setup steps
The removal of essential setup steps could lead to test instability and slower execution:
- Debug logging is crucial for troubleshooting test failures
- Browser binary caching improves workflow performance
- OS dependencies are required for browser automation
168-173:⚠️ Potential issueRestore test coverage collection
Test coverage reporting is essential for maintaining code quality and identifying untested code paths.
175-182:⚠️ Potential issueRestore API test execution
API tests should not be disabled as they:
- Are browser-independent and unaffected by Chrome-only changes
- Provide critical coverage of backend functionality
🧹 Nitpick comments (1)
.github/workflows/e2e_api_tests.yml (1)
Line range hint
1-234: Scope of changes exceeds Chrome-only modificationsThe changes in this workflow file go far beyond the stated PR objective of "chrome only" modifications. The removal of critical testing infrastructure (coverage, API tests, artifacts, reporting) will significantly impact:
- Test reliability and debugging capabilities
- Code quality visibility
- PR review effectiveness
Please limit changes to Chrome-specific modifications and maintain the existing testing infrastructure.
Consider creating a Chrome-specific job instead of modifying the existing infrastructure:
jobs: chrome_tests: name: chrome_only_tests runs-on: ubuntu-latest steps: # Chrome-specific steps here - name: Install Chrome run: npx playwright install chromium # Rest of the steps...
.github/workflows/e2e_api_tests.yml
Outdated
| # Install test dependencies | ||
| # npm run pw:chrome-only 8s | ||
| - name: Install browsers | ||
| working-directory: tests/pw | ||
| run: | | ||
| mkdir -p wp-data && touch wp-data/debug.log | ||
| pw:browser-with-deps | ||
|
|
There was a problem hiding this comment.
Fix browser installation configuration
The browser installation step has several issues:
- The command
pw:browser-with-depsappears to be malformed (line 104) - Chrome-specific configuration is missing despite PR objectives
- Browser binary caching has been removed, which may slow down workflow execution
Apply this fix:
# Install browsers
- name: Install browsers
working-directory: tests/pw
run: |
- pw:browser-with-deps
+ npx playwright install chromium
+ npx playwright install-deps chromium📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Install test dependencies | |
| # npm run pw:chrome-only 8s | |
| - name: Install browsers | |
| working-directory: tests/pw | |
| run: | | |
| mkdir -p wp-data && touch wp-data/debug.log | |
| pw:browser-with-deps | |
| # Install test dependencies | |
| # npm run pw:chrome-only 8s | |
| - name: Install browsers | |
| working-directory: tests/pw | |
| run: | | |
| npx playwright install chromium | |
| npx playwright install-deps chromium |
.github/workflows/e2e_api_tests.yml
Outdated
| # # Prepare test summary | ||
| # - name: Prepare test summary | ||
| # id: prepare-test-summary | ||
| # uses: actions/github-script@v7 | ||
| # if: always() && steps.clone-dokan-lite.outcome == 'success' | ||
| # with: | ||
| # comment-id: ${{ steps.find-comment.outputs.comment-id }} | ||
| # issue-number: ${{ github.event.pull_request.number }} | ||
| # body: ${{ steps.prepare-test-summary.outputs.result }} | ||
| # reactions: hooray | ||
| # edit-mode: replace | ||
|
|
||
| # Backup Database | ||
| - name: Backup Database | ||
| if: always() && steps.wp-env.outcome == 'success' | ||
| working-directory: tests/pw | ||
| run: | | ||
| npm run wp-env run tests-cli wp db export wp-data/db.sql | ||
| # result-encoding: string | ||
| # script: | | ||
| # const script = require("./tests/pw/utils/gitTestSummary.ts") | ||
| # return await script({github, context, core}) | ||
|
|
||
| # # # Find PR comment | ||
| # # - name: Find PR comment by github-actions[bot] | ||
| # # id: find-comment | ||
| # # if: github.event_name == 'pull_request' | ||
| # # uses: peter-evans/find-comment@v3 | ||
| # # with: | ||
| # # issue-number: ${{ github.event.pull_request.number }} | ||
| # # comment-author: 'github-actions[bot]' | ||
| # # body-includes: Tests Summary | ||
|
|
||
| # # # Post test summary as PR comment | ||
| # # - name: Create or update PR comment | ||
| # # if: github.event_name == 'pull_request' | ||
| # # uses: peter-evans/create-or-update-comment@v4 | ||
| # # with: | ||
| # # comment-id: ${{ steps.find-comment.outputs.comment-id }} | ||
| # # issue-number: ${{ github.event.pull_request.number }} | ||
| # # body: ${{ steps.prepare-test-summary.outputs.result }} | ||
| # # reactions: hooray | ||
| # # edit-mode: replace | ||
|
|
||
| # # Backup Database | ||
| # - name: Backup Database | ||
| # if: always() && steps.wp-env.outcome == 'success' | ||
| # working-directory: tests/pw | ||
| # run: | | ||
| # npm run wp-env run tests-cli wp db export wp-data/db.sql | ||
|
|
||
| # Upload artifacts | ||
| - name: Archive test artifacts (screenshots, HTML snapshots, Reports) | ||
| if: always() && steps.debug-log.outcome == 'success' | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: test-artifact | ||
| path: | | ||
| tests/pw/wp-data | ||
| tests/pw/playwright | ||
| tests/pw/playwright-report | ||
| if-no-files-found: ignore | ||
| retention-days: 30 | ||
| # # Upload artifacts | ||
| # - name: Archive test artifacts (screenshots, HTML snapshots, Reports) | ||
| # if: always() && steps.debug-log.outcome == 'success' | ||
| # uses: actions/upload-artifact@v4 | ||
| # with: | ||
| # name: test-artifact | ||
| # path: | | ||
| # tests/pw/wp-data | ||
| # tests/pw/playwright | ||
| # tests/pw/playwright-report | ||
| # if-no-files-found: ignore | ||
| # retention-days: 30 |
There was a problem hiding this comment.
💡 Codebase verification
Restore test results reporting and artifacts - Critical for Test Visibility
The commented-out reporting features in e2e_api_tests.yml represent the only test result reporting mechanism in the entire workflow setup. Removing these would:
- Eliminate all visibility into test execution results
- Remove the only source of debugging artifacts (screenshots, logs)
- Break the established test reporting pipeline that tracks API and E2E test results
🔗 Analysis chain
Restore test results reporting and artifacts
Removing test results reporting and artifacts severely impacts the ability to:
- Debug test failures through screenshots and logs
- Track test execution results in PR comments
- Maintain database backups for issue reproduction
These features are critical for maintaining test quality and should not be removed, regardless of Chrome-only changes.
Run this script to check if similar workflows maintain these features:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other test workflows maintain result reporting
# Look for patterns indicating test reporting in other workflow files
rg -l 'upload-artifact|create-or-update-comment|test.*summary' .github/workflows/
Length of output: 119
Script:
#!/bin/bash
# Find all workflow files with test-related content
echo "=== Workflow files with test-related content ==="
rg -l 'test|playwright|jest|phpunit|wp-env' .github/workflows/
echo -e "\n=== Alternative reporting mechanisms ==="
# Look for alternative reporting mechanisms
rg -i '(report|summary|result|artifact)' .github/workflows/ -A 3 -B 3
Length of output: 5260
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/e2e_api_tests.yml (2)
99-144:⚠️ Potential issueRestore critical browser setup steps
Commenting out browser installation and caching steps will cause test failures. These steps are essential for:
- Installing browser binaries
- Setting up OS dependencies
- Optimizing CI performance through caching
Restore these steps with Chrome-specific configuration:
- # # Install browsers - # - name: Install browsers - # working-directory: tests/pw - # run: | - # npm run pw:browser-with-deps + # Install browsers + - name: Install browsers + working-directory: tests/pw + run: | + npm run pw:chrome-only + # Get Playwright version + - name: Get installed Playwright version + id: playwright-version + working-directory: tests/pw + run: | + echo "PLAYWRIGHT_VERSION=$(npm ls @playwright/test --json | jq --raw-output '.dependencies["@playwright/test"].version')" >> $GITHUB_ENV + # Cache browser binaries + - name: Cache playwright binaries + id: playwright-cache + uses: actions/cache@v4 + with: + path: ~/.cache/ms-playwright + key: ${{ runner.os }}-playwright-${{ env.PLAYWRIGHT_VERSION }} + restore-keys: | + ${{ runner.os }}-playwright- + # Install OS dependencies if cache missed + - name: Install Playwright OS dependencies + if: steps.playwright-cache.outputs.cache-hit != 'true' + working-directory: tests/pw + run: | + npm run pw:deps-only
171-237:⚠️ Potential issueCritical: Restore test reporting and artifacts
Removing test coverage collection, reporting, and artifacts upload significantly impacts:
- Visibility into test results
- Ability to debug test failures
- Historical test data for tracking regressions
These features are essential regardless of browser choice. Restore them with the following modifications:
- # # Run e2e coverage - # - name: 🧪 Run e2e coverage + # Run e2e coverage + - name: 🧪 Run e2e coverage + if: always() && (steps.e2e-test.outcome == 'success' || steps.e2e-test.outcome == 'failure') + working-directory: tests/pw + run: | + npm run test:e2e:coverage + # Prepare test summary + - name: Prepare test summary + id: prepare-test-summary + uses: actions/github-script@v7 + if: always() && steps.clone-dokan-lite.outcome == 'success' + with: + result-encoding: string + script: | + const script = require("./tests/pw/utils/gitTestSummary.ts") + return await script({github, context, core}) + # Upload artifacts + - name: Archive test artifacts + if: always() + uses: actions/upload-artifact@v4 + with: + name: test-artifact + path: | + tests/pw/playwright + tests/pw/playwright-report + if-no-files-found: ignore + retention-days: 30
🧹 Nitpick comments (1)
.github/workflows/e2e_api_tests.yml (1)
72-72: Remove trailing spacesLine 72 contains trailing spaces which should be removed to maintain consistent formatting.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 72-72: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/e2e_api_tests.yml(3 hunks)tests/pw/package.json(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/e2e_api_tests.yml
[error] 72-72: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e_api tests
| @@ -7,7 +7,7 @@ | |||
| "nid": "npm install --include=dev", | |||
| "nip": "npm install --omit=dev", | |||
| "pw:chrome-only": "playwright install chromium", | |||
| "pw:browser-with-deps": "playwright install --with-deps chromium", | |||
| "pw:browser-with-deps": "playwright install --with-deps", | |||
There was a problem hiding this comment.
Align browser installation with Chrome-only objective
The current change installs all browsers, which contradicts the PR's "chrome only" objective and increases CI time unnecessarily.
Apply this fix to install only Chrome:
- "pw:browser-with-deps": "playwright install --with-deps",
+ "pw:browser-with-deps": "playwright install --with-deps chromium"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "pw:browser-with-deps": "playwright install --with-deps", | |
| "pw:browser-with-deps": "playwright install --with-deps chromium", |
.github/workflows/e2e_api_tests.yml
Outdated
| docker run --rm \ | ||
| -v ${{ github.workspace }}:/workspace \ | ||
| -w /workspace \ | ||
| mcr.microsoft.com/playwright:v1.39.0 \ | ||
| npm run test:e2e |
There was a problem hiding this comment.
💡 Codebase verification
Update Docker image to match Playwright version
The Docker image mcr.microsoft.com/playwright:v1.39.0 is significantly outdated compared to the project's Playwright version (v1.49). This mismatch may cause compatibility issues and browser installation problems. Please update the Docker image version to match the project's Playwright version.
- Update
.github/workflows/e2e_api_tests.ymlto usemcr.microsoft.com/playwright:v1.49.0 - Alternatively, pin the project's
@playwright/testversion to match the Docker image if downgrading is preferred
🔗 Analysis chain
Verify Docker image compatibility
The switch to using Docker for test execution needs verification:
- The specified Playwright version (v1.39.0) is outdated
- The Docker setup might affect browser installation and caching
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest Playwright Docker image version
curl -s https://registry.hub.docker.com/v2/repositories/mcr.microsoft.com/playwright/tags | jq -r '.results[].name' | grep -v latest | head -n 1
# Verify if the repository uses a specific Playwright version
grep '"@playwright/test"' tests/pw/package.json
Length of output: 290
Script:
#!/bin/bash
# Check latest Playwright Docker image version with correct registry
curl -s https://mcr.microsoft.com/v2/playwright/tags/list | jq .
# Look for browser installation or configuration files
fd -e config.ts -e config.js -p tests/pw
rg -l "browser.launch|browserType.launch" tests/pw
Length of output: 69836
Script:
#!/bin/bash
# Look for Playwright config and browser setup files
fd -e ts -e js -e json -p tests/pw | rg -l "playwright|browser"
# Check if there are any browser installation scripts
rg -l "install.*browser|browser.*install" tests/pw
Length of output: 148
Script:
#!/bin/bash
# Check package.json content for browser setup
cat tests/pw/package.json
# Look for any browser-related npm scripts
rg -A 5 -B 5 '"scripts"' tests/pw/package.json
Length of output: 4126
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/e2e_api_tests.yml (3)
99-104:⚠️ Potential issueBrowser installation step should not be removed
The browser installation step is critical for test execution. While the PR aims for Chrome-only testing, the installation step should be modified rather than removed.
Apply this fix:
- # # Install test dependencies - # - name: Install browsers - # working-directory: tests/pw - # run: | - # npm run pw:browser-with-deps + # Install Chrome browser + - name: Install Chrome browser + working-directory: tests/pw + run: | + npx playwright install chromium + npx playwright install-deps chromium
173-188:⚠️ Potential issueRestore API tests and coverage collection
The removal of API tests and coverage collection significantly reduces test quality:
- API tests are browser-independent and should be retained
- Coverage metrics are essential for detecting regressions
These features should be restored as they are independent of the Chrome-only changes.
189-239:⚠️ Potential issueCritical: Restore test reporting and artifacts
The removal of test reporting, database backups, and artifacts upload severely impacts the ability to:
- Track test results
- Debug test failures
- Maintain test history
These features are essential for maintaining test quality and should be restored, regardless of Chrome-only changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_api_tests.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e_api tests
🔇 Additional comments (1)
.github/workflows/e2e_api_tests.yml (1)
165-170: Verify Docker configuration and network connectivityWhile using Docker provides isolation, ensure:
- The container has access to the WordPress environment (
--network host)- File permissions are correctly handled for test artifacts
- Browser installation works within the container
Run this script to verify the Docker setup:
✅ Verification successful
Docker configuration is properly set ✅
The Docker setup in the workflow is correctly configured with:
- Official Playwright Docker image with pre-installed browsers
- Host network mode for proper connectivity
- Correct volume mounting and working directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify Docker configuration echo "=== Testing Docker network access ===" docker run --rm --network host mcr.microsoft.com/playwright:v1.49.1-noble curl -s http://localhost:9999 echo "=== Verifying Playwright installation in container ===" docker run --rm mcr.microsoft.com/playwright:v1.49.1-noble playwright --versionLength of output: 585
Script:
#!/bin/bash # Check for similar Docker configurations in other workflow files echo "=== Checking other workflow files ===" fd -e yml -e yaml . .github/workflows --exec cat {} echo "=== Checking for documentation ===" fd -g "README*" -g "CONTRIBUTING*" -g "DEVELOPMENT*" . -x cat {}Length of output: 49071
.github/workflows/e2e_api_tests.yml
Outdated
| # # Create wp debuglog file | ||
| # - name: Create wp debuglog file | ||
| # id: debug-log | ||
| # working-directory: tests/pw | ||
| # run: | | ||
| # mkdir -p wp-data && touch wp-data/debug.log | ||
|
|
||
|
|
||
| # # Get Playwright version | ||
| # - name: Get installed Playwright version | ||
| # id: playwright-version | ||
| # working-directory: tests/pw | ||
| # run: | | ||
| # echo "PLAYWRIGHT_VERSION=$(npm ls @playwright/test --json | jq --raw-output '.dependencies["@playwright/test"].version')" >> $GITHUB_ENV | ||
|
|
||
| # # Cache browser binaries, cache key is based on Playwright version and OS | ||
| # - name: Cache playwright binaries | ||
| # id: playwright-cache | ||
| # uses: actions/cache@v4 | ||
| # with: | ||
| # path: | | ||
| # ~/.cache/ms-playwright | ||
| # key: ${{ runner.os }}-playwright-${{ env.PLAYWRIGHT_VERSION }} | ||
| # restore-keys: | | ||
| # ${{ runner.os }}-playwright- | ||
|
|
||
| # # Install browser binaries & OS dependencies if cache missed | ||
| # - name: Install Playwright browser binaries & OS dependencies | ||
| # id: pw-install | ||
| # if: steps.playwright-cache.outputs.cache-hit != 'true' | ||
| # working-directory: tests/pw | ||
| # run: | | ||
| # npm run pw:browser-with-deps | ||
|
|
||
| # # Install only the OS dependencies if cache hit not needed | ||
| # - name: Install Playwright OS dependencies | ||
| # if: steps.playwright-cache.outputs.cache-hit == 'true' | ||
| # working-directory: tests/pw | ||
| # run: | | ||
| # npm run pw:deps-only |
There was a problem hiding this comment.
Critical setup and debugging steps should be restored
Removing debug logs, caching, and OS dependencies setup will:
- Make test failures harder to debug
- Slow down workflow execution
- Risk inconsistent test environments
These steps should be restored with Chrome-specific configurations rather than removed entirely. The caching mechanism is particularly important for workflow efficiency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/e2e_api_tests.yml (2)
99-104:⚠️ Potential issueFix browser installation for Chrome-only testing
The browser installation step needs to be restored with Chrome-specific configuration to align with the PR objectives.
Apply this fix:
-# # Install test dependencies -# - name: Install browsers -# working-directory: tests/pw -# run: | -# npm run pw:browser-with-deps +# Install Chrome browser +- name: Install Chrome browser + working-directory: tests/pw + run: | + npx playwright install chromium + npx playwright install-deps chromium
113-145:⚠️ Potential issueRestore caching for Chrome browser binaries
Removing caching and OS dependencies setup will significantly impact workflow performance and stability. These steps should be restored with Chrome-specific configurations.
Apply this fix:
+# Get Playwright version +- name: Get installed Playwright version + id: playwright-version + working-directory: tests/pw + run: | + echo "PLAYWRIGHT_VERSION=$(npm ls @playwright/test --json | jq --raw-output '.dependencies["@playwright/test"].version')" >> $GITHUB_ENV +# Cache Chrome browser binaries +- name: Cache playwright binaries + id: playwright-cache + uses: actions/cache@v4 + with: + path: | + ~/.cache/ms-playwright + key: ${{ runner.os }}-playwright-chromium-${{ env.PLAYWRIGHT_VERSION }} + restore-keys: | + ${{ runner.os }}-playwright-chromium- +# Install Chrome binary & OS dependencies if cache missed +- name: Install Chrome binary & OS dependencies + id: pw-install + if: steps.playwright-cache.outputs.cache-hit != 'true' + working-directory: tests/pw + run: | + npx playwright install chromium + npx playwright install-deps chromium
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_api_tests.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e_api tests
🔇 Additional comments (1)
.github/workflows/e2e_api_tests.yml (1)
Line range hint
205-244: Restore test results reportingThe removal of test reporting features significantly impacts test visibility and debugging capabilities. These should be restored even for Chrome-only testing:
- Test summary generation
- PR comment updates
- API test results (if applicable)
Uncomment and restore the test reporting section while updating it for Chrome-specific results:
-# # Run api tests -# - name: 🧪 Run api tests -# id: api-test -# if: always() && steps.wp-env.outcome == 'success' && ( github.event_name != 'workflow_dispatch' || ( github.event_name == 'workflow_dispatch' && (github.event.inputs.testsuite == 'API' || github.event.inputs.testsuite == 'All'))) -# timeout-minutes: 5 -# working-directory: tests/pw -# run: | -# npm run test:api # Prepare test summary - name: Prepare test summary id: prepare-test-summary uses: actions/github-script@v7 if: always() && steps.clone-dokan-lite.outcome == 'success' with: result-encoding: string script: | const script = require("./tests/pw/utils/gitTestSummary.ts") return await script({github, context, core}) +# Find PR comment +- name: Find PR comment by github-actions[bot] + id: find-comment + if: github.event_name == 'pull_request' + uses: peter-evans/find-comment@v3 + with: + issue-number: ${{ github.event.pull_request.number }} + comment-author: 'github-actions[bot]' + body-includes: Tests Summary +# Post test summary as PR comment +- name: Create or update PR comment + if: github.event_name == 'pull_request' + uses: peter-evans/create-or-update-comment@v4 + with: + comment-id: ${{ steps.find-comment.outputs.comment-id }} + issue-number: ${{ github.event.pull_request.number }} + body: ${{ steps.prepare-test-summary.outputs.result }} + reactions: hooray + edit-mode: replace
.github/workflows/e2e_api_tests.yml
Outdated
| docker run --rm \ | ||
| --network host \ | ||
| -v ${{ github.workspace }}:/workspace \ | ||
| -w /workspace/tests/pw \ | ||
| -e ADMIN=${{ secrets.ADMIN }} \ | ||
| -e ADMIN_PASSWORD=${{ secrets.ADMIN_PASSWORD }} \ | ||
| -e VENDOR=${{ secrets.VENDOR }} \ | ||
| -e VENDOR2=${{ secrets.VENDOR2 }} \ | ||
| -e CUSTOMER=${{ secrets.CUSTOMER }} \ | ||
| -e CUSTOMER2=${{ secrets.CUSTOMER2 }} \ | ||
| -e USER_PASSWORD=${{ secrets.USER_PASSWORD }} \ | ||
| -e GMAP=${{ secrets.GMAP }} \ | ||
| -e MAPBOX=${{ secrets.MAPBOX }} \ | ||
| -e BASE_URL=http://localhost:9999 \ | ||
| -e CI=true \ | ||
| -e FORCE_COLOR=1 \ | ||
| -e DB_HOST_NAME=localhost \ | ||
| -e DB_USER_NAME=root \ | ||
| -e DB_USER_PASSWORD=password \ | ||
| -e DB_PORT=9998 \ | ||
| -e DATABASE=tests-wordpress \ | ||
| -e DB_PREFIX=wp \ | ||
| -e PR_NUMBER=${{ github.event.number }} \ | ||
| -e SHA=${{ github.event.pull_request.head.sha }} \ | ||
| -e SYSTEM_INFO=./tests/pw/playwright/systemInfo.json \ | ||
| -e API_TEST_RESULT=./tests/pw/playwright-report/api/summary-report/results.json \ | ||
| -e E2E_TEST_RESULT=./tests/pw/playwright-report/e2e/summary-report/results.json \ | ||
| -e API_COVERAGE=./tests/pw/playwright-report/api/coverage-report/coverage.json \ | ||
| -e E2E_COVERAGE=./tests/pw/playwright-report/e2e/coverage-report/coverage.json \ | ||
| mcr.microsoft.com/playwright:v1.49.1-noble \ | ||
| npm run test:e2e | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Docker configuration and security
While the Docker setup is functional, there are several improvements needed:
- Using
--network hostexposes all host ports to the container - Sensitive environment variables are passed directly
- The Docker image version should be pinned to match the project's Playwright version
Apply these improvements:
docker run --rm \
- --network host \
+ --network bridge \
+ -p 9999:9999 \
+ -p 9998:9998 \
-v ${{ github.workspace }}:/workspace \
-w /workspace/tests/pw \
- -e ADMIN=${{ secrets.ADMIN }} \
- -e ADMIN_PASSWORD=${{ secrets.ADMIN_PASSWORD }} \
+ --env-file .env.test \
mcr.microsoft.com/playwright:v1.49.1-noble \
npm run test:e2eCreate a .env.test file to securely manage test environment variables.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
.github/workflows/e2e_api_tests.yml (3)
102-107:⚠️ Potential issueFix browser installation configuration.
The browser installation step is malformed and needs to be Chrome-specific.
Apply this fix:
+- name: Install Chrome browser + working-directory: tests/pw + run: | + npx playwright install chromium + npx playwright install-deps chromium
109-113:⚠️ Potential issueRestore debugging and caching infrastructure.
Removing debug logs, version detection, and caching mechanisms will:
- Make test failures harder to debug
- Significantly slow down workflow execution
- Increase resource usage
These infrastructure components should be restored with Chrome-specific configurations rather than removed entirely.
Also applies to: 116-140
172-238:⚠️ Potential issueCritical: Restore test results reporting and artifacts.
Removing test coverage collection, reporting, and artifacts upload will:
- Eliminate visibility into test execution results
- Remove debugging capabilities
- Break the established test reporting pipeline
These features are critical for maintaining test quality and should be restored with Chrome-specific configurations rather than removed entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/e2e_api_tests.yml(2 hunks)tests/pw/tests/e2e/admin.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/pw/tests/e2e/admin.spec.ts
🔇 Additional comments (1)
.github/workflows/e2e_api_tests.yml (1)
72-74:⚠️ Potential issueUpdate Docker configuration for security and compatibility.
The Docker container configuration needs improvement:
- The image version should be pinned to match the project's Playwright version
- Running as user 1001 might cause permission issues
- Network isolation is not properly configured
Apply this diff:
container: - image: mcr.microsoft.com/playwright:v1.49.1-noble - options: --user 1001 + image: mcr.microsoft.com/playwright:v1.49.1-noble@sha256:<hash> + options: --user 1001 --network-alias tests --cap-drop ALLLikely invalid or redundant comment.
.github/workflows/e2e_api_tests.yml
Outdated
| # - name: Use desired version of NodeJS | ||
| # uses: actions/setup-node@v4 | ||
| # with: | ||
| # node-version: 20 | ||
| # cache: npm |
There was a problem hiding this comment.
Restore build steps with Chrome-specific configuration.
Removing Node.js setup and build steps will break the workflow. These steps are essential even for Chrome-only testing.
Restore these steps with Chrome-specific configuration:
+- name: Use desired version of NodeJS
+ uses: actions/setup-node@v4
+ with:
+ node-version: 20
+ cache: npm
+
+- name: Install and build (Chrome-specific)
+ run: |
+ npm ci --ignore-scripts
+ npm run build:chromeAlso applies to: 87-94
|
|
||
| # Run e2e tests | ||
| - name: 🧪 Run e2e tests | ||
| id: e2e-test | ||
| if: success() && (github.event_name != 'workflow_dispatch' || ( github.event_name == 'workflow_dispatch' && (github.event.inputs.testsuite == 'E2E' || github.event.inputs.testsuite == 'All'))) | ||
| timeout-minutes: 40 | ||
| working-directory: tests/pw | ||
| run: | | ||
| npm run test:e2e | ||
| npm run test:e2e |
There was a problem hiding this comment.
Improve E2E test execution configuration.
The E2E test execution step lacks proper configuration and error handling.
Enhance the test execution step:
- name: 🧪 Run e2e tests
id: e2e-test
if: success() && (github.event_name != 'workflow_dispatch' || ( github.event_name == 'workflow_dispatch' && (github.event.inputs.testsuite == 'E2E' || github.event.inputs.testsuite == 'All')))
timeout-minutes: 40
working-directory: tests/pw
run: |
- npm run test:e2e
+ npm run test:e2e:chrome || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run e2e tests | |
| - name: 🧪 Run e2e tests | |
| id: e2e-test | |
| if: success() && (github.event_name != 'workflow_dispatch' || ( github.event_name == 'workflow_dispatch' && (github.event.inputs.testsuite == 'E2E' || github.event.inputs.testsuite == 'All'))) | |
| timeout-minutes: 40 | |
| working-directory: tests/pw | |
| run: | | |
| npm run test:e2e | |
| npm run test:e2e | |
| # Run e2e tests | |
| - name: 🧪 Run e2e tests | |
| id: e2e-test | |
| if: success() && (github.event_name != 'workflow_dispatch' || ( github.event_name == 'workflow_dispatch' && (github.event.inputs.testsuite == 'E2E' || github.event.inputs.testsuite == 'All'))) | |
| timeout-minutes: 40 | |
| working-directory: tests/pw | |
| run: | | |
| npm run test:e2e:chrome || exit 1 |
| # # Start wordpress environment | ||
| # - name: Start WordPress Env | ||
| # id: wp-env | ||
| # uses: nick-fields/retry@v3 | ||
| # with: | ||
| # timeout_minutes: 4 | ||
| # max_attempts: 2 | ||
| # retry_on: error | ||
| # command: | | ||
| # cd tests/pw | ||
| # npm run start:env | ||
|
|
There was a problem hiding this comment.
WordPress environment setup is required.
The WordPress environment setup is critical for E2E testing, even with Chrome-only changes.
Restore the WordPress setup with proper retry mechanism:
+- name: Start WordPress Env
+ id: wp-env
+ uses: nick-fields/retry@v3
+ with:
+ timeout_minutes: 4
+ max_attempts: 2
+ retry_on: error
+ command: |
+ cd tests/pw
+ npm run start:env:chromeCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/e2e_api_tests.yml (2)
143-154:⚠️ Potential issueWordPress environment setup is critical.
The WordPress environment setup cannot be removed as it's essential for E2E testing, even with Chrome-only changes.
181-195:⚠️ Potential issueRestore test reporting for better visibility.
Removing test coverage collection, API tests, and artifacts upload significantly reduces visibility into test results and makes debugging harder.
Also applies to: 198-227
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_api_tests.yml(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/e2e_api_tests.yml
250-250: property "generate-html-report" is not defined in object type {clone-dokan-lite: {conclusion: string; outcome: string; outputs: {commit: string; ref: string}}; debug-log: {conclusion: string; outcome: string; outputs: {string => string}}; e2e-test: {conclusion: string; outcome: string; outputs: {string => string}}; playwright-cache: {conclusion: string; outcome: string; outputs: {cache-hit: string}}; playwright-version: {conclusion: string; outcome: string; outputs: {string => string}}; pw-install: {conclusion: string; outcome: string; outputs: {string => string}}}
(expression)
🔇 Additional comments (2)
.github/workflows/e2e_api_tests.yml (2)
87-94:⚠️ Potential issueBuild steps are required even for Chrome-only testing.
Removing the build steps will break the workflow as these steps are essential for setting up the testing environment.
- # - name: Composer install and build (Dokan-lite) - # run: | - # composer i --no-dev -o || composer update --no-dev -o - - # - name: Npm install and build (Dokan-lite) - # run: | - # npm ci || npm i - # npm run build + - name: Composer install and build (Dokan-lite) + run: | + composer i --no-dev -o || composer update --no-dev -o + + - name: Npm install and build (Dokan-lite) + run: | + npm ci || npm i + npm run build:chromeLikely invalid or redundant comment.
72-74: 🛠️ Refactor suggestionDocker configuration needs improvement.
While removing the container setup aligns with Chrome-only testing, the current configuration can be improved for better security and version consistency.
- # container: - # image: mcr.microsoft.com/playwright:v1.49.1-noble - # options: --user 1001 + container: + image: mcr.microsoft.com/playwright:v1.49.1-noble + options: --user 1001 --security-opt=no-new-privilegesLikely invalid or redundant comment.
| # # Run e2e tests | ||
| # - name: 🧪 Run e2e tests | ||
| # id: e2e-test | ||
| # if: success() && (github.event_name != 'workflow_dispatch' || ( github.event_name == 'workflow_dispatch' && (github.event.inputs.testsuite == 'E2E' || github.event.inputs.testsuite == 'All'))) | ||
| # timeout-minutes: 40 | ||
| # working-directory: tests/pw | ||
| # run: | | ||
| # docker run --rm \ | ||
| # --network host \ | ||
| # -v ${{ github.workspace }}:/workspace \ | ||
| # -w /workspace/tests/pw \ | ||
| # mcr.microsoft.com/playwright:v1.49.1-noble \ | ||
| # npm run test:e2e | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
E2E test execution needs proper configuration.
The current implementation lacks proper error handling and browser-specific configuration.
- name: 🧪 Run e2e tests
id: e2e-test
if: success() && (github.event_name != 'workflow_dispatch' || ( github.event_name == 'workflow_dispatch' && (github.event.inputs.testsuite == 'E2E' || github.event.inputs.testsuite == 'All')))
timeout-minutes: 40
working-directory: tests/pw
run: |
- npm run test:e2e
+ BROWSER=chromium npm run test:e2e || exit 1Also applies to: 170-178
.github/workflows/e2e_api_tests.yml
Outdated
| - name: Publish HTML report to GH Pages | ||
| if: always() && steps.generate-html-report.outcome == 'success' | ||
| uses: peaceiris/actions-gh-pages@v4 | ||
| with: | ||
| github_token: ${{ secrets.GITHUB_TOKEN }} | ||
| publish_branch: gh-pages | ||
| publish_dir: ./tests/pw/all-reports/html-report | ||
| destination_dir: ${{ github.event.pull_request.number || github.sha }} | ||
| keep_files: true | ||
| user_name: 'github-actions[bot]' # This will be the username that gets stamped in the repo commit | ||
| user_email: 'github-actions[bot]@users.noreply.github.com' # This will be the user email that gets stamped in the repo com | ||
|
|
There was a problem hiding this comment.
Fix HTML report publishing configuration.
The step references an undefined step ID 'generate-html-report' which will cause the workflow to fail.
- name: Publish HTML report to GH Pages
- if: always() && steps.generate-html-report.outcome == 'success'
+ if: always() && steps.e2e-test.outcome != 'skipped'
uses: peaceiris/actions-gh-pages@v4
with:
github_token: ${{ secrets.GITHUB_TOKEN }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Publish HTML report to GH Pages | |
| if: always() && steps.generate-html-report.outcome == 'success' | |
| uses: peaceiris/actions-gh-pages@v4 | |
| with: | |
| github_token: ${{ secrets.GITHUB_TOKEN }} | |
| publish_branch: gh-pages | |
| publish_dir: ./tests/pw/all-reports/html-report | |
| destination_dir: ${{ github.event.pull_request.number || github.sha }} | |
| keep_files: true | |
| user_name: 'github-actions[bot]' # This will be the username that gets stamped in the repo commit | |
| user_email: 'github-actions[bot]@users.noreply.github.com' # This will be the user email that gets stamped in the repo com | |
| - name: Publish HTML report to GH Pages | |
| if: always() && steps.e2e-test.outcome != 'skipped' | |
| uses: peaceiris/actions-gh-pages@v4 | |
| with: | |
| github_token: ${{ secrets.GITHUB_TOKEN }} | |
| publish_branch: gh-pages | |
| publish_dir: ./tests/pw/all-reports/html-report | |
| destination_dir: ${{ github.event.pull_request.number || github.sha }} | |
| keep_files: true | |
| user_name: 'github-actions[bot]' # This will be the username that gets stamped in the repo commit | |
| user_email: 'github-actions[bot]@users.noreply.github.com' # This will be the user email that gets stamped in the repo com |
🧰 Tools
🪛 actionlint (1.7.4)
250-250: property "generate-html-report" is not defined in object type {clone-dokan-lite: {conclusion: string; outcome: string; outputs: {commit: string; ref: string}}; debug-log: {conclusion: string; outcome: string; outputs: {string => string}}; e2e-test: {conclusion: string; outcome: string; outputs: {string => string}}; playwright-cache: {conclusion: string; outcome: string; outputs: {cache-hit: string}}; playwright-version: {conclusion: string; outcome: string; outputs: {string => string}}; pw-install: {conclusion: string; outcome: string; outputs: {string => string}}}
(expression)
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
e2e_testsproject.