add job matrix #92
add job matrix #92
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces modifications to the E2E API testing workflow and Playwright configuration. A matrix strategy is implemented for parallel execution of tests, creating two groups: 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: 3
🧹 Outside diff range and nitpick comments (1)
.github/workflows/e2e_api_tests.yml (1)
167-177: Consider adding error handling for the test execution.The test execution should handle potential failures gracefully and provide clear error messages.
Consider adding error handling:
- name: 🧪 Run e2e tests ${{ matrix.group }} 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: | + set -e + echo "Running tests for group: ${{ matrix.group }}" npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts + exit_code=$? + if [ $exit_code -ne 0 ]; then + echo "::error::Tests failed for group ${{ matrix.group }} with exit code $exit_code" + exit $exit_code + fi🧰 Tools
🪛 actionlint (1.7.4)
167-167: property "group" is not defined in object type {}
(expression)
172-172: property "group" is not defined in object type {}
(expression)
📜 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)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/e2e_api_tests.yml
72-72: "strategy" section should not be empty. please remove this section if it's unnecessary
(syntax-check)
73-73: unexpected key "matrix" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
167-167: property "group" is not defined in object type {}
(expression)
172-172: property "group" is not defined in object type {}
(expression)
🔇 Additional comments (2)
tests/pw/tests/e2e/admin.spec.ts (1)
6-6: LGTM! Test cases are correctly tagged for matrix execution.
The test cases are logically split between matrix groups with login in @e2e_1 and logout in @e2e_2. Let's verify that these are the only test cases in this file that need matrix tags.
Also applies to: 11-11
✅ Verification successful
All test cases in admin.spec.ts are correctly tagged for matrix execution
The verification confirms that there are only two test cases in the file:
- 'admin can login' with
@e2e_1tag - 'admin can logout' with
@e2e_2tag
Both tests are properly tagged for matrix execution with no other test cases requiring attention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other test cases in the file that might need matrix tags
ast-grep --pattern 'test($_, $_, $_)' tests/pw/tests/e2e/admin.spec.ts
Length of output: 820
tests/pw/e2e.config.ts (1)
154-170: Verify test distribution across matrix groups.
The configuration looks correct, but let's ensure the tests are evenly distributed between the two groups for optimal parallel execution.
✅ Verification successful
Let me check the actual test content to better understand the distribution.
Let me check if there are any other test files that might contain these tags.
Test distribution is balanced but test coverage is minimal
The test distribution is perfectly balanced with exactly one test in each matrix group (@e2e_1 and @e2e_2), but there are only two e2e tests total. Both tests are in the same file (admin.spec.ts) and test basic admin authentication functionality:
- Group e2e_1: admin login test
- Group e2e_2: admin logout test
While the distribution is even, consider adding more e2e tests to make better use of the parallel execution setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Count tests in each matrix group
echo "Tests in e2e_1 group:"
rg "@e2e_1" -c
echo "Tests in e2e_2 group:"
rg "@e2e_2" -c
Length of output: 255
Script:
#!/bin/bash
# Get the content of the test file to see how the tests are organized
cat tests/pw/tests/e2e/admin.spec.ts
Length of output: 649
Script:
#!/bin/bash
# Find all test files and check their content for e2e tags
fd -e spec.ts -x cat {} \; | grep -A 1 "@e2e_"
Length of output: 327
tests/pw/e2e.config.ts
Outdated
|
|
||
| // e2e_tests | ||
| { | ||
| name: 'e2e_1', | ||
| testMatch: /.*\.spec\.ts/, | ||
| grep: [/@e2e_1/], | ||
| dependencies: NO_SETUP ? [] : ['e2e_setup'], | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Remove or update the e2e_tests project to avoid test duplication
The codebase analysis confirms that there is an existing e2e_tests project that matches all .spec.ts files without any grep patterns, while the new e2e_1 and e2e_2 projects match the same files but with specific grep patterns. This creates an overlap where tests tagged with @e2e_1 and @e2e_2 will be executed twice - once by their respective projects and once by the e2e_tests project.
- Remove the
e2e_testsproject if matrix execution with tags is the new intended way to run tests - Or add
grep: [/^(?!.*@e2e_[12]).*$/]to thee2e_testsproject to exclude tests tagged with@e2e_1and@e2e_2
🔗 Analysis chain
Consider removing or updating the existing e2e_tests project.
The new matrix projects (e2e_1 and e2e_2) might overlap with the existing e2e_tests project since they all match the same test pattern. Consider either:
- Removing the
e2e_testsproject if it's no longer needed - Adding a grep pattern to exclude
@e2e_1and@e2e_2tagged tests
Let me check if there's an existing e2e_tests project configuration to verify the potential overlap.
Let me fix the regex and try again to find the e2e_tests configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any test files that don't have matrix tags
rg -l "test\(" --type ts | xargs rg -L "@e2e_[12]"
Length of output: 299
Script:
#!/bin/bash
# Check for e2e_tests project configuration
rg -A 10 "e2e_tests.*{" tests/pw/e2e.config.ts
Length of output: 136
Script:
#!/bin/bash
# Check for e2e_tests project configuration
rg "name: ['\"](e2e_tests|e2e_1|e2e_2)['\"]" -B2 -A8 tests/pw/e2e.config.ts
Length of output: 942
.github/workflows/e2e_api_tests.yml
Outdated
| if [[ "${{ matrix.group }}" == e2e_1 ]]; then | ||
| npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts | ||
| elif [[ "${{ matrix.group }}" == a2e_2 ]]; then | ||
| npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts | ||
| fi |
There was a problem hiding this comment.
Simplify the test execution logic and fix the typo.
There are several issues in the test execution step:
- There's a typo in the elif condition:
a2e_2should bee2e_2 - The commands in both conditions are identical, making the if-elif structure unnecessary
Apply this fix:
- if [[ "${{ matrix.group }}" == e2e_1 ]]; then
- npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts
- elif [[ "${{ matrix.group }}" == a2e_2 ]]; then
- npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts
- fi
+ npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts📝 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.
| if [[ "${{ matrix.group }}" == e2e_1 ]]; then | |
| npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts | |
| elif [[ "${{ matrix.group }}" == a2e_2 ]]; then | |
| npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts | |
| fi | |
| npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts |
.github/workflows/e2e_api_tests.yml
Outdated
| strategy: | ||
| matrix: | ||
| test_group: [e2e_1, e2e_2] |
There was a problem hiding this comment.
Fix matrix strategy indentation.
The matrix strategy section has incorrect indentation. It should be indented under the strategy key.
Apply this fix:
strategy:
- matrix:
- test_group: [e2e_1, e2e_2]
+ matrix:
+ group: [e2e_1, e2e_2]📝 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.
| strategy: | |
| matrix: | |
| test_group: [e2e_1, e2e_2] | |
| strategy: | |
| matrix: | |
| group: [e2e_1, e2e_2] |
🧰 Tools
🪛 actionlint (1.7.4)
72-72: "strategy" section should not be empty. please remove this section if it's unnecessary
(syntax-check)
73-73: unexpected key "matrix" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
tests/pw/tests/e2e/_coverage.teardown.ts (1)
20-21: Ensure directory creation for group-specific pathsThe paths now include dynamic GROUP segments. While
mkdirSyncis used later withrecursive: true, consider extracting path creation to a helper function for better maintainability.+function ensureDirectoryExists(filePath: string) { + const dir = path.dirname(filePath); + if (!fs.existsSync(dir)) { + fs.mkdirSync(dir, { recursive: true }); + } +} + const outputFile = `playwright-report/e2e/${GROUP}/coverage-report/coverage.json`; const testReport = `playwright-report/e2e/${GROUP}/summary-report/results.json`; + +// Ensure directories exist +ensureDirectoryExists(outputFile); +ensureDirectoryExists(testReport);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/e2e_api_tests.yml(5 hunks)tests/pw/api.config.ts(1 hunks)tests/pw/e2e.config.ts(3 hunks)tests/pw/tests/e2e/_coverage.teardown.ts(2 hunks)tests/pw/types/environment.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/pw/e2e.config.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/e2e_api_tests.yml
64-64: context "matrix" is not allowed here. available contexts are "github", "inputs", "secrets", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
64-64: property "group" is not defined in object type {}
(expression)
66-66: context "matrix" is not allowed here. available contexts are "github", "inputs", "secrets", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
66-66: property "group" is not defined in object type {}
(expression)
🔇 Additional comments (5)
tests/pw/types/environment.d.ts (1)
66-66: LGTM! Verify GROUP environment variable usage
The addition of the GROUP environment variable is well-typed and consistent with the existing pattern.
Let's verify the usage of this environment variable across the codebase:
✅ Verification successful
GROUP environment variable is properly used in the test framework
The GROUP environment variable is correctly used in the test framework for organizing test reports and coverage data:
- Used in
tests/pw/e2e.config.tsfor configuring test reporters - Used in
tests/pw/tests/e2e/_coverage.teardown.tsfor organizing coverage and summary reports - Properly typed as string in
ProcessEnvinterface
The implementation is consistent and follows the established patterns for environment variable usage in the test framework.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GROUP environment variable usage
rg "process\.env\.GROUP|GROUP\s*=\s*process\.env" --type ts
Length of output: 61
Script:
#!/bin/bash
# Let's try a broader search for GROUP usage and environment variables
rg "GROUP" --type ts -B 2 -A 2
# Also search for environment variable declarations
ast-grep --pattern 'interface ProcessEnv {
$$$
}'
Length of output: 6219
tests/pw/api.config.ts (1)
47-47: Verify impact of commenting out GitHub reporter
The GitHub reporter is commented out in CI configuration. This might affect visibility of test results in GitHub UI.
Let's check if there are any related configurations or alternative reporters:
.github/workflows/e2e_api_tests.yml (3)
239-239: LGTM: Good artifact naming strategy
Including the matrix group in the artifact name prevents overwriting between parallel runs.
72-74:
Fix matrix strategy indentation
The matrix configuration has incorrect indentation which could cause parsing issues.
Apply this fix:
strategy:
- matrix:
- group: [e2e_1, e2e_2]
+ matrix:
+ group: [e2e_1, e2e_2]Likely invalid or redundant comment.
173-177: 🛠️ Refactor suggestion
Simplify test execution logic
The conditional logic is redundant as both branches execute the same command.
Apply this fix:
- if [[ ${{ matrix.group }} == e2e_1 ]]; then
- GROUP=${{ matrix.group }} npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts
- elif [[ ${{ matrix.group }} == e2e_2 ]]; then
- GROUP=${{ matrix.group }} npx playwright test --project=${{ matrix.group }} --config=e2e.config.ts
- fi
+ GROUP=${{ matrix.group }} npx playwright test --project=${{ matrix.group }} --config=e2e.config.tsLikely invalid or redundant comment.
| @@ -4,7 +4,7 @@ import fs from 'fs'; | |||
| import path from 'path'; | |||
| import { helpers } from '@utils/helpers'; | |||
|
|
|||
| const { DOKAN_PRO } = process.env; | |||
| const { DOKAN_PRO, GROUP } = process.env; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required GROUP environment variable
The GROUP variable is destructured but not validated. Consider adding a validation check to ensure it's defined.
-const { DOKAN_PRO, GROUP } = process.env;
+const { DOKAN_PRO, GROUP } = process.env;
+if (!GROUP) {
+ throw new Error('GROUP environment variable is required for coverage reporting');
+}📝 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.
| const { DOKAN_PRO, GROUP } = process.env; | |
| const { DOKAN_PRO, GROUP } = process.env; | |
| if (!GROUP) { | |
| throw new Error('GROUP environment variable is required for coverage reporting'); | |
| } |
.github/workflows/e2e_api_tests.yml
Outdated
| @@ -61,14 +61,17 @@ env: | |||
| SHA: ${{ github.event.pull_request.head.sha }} | |||
| SYSTEM_INFO: ./tests/pw/playwright/systemInfo.json | |||
| API_TEST_RESULT: ./tests/pw/playwright-report/api/summary-report/results.json | |||
| E2E_TEST_RESULT: ./tests/pw/playwright-report/e2e/summary-report/results.json | |||
| E2E_TEST_RESULT: ./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json | |||
There was a problem hiding this comment.
Fix invalid usage of matrix context in environment variables
The matrix context is only available within jobs, not at the workflow level. This will cause the workflow to fail.
Move these environment variables to the job level:
# At workflow level
env:
- E2E_TEST_RESULT: ./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json
- E2E_COVERAGE: ./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json
jobs:
tests:
# Add environment variables here
+ env:
+ E2E_TEST_RESULT: ./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json
+ E2E_COVERAGE: ./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.jsonAlso applies to: 66-66
🧰 Tools
🪛 actionlint (1.7.4)
64-64: context "matrix" is not allowed here. available contexts are "github", "inputs", "secrets", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
64-64: property "group" is not defined in object type {}
(expression)
.github/workflows/e2e_api_tests.yml
Outdated
| # # Set permalink structure | ||
| # - name: Set Permalink structure | ||
| # working-directory: tests/pw | ||
| # run: | | ||
| # npm run wp-env run tests-cli wp rewrite structure /%postname%/ | ||
|
|
||
| # Activate theme | ||
| - name: Activate theme:Storefront | ||
| working-directory: tests/pw | ||
| run: | | ||
| npm run wp-env run tests-cli wp theme activate storefront | ||
| # # Activate theme | ||
| # - name: Activate theme:Storefront | ||
| # working-directory: tests/pw | ||
| # run: | | ||
| # npm run wp-env run tests-cli wp theme activate storefront |
There was a problem hiding this comment.
💡 Codebase verification
Re-enable commented out steps - they are critical dependencies
The verification shows these steps are essential and actively used in the test suite:
- Permalink structure setup is required for product permalink tests in
productsDetails.spec.tsand site setup in_site.setup.ts - Storefront theme is a critical dependency used across multiple setup files (
_site.setup.ts,_localSite.setup.ts,_env.setup.ts) including theme-specific configurations - API tests are configured in
package.jsonand have dedicated test commands
Locations requiring attention:
.github/workflows/e2e_api_tests.yml: Re-enable lines 121-131 (permalink and theme setup).github/workflows/e2e_api_tests.yml: Re-enable lines 187-193 (API tests execution)
🔗 Analysis chain
Verify impact of disabled steps
Several critical steps have been commented out:
- Permalink structure setup
- Theme activation
- API tests execution
These changes could impact the test environment setup and overall test coverage.
Let's verify if these steps are still needed:
Also applies to: 187-193
Let me check the actual content of these files to understand the dependencies better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if permalink structure is referenced in tests
rg -l "permalink" tests/pw/tests/e2e/
# Check if theme-dependent selectors are used in tests
rg -l "storefront" tests/pw/tests/e2e/
# Check for API test dependencies
rg -l "test:api" tests/pw/package.json
Length of output: 320
Script:
#!/bin/bash
# Check permalink usage in test files
rg "permalink" tests/pw/tests/e2e/productsDetails.spec.ts tests/pw/tests/e2e/_site.setup.ts -C 2
# Check storefront theme dependencies
rg "storefront" tests/pw/tests/e2e/_site.setup.ts tests/pw/tests/e2e/_localSite.setup.ts tests/pw/tests/e2e/_env.setup.ts -C 2
# Check API test configuration
rg "test:api" tests/pw/package.json -C 2
Length of output: 3065
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 comments (4)
.github/workflows/e2e_api_tests.yml (4)
68-72: Fix matrix strategy indentation.
The matrix strategy section still has incorrect indentation. The matrix key should be indented under the strategy key.
Apply this fix:
strategy:
- matrix:
- group: [e2e_1, e2e_2]
+ matrix:
+ group: [e2e_1, e2e_2]Line range hint 241-246: LGTM! Artifact configuration properly handles matrix groups.
The artifact upload step correctly includes the matrix group in the artifact name, ensuring proper separation of artifacts from different test groups.
119-129:
Re-enable permalink and theme setup steps.
These commented-out steps are critical dependencies for the test suite:
- Permalink structure is required for product permalink tests
- Storefront theme is used across multiple setup files
Remove the comment markers to re-enable these essential setup steps.
165-179: 🛠️ Refactor suggestion
Optimize test execution configuration.
Several improvements are needed:
- The conditional blocks contain identical commands, making the if-elif structure unnecessary
- Environment variables should be moved to the job level
- The timeout reduction from 40 to 30 minutes needs justification
Apply these changes:
jobs:
tests:
name: e2e_api tests-${{ matrix.group }}
env:
+ E2E_TEST_RESULT: ./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json
+ E2E_COVERAGE: ./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json
steps:
- name: 🧪 Run e2e tests-${{ matrix.group }}
timeout-minutes: 40 # Restore original timeout unless reduction is justified
run: |
- if [[ "${{ matrix.group }}" == "e2e_1" ]]; then
- export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json"
- export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json"
- GROUP="${{ matrix.group }}" npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts
- elif [[ "${{ matrix.group }}" == "e2e_2" ]]; then
- export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json"
- export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json"
- GROUP="${{ matrix.group }}" npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts
- fi
+ GROUP="${{ matrix.group }}" npx playwright test --project="${{ matrix.group }}" --config=e2e.config.tsLikely invalid or redundant comment.
| # - 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 |
There was a problem hiding this comment.
Re-enable API test execution.
The API tests have been commented out without justification. These tests are configured in package.json and are an essential part of the test suite.
Remove the comment markers to restore API test coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/e2e_api_tests.yml (1)
68-72: Consider a more descriptive job name format.The current job name
e2e_api tests-${{ matrix.group }}could be more descriptive. Consider using a format that better reflects the test type and group.- name: e2e_api tests-${{ matrix.group }} + name: E2E API Tests (Group: ${{ matrix.group }})
📜 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
🪛 yamllint (1.35.1)
.github/workflows/e2e_api_tests.yml
[error] 174-174: trailing spaces
(trailing-spaces)
[error] 179-179: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/e2e_api_tests.yml (3)
Line range hint 243-247: LGTM! Artifact upload properly handles matrix groups.
The changes correctly include the matrix group in the artifact name and maintain appropriate paths and retention settings.
119-129:
Re-enable permalink and theme setup steps.
These commented-out steps are critical dependencies for the test suite:
- Permalink structure is required for product permalink tests
- Storefront theme is used across multiple setup files
Remove the comment markers to restore these essential setup steps:
- # # Set permalink structure
- # - name: Set Permalink structure
- # working-directory: tests/pw
- # run: |
- # npm run wp-env run tests-cli wp rewrite structure /%postname%/
+ # Set permalink structure
+ - name: Set Permalink structure
+ working-directory: tests/pw
+ run: |
+ npm run wp-env run tests-cli wp rewrite structure /%postname%/
- # # Activate theme
- # - name: Activate theme:Storefront
- # working-directory: tests/pw
- # run: |
- # npm run wp-env run tests-cli wp theme activate storefront
+ # Activate theme
+ - name: Activate theme:Storefront
+ working-directory: tests/pw
+ run: |
+ npm run wp-env run tests-cli wp theme activate storefront191-197:
Re-enable API test execution.
The API tests are an essential part of the test suite and should not be disabled.
Remove the comment markers to restore API test coverage:
- # - 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
+ - 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
.github/workflows/e2e_api_tests.yml
Outdated
| - name: 🧪 Run e2e tests-${{ matrix.group }} | ||
| 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 | ||
| timeout-minutes: 30 | ||
| working-directory: tests/pw | ||
| run: | | ||
| npm run test:e2e | ||
| if [[ "${{ matrix.group }}" == "e2e_1" ]]; then | ||
| export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json" | ||
| export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json" | ||
| export GROUP="${{ matrix.group }}" | ||
| npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts | ||
| elif [[ "${{ matrix.group }}" == "e2e_2" ]]; then | ||
| export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json" | ||
| export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json" | ||
| export GROUP="${{ matrix.group }}" | ||
| npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify test execution and fix formatting issues.
The current implementation has several issues:
- Redundant conditional logic with identical commands
- Environment variables could be set outside the conditions
- Trailing spaces in export statements
Apply this simplified implementation:
- name: 🧪 Run e2e tests-${{ matrix.group }}
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: 30
working-directory: tests/pw
run: |
- if [[ "${{ matrix.group }}" == "e2e_1" ]]; then
- export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json"
- export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json"
- export GROUP="${{ matrix.group }}"
- npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts
- elif [[ "${{ matrix.group }}" == "e2e_2" ]]; then
- export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json"
- export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json"
- export GROUP="${{ matrix.group }}"
- npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts
- fi
+ export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json"
+ export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json"
+ export GROUP="${{ matrix.group }}"
+ npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts📝 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: 🧪 Run e2e tests-${{ matrix.group }} | |
| 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 | |
| timeout-minutes: 30 | |
| working-directory: tests/pw | |
| run: | | |
| npm run test:e2e | |
| if [[ "${{ matrix.group }}" == "e2e_1" ]]; then | |
| export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json" | |
| export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json" | |
| export GROUP="${{ matrix.group }}" | |
| npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts | |
| elif [[ "${{ matrix.group }}" == "e2e_2" ]]; then | |
| export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json" | |
| export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json" | |
| export GROUP="${{ matrix.group }}" | |
| npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts | |
| fi | |
| - name: 🧪 Run e2e tests-${{ matrix.group }} | |
| 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: 30 | |
| working-directory: tests/pw | |
| run: | | |
| export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json" | |
| export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json" | |
| export GROUP="${{ matrix.group }}" | |
| npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 174-174: trailing spaces
(trailing-spaces)
[error] 179-179: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/e2e_api_tests.yml (1)
70-72: Consider adding fail-fast strategy for matrix jobs.Add
fail-fastconfiguration to stop all matrix jobs when one fails, saving CI resources:strategy: + fail-fast: true matrix: group: [e2e_1, e2e_2]
📜 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
🪛 yamllint (1.35.1)
.github/workflows/e2e_api_tests.yml
[error] 174-174: trailing spaces
(trailing-spaces)
[error] 182-182: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
.github/workflows/e2e_api_tests.yml (4)
119-129: Re-enable permalink and theme setup steps.
These commented-out steps are critical dependencies for the test suite:
- Permalink structure is required for product permalink tests
- Storefront theme is used across multiple setup files
194-200: Re-enable API test execution.
The API tests have been commented out without justification. These tests are configured in package.json and are an essential part of the test suite.
Line range hint 246-252: LGTM! Artifact configuration is well structured.
The artifact upload step correctly:
- Uses unique names for matrix jobs
- Includes all necessary test artifacts
- Has appropriate retention settings
171-184: 🛠️ Refactor suggestion
Simplify test execution logic and fix formatting.
The current implementation has redundant logic and formatting issues:
- Identical commands in both conditions
- Environment variables could be set once
- Trailing spaces in export statements
Apply this simplified implementation:
-if [[ "${{ matrix.group }}" == "e2e_1" ]]; then
- export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json"
- export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json"
- export GROUP="${{ matrix.group }}"
- npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts
-elif [[ "${{ matrix.group }}" == "e2e_2" ]]; then
- export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json"
- export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json"
- export GROUP="${{ matrix.group }}"
- npx playwright test --project="${{ matrix.group }}" --config=e2e.config.ts
-fi
+export E2E_TEST_RESULT="./tests/pw/playwright-report/e2e/${{ matrix.group }}/summary-report/results.json"
+export E2E_COVERAGE="./tests/pw/playwright-report/e2e/${{ matrix.group }}/coverage-report/coverage.json"
+export GROUP="${{ matrix.group }}"
+npx playwright test --project="${{ matrix.group }}" --config=e2e.config.tsLikely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 174-174: trailing spaces
(trailing-spaces)
[error] 182-182: trailing spaces
(trailing-spaces)
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
New Features
GROUPfor dynamic output folder naming in test reports.e2e_1ande2e_2, with specific filtering tags.Bug Fixes
Chores