Fix some phpcs issues in merged tests - #1701
Conversation
…b.com:equalizedigital/accessibility-checker into william/add-some-tests-for-rest-api-callbacks
There was a problem hiding this comment.
Pull request overview
Minor test cleanup to address PHPCS issues and align with PHPUnit 10+ requirements, plus stronger assertions on stats endpoints.
Changes:
- Made
setUp/tearDownpublic for PHPUnit 10+ compliance. - Added assertion for
scannable_posts_countkey in scans-stats response. - Tightened per-post-type stats assertions to require array with
post_typekey.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRestApiEndpointsTest makes ChangesREST API Test Updates
Possibly related PRs
Suggested reviewers
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/phpunit/includes/classes/RestApiEndpointsTest.php`:
- Around line 326-329: The test loop in RestApiEndpointsTest assumes each $stat
in $data['stats'] contains an embedded 'post_type' key, but the API returns
stats keyed by post type instead; update the assertions to reflect that shape by
asserting $data['stats'] is an array and iterating with keys: foreach
($data['stats'] as $post_type => $stat) { assertIsString($post_type);
assertIsArray($stat); } (replace the assertArrayHasKey('post_type', $stat)
check) so the test validates keyed-by-post_type payloads rather than expecting a
'post_type' field inside each item.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb8c7695-ff67-485a-9eb7-98d81fd34018
📒 Files selected for processing (1)
tests/phpunit/includes/classes/RestApiEndpointsTest.php
There was a problem hiding this comment.
Code Review
This pull request updates the visibility of test lifecycle methods and refactors assertions for scan statistics REST API endpoints. Feedback indicates that the loop logic for post-type statistics is incorrect based on the API's response structure and should be reverted to correctly handle the associative array. Additionally, a redundant conditional check in the general statistics test should be removed to ensure stricter validation of the returned payload, as the test environment expects data to be present.
| foreach ( $data['stats'] as $stat ) { | ||
| $this->assertIsArray( $stat ); | ||
| // Each stat should have post_type key. | ||
| $this->assertArrayHasKey( 'post_type', $stat ); | ||
| } |
There was a problem hiding this comment.
This loop logic is incorrect and will cause the test to fail. The scans-stats-by-post-types endpoint returns an associative array where the keys are post type slugs and the values are either false (for non-scannable types) or an array of statistics. The statistics array itself does not contain a post_type key. Additionally, use is_array for strict type checking to align with repository standards.
foreach ( $data['stats'] as $post_type => $stat ) {
$this->assertIsString( $post_type );
$this->assertTrue( $stat === false || is_array( $stat ) );
}References
- When the contract for a variable is a specific type, prefer a stricter type check (e.g., is_array) over a more general one (e.g., is_iterable) to enforce that contract.
| if ( ! empty( $data['stats'] ) ) { | ||
| $this->assertArrayHasKey( 'scannable_posts_count', $data['stats'] ); | ||
| } |
There was a problem hiding this comment.
The if ( ! empty( $data['stats'] ) ) check is redundant and weakens the test. Since the test setup creates a post, the stats array should not be empty. Removing the conditional ensures that the presence of the scannable_posts_count key is always verified, and the test will correctly fail if the API returns an empty response unexpectedly.
$this->assertArrayHasKey( 'scannable_posts_count', $data['stats'] );Updated the assertions for the stats structure to ensure each key is a post type slug and each value is either false or a summary array.
This pull request updates the
RestApiEndpointsTestclass to improve test coverage and ensure proper visibility for PHPUnit lifecycle methods. The most important changes include making thesetUpandtearDownmethods public, and enhancing assertions in stats-related test cases to verify the expected structure and keys in the API responses.Test lifecycle method visibility:
setUpandtearDownmethods fromprotectedtopublicin theRestApiEndpointsTestclass to comply with PHPUnit 10+ requirements. [1] [2]Test assertion improvements for API responses:
test_scans_stats_permissions_and_payload, added an assertion to check that thestatsarray includes thescannable_posts_countkey when stats are present.test_scans_stats_by_post_types_permissions_and_payload, updated the loop to assert that each stat is an array and contains apost_typekey, ensuring the structure matches expected output.Summary by CodeRabbit