Tests: Add PHP 8.4 and move PCOV to PHP 8.2#2878
Conversation
…o refactor/internal-api-stats # Conflicts: # tests/Integration/RestAPI/ContentHelper/ContentHelperFeatureTestTrait.php
…elper REST API Refactor: Base classes and Content Helper namespace implementation
…-and-move-pcov-to-php-8.2-retry
* Implement and update integration tests * Adjust some DocBlocks and whitespace * Adjust some DocBlocks and whitespace * Fix tests * Fix leaking tests * Apply code review suggestions * Remove backticks from function summary * Improve code formatting consistency between 2 tests --------- Co-authored-by: Alex Cicovic <23142906+acicovic@users.noreply.github.com>
# Conflicts: # build/content-helper/editor-sidebar.asset.php # build/content-helper/editor-sidebar.js
…-and-move-pcov-to-php-8.2-retry
📝 WalkthroughWalkthroughThe pull request updates the workflow configurations for integration and unit tests to support additional PHP versions, including 8.4, while removing 8.2. It modifies conditions for running tests based on PHP versions and experimental flags. Additionally, method signatures in several classes are updated to allow nullable parameters, enhancing flexibility and type safety without altering existing logic. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
|
After the integration test fixes on the base branch, we do have integration test failures that are specific to PHP 8.4. |
|
Opening this PR though we're getting PHP 8.4 failures on integration tests. To be discussed what we do with that. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.github/workflows/unit-tests.yml (1)
43-45: LGTM: PHP 8.4 properly configured as experimentalThe PHP 8.4 configuration is correctly set up with experimental flag, which will allow tests to fail without breaking the workflow. This is appropriate for an unreleased PHP version.
Consider adding a comment explaining why PHP 8.4 is marked as experimental (i.e., it's an unreleased version) to help future maintainers understand the configuration.
- php: '8.4' + # PHP 8.4 is marked as experimental as it's unreleased (expected end of November) coverage: none experimental: true.github/workflows/integration-tests.yml (1)
26-44: LGTM! Consider enhancing the matrix configuration documentation.The matrix configuration correctly implements the changes, with PHP 8.4 marked as experimental and PCOV moved to PHP 8.2. The warning comment about PCOV compatibility is valuable.
Consider adding a comment explaining why PHP 8.4 uses WordPress trunk:
- php: '8.4' + # PHP 8.4 requires WordPress trunk as stable versions may not be fully compatible yet wp: 'trunk' coverage: none experimental: true
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- .github/workflows/integration-tests.yml (2 hunks)
- .github/workflows/unit-tests.yml (1 hunks)
- src/class-parsely.php (1 hunks)
- src/rest-api/content-helper/trait-content-helper-feature.php (1 hunks)
- src/services/content-api/class-content-api-service.php (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/class-parsely.php (1)
Pattern
**/*.{html,php}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the HTML and PHP code to ensure it is well-structured and adheres to best practices.
- Ensure the code follows WordPress coding standards and is well-documented.
- Confirm the code is secure and free from vulnerabilities.
- Optimize the code for performance, removing any unnecessary elements.
- Validate comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Verify code compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/rest-api/content-helper/trait-content-helper-feature.php (1)
Pattern
**/*.{html,php}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the HTML and PHP code to ensure it is well-structured and adheres to best practices.
- Ensure the code follows WordPress coding standards and is well-documented.
- Confirm the code is secure and free from vulnerabilities.
- Optimize the code for performance, removing any unnecessary elements.
- Validate comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Verify code compatibility with the latest version of WordPress, avoiding deprecated functions or features."
src/services/content-api/class-content-api-service.php (1)
Pattern
**/*.{html,php}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the HTML and PHP code to ensure it is well-structured and adheres to best practices.
- Ensure the code follows WordPress coding standards and is well-documented.
- Confirm the code is secure and free from vulnerabilities.
- Optimize the code for performance, removing any unnecessary elements.
- Validate comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Verify code compatibility with the latest version of WordPress, avoiding deprecated functions or features."
🔇 Additional comments (7)
src/rest-api/content-helper/trait-content-helper-feature.php (1)
59-59: LGTM! Excellent improvement to type safety.The change to use
?WP_REST_Requestexplicitly declares nullability in the method signature, which is a good practice in PHP 8.x. This maintains backward compatibility while making the code more type-safe..github/workflows/unit-tests.yml (2)
28-28: LGTM: Matrix configuration is well structuredThe simplified PHPUnit version comment and PHP version matrix provide clear configuration while maintaining all necessary test combinations.
Also applies to: 33-33
38-42: LGTM: PCOV configuration properly moved to PHP 8.2The PCOV configuration is correctly set up for PHP 8.2 with appropriate directory settings and exclusions. The warning about PCOV compatibility with PHP 8.4+ is a helpful addition.
Let's verify the PCOV configuration in other workflow files:
✅ Verification successful
PCOV configuration is consistent across workflow files
The PCOV configuration is identical in both
unit-tests.ymlandintegration-tests.yml, with proper settings for PHP 8.2 and the same warning comment about PHP 8.4+ compatibility. Both files correctly:
- Use PCOV with PHP 8.2
- Have matching directory and exclusion settings
- Include the compatibility warning
- Avoid using PCOV with PHP 8.4
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent PCOV configuration across workflow files rg -l "pcov" .github/workflows/Length of output: 104
Script:
#!/bin/bash # Check PCOV configuration in both workflow files echo "=== unit-tests.yml ===" rg -A 3 -B 1 "pcov" .github/workflows/unit-tests.yml echo -e "\n=== integration-tests.yml ===" rg -A 3 -B 1 "pcov" .github/workflows/integration-tests.ymlLength of output: 933
.github/workflows/integration-tests.yml (1)
31-44: Verify WordPress version compatibility matrix.The current configuration uses 'latest' for most PHP versions and 'trunk' for PHP 8.4. Let's verify this setup against WordPress's PHP compatibility requirements.
✅ Verification successful
WordPress and PHP version compatibility matrix is correctly configured
The configuration aligns perfectly with the requirements:
- Minimum PHP requirement is 7.2 as specified in both README.md and composer.json
- All PHP versions in the matrix (7.2 through 8.4) meet this requirement
- Using WordPress 'latest' for stable PHP versions and 'trunk' for experimental PHP 8.4 is appropriate since there are no specific WordPress version constraints beyond 5.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check WordPress PHP version requirements in readme files # Look for PHP version requirements in WordPress trunk rg -i "php.*version|requires.*php" --type md # Check if there are any specific version constraints in composer files rg -i "\"php\":\s*\"[^\"]+\"" --type jsonLength of output: 1167
src/services/content-api/class-content-api-service.php (2)
90-91: LGTM! Improved type safety with explicit nullable parameters.The change to use
?stringtype hints enhances code clarity and type safety while maintaining backward compatibility.
119-120: LGTM! Consistent type safety improvement.The nullable type hints are correctly applied, maintaining consistency with the
get_post_detailsmethod.src/class-parsely.php (1)
642-642: LGTM: Explicit nullable type declaration aligns with PHP 8.4.The change to use
?intinstead of justintfor the nullable parameter makes the type declaration more explicit and follows PHP 8.4's best practices for type declarations.
…-to-php-8.2-retry
…move-pcov-to-php-8.2-retry' into update/add-php-8.4-to-tests-and-move-pcov-to-php-8.2-retry
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
phpunit-experimental.xml.dist (1)
19-19: LGTM! Consider adding documentation.
The constant is correctly placed in the <php> section. Consider adding an XML comment to document its purpose for future maintainers.
+ <!-- Indicates that integration tests are being executed, used by test workflows -->
<const name="INTEGRATION_TESTS_RUNNING" value="true"/>📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- phpunit-experimental.xml.dist (1 hunks)
🔇 Additional comments (1)
phpunit-experimental.xml.dist (1)
19-19: Verify constant presence in regular PHPUnit config.
Since this constant controls integration test execution, verify if it should also be present in the regular phpunit.xml.dist file.
✅ Verification successful
Constant correctly placed in integration test configurations only
The INTEGRATION_TESTS_RUNNING constant is appropriately present in both integration test configurations (phpunit-integration.xml.dist and phpunit-experimental.xml.dist) and correctly absent from the regular unit test configuration (phpunit.xml.dist). This is the expected behavior since:
- The regular
phpunit.xml.distis for unit tests only (bootstrap points to./tests/Unit/bootstrap.php) - Both integration configurations point to the integration bootstrap (
./tests/Integration/bootstrap.php) and need this constant - The constant's purpose is specifically to control integration test execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the constant exists in the regular PHPUnit config
# Expected: The constant should either exist in both files or have a clear reason for being only in experimental
# Test: Search for phpunit.xml.dist files and check for the constant
fd -e xml.dist -e xml --exec grep -l "INTEGRATION_TESTS_RUNNING" {}
Length of output: 132
Script:
#!/bin/bash
# Let's check the content of both files to understand the context better
echo "=== Content of phpunit.xml.dist ==="
cat phpunit.xml.dist
echo -e "\n=== Content of phpunit-experimental.xml.dist ==="
cat phpunit-experimental.xml.dist
echo -e "\n=== Content of phpunit-integration.xml.dist ==="
cat phpunit-integration.xml.dist
Length of output: 3309
acicovic
deleted the
update/add-php-8.4-to-tests-and-move-pcov-to-php-8.2-retry
branch
2 participants
Description
This PR adds PHP 8.4 to our unit and integration test matrices in experimental mode, as PHP 8.4 is on its merry way to release towards the end of November.
It also moves PCOV from using PHP 8.1 to 8.2, which is currently the most popular PHP version in the 8.x branch according to WordPress Stats.
Motivation and context
How has this been tested?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor