Conversation
|
In this PR we are adding a filter inside the active theme functions.php which I don't like for many reasons. |
Miraeld
left a comment
Miraeld
left a comment
There was a problem hiding this comment.
Overall Assessment
This PR successfully addresses issue #280 by disabling Rocket Insights before all E2E tests run. The implementation is functional but has some concerns that should be addressed.
Strengths ✅
- Clear Problem Solution: Directly addresses the issue of Rocket Insights interfering with E2E tests
- Idempotent Operations: Both
addFilterToThemeandremoveFilterFromThemecheck for existence before adding/removing - Good Error Handling: Proper error checking and informative console messages
- Proper Cleanup: Filter is removed in
AfterAllhook
Critical Issues 🚨
1. Theme Dependency Fragility
The solution hardcodes the twentytwenty theme. As noted in the PR description:
"If any test change theme, this filter won't be applied now"
This creates a maintenance burden and potential failure point. Consider:
- Using a more robust approach (e.g., a custom plugin or mu-plugin)
- At minimum, add a check to verify the active theme or document this limitation clearly
2. Unused Dependency
ssh2-sftp-client was added to package.json but is not used anywhere in the codebase. This should be removed unless there's a planned use.
3. .gitignore Change
The Codacy instructions file is being ignored, which suggests it shouldn't be tracked. If it's generated, this is fine, but if it contains important project rules, it should be committed.
Code Quality Issues 📝
In utils/commands.ts:
- Inconsistent Whitespace (lines 61-62 in hooks.ts):
await addFilterToTheme('rocket_rocket_insights_enabled', '__return_false', 'twentytwenty');
browser = await chromium.launch({ headless: false });Extra leading spaces before both lines - should be consistent with surrounding code.
-
Shell Command Complexity: The heredoc approach with temporary files works but is complex. Consider:
- Using a simpler approach with escaped quotes
- Or using a proper file manipulation library
-
Missing Input Validation: The functions don't validate that:
- The theme exists
- The functions.php file is writable
- The filter name/callback are valid
Suggestions for Improvement 💡
High Priority:
- Remove unused dependency: Delete
ssh2-sftp-clientfrom package.json - Fix whitespace issues in hooks.ts
- Consider alternative approach: As suggested in the timeline comments, using a UI checkbox in the e2e-helper plugin would be cleaner and more maintainable
Medium Priority:
- Add input validation to the filter functions
- Make theme name configurable via environment variable or config
- Add error handling for cases where the theme doesn't exist
Low Priority:
- Consider extracting the shell command logic into a separate utility
- Add unit tests for the new functions
Testing Verification ✓
The PR shows that smoke tests passed with RI disabled, which is good. However:
- Should verify behavior when theme is switched during tests
- Should test the cleanup in
AfterAllactually works
Recommendation
Conditional Approval - The core functionality works, but I recommend addressing at least:
- Remove the unused
ssh2-sftp-clientdependency - Fix the whitespace issues
- Add a clear comment/documentation about the theme dependency limitation
The team should also consider the alternative approach suggested by @wordpressfan using the e2e-helper plugin, which would be more robust long-term.
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to disable Rocket Insights (RI) by default before running e2e tests by adding filters to the WordPress theme's functions.php file. The implementation adds utility functions to manage theme filters and integrates them into the test lifecycle hooks.
- Adds three new utility functions to manipulate WordPress theme files via SSH
- Modifies test hooks to add/remove the RI disable filter before/after all tests
- Updates dependencies (ssh2, nan, cpu-features)
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| utils/commands.ts | Adds checkFunctionsPhpAccess, addFilterToTheme, and removeFilterFromTheme functions to manage WordPress theme filters via SSH commands |
| src/support/hooks.ts | Integrates filter management into BeforeAll/AfterAll hooks, adds isPluginInstalled check before activation, and imports new utility functions |
| package-lock.json | Updates dependency versions for ssh2 (1.14.0 → 1.17.0), nan (2.17.0 → 2.23.1), and cpu-features (0.0.8 → 0.0.10) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /** | ||
| * After all tests, closes the Chromium browser. | ||
| * After all tests, closes the Chromium browser and remove filter added |
There was a problem hiding this comment.
The comment says "wasnot" but should be "was not" (two separate words). This is a minor spelling error in the PR description context.
| * After all tests, closes the Chromium browser and remove filter added | |
| * After all tests, closes the Chromium browser and removes filter added |
| const filterLine = `add_filter( '${filter}', '${callback}' );`; | ||
|
|
||
| // Use a multi-step approach: create pattern file, check if exists, add if not found | ||
| const patternFile = `${wpDir}wp-content/themes/${themeName}/.filter_pattern.tmp`; | ||
|
|
||
| // First check if the filter already exists, only add if not found | ||
| const command = wrapSSHPrefix(`cat > ${patternFile} << 'PHPEOF' | ||
| ${filterLine} | ||
| PHPEOF | ||
| if grep -q -F -f ${patternFile} ${functionsPath}; then | ||
| echo "Filter already exists, skipping addition" | ||
| else | ||
| cat >> ${functionsPath} << 'PHPEOF' | ||
| ${filterLine} | ||
| PHPEOF | ||
| echo "Filter added" | ||
| fi | ||
| rm ${patternFile}`); |
There was a problem hiding this comment.
Potential security issue: The shell commands constructed using template literals with user-provided values (filter, callback, themeName) are vulnerable to command injection. Although these values may be controlled in the test context, it's best practice to properly escape or sanitize them before using in shell commands.
| const filterLine = `add_filter( '${filter}', '${callback}' );`; | ||
|
|
||
| // Use a multi-step approach: create pattern file, use grep, then cleanup | ||
| const patternFile = `${wpDir}wp-content/themes/${themeName}/.filter_pattern.tmp`; | ||
|
|
||
| // First check if the filter exists, then remove it if found | ||
| const command = wrapSSHPrefix(`cat > ${patternFile} << 'PATTERNEOF' | ||
| ${filterLine} | ||
| PATTERNEOF | ||
| if grep -q -F -f ${patternFile} ${functionsPath}; then | ||
| grep -v -F -f ${patternFile} ${functionsPath} > ${functionsPath}.tmp && mv ${functionsPath}.tmp ${functionsPath} | ||
| echo "Filter removed" | ||
| else | ||
| echo "Filter not found, skipping removal" | ||
| fi | ||
| rm ${patternFile}`); |
There was a problem hiding this comment.
Potential security issue: Similar to addFilterToTheme, this function is vulnerable to command injection through the filter, callback, and themeName parameters. The values should be properly escaped or sanitized before being used in shell commands.
|
Will open another PR for this #298 (comment), which may result in closing the current PR |
Description
This PR disable RI by default before running e2e tests
Fixes #280
Explain how this code impacts users.
Type of change
Detailed scenario
What was tested
Running smoke test, it passed and RI wasnot displayed in UI
How to test
Run all e2e => should pass
Technical description
Documentation
New dependencies
N/A
Risks
N/A
Mandatory Checklist
Code validation
Code style
Unticked items justification
N/A
Additional Checks