add api test case #567
add api test case #567
Conversation
WalkthroughAdds PHPUnit testing support: dev dependencies in composer.json, a phpunit.xml config, a base REST API test case class, and initial API tests for the Activities endpoint. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (5)
tests/api/test-activities.php (1)
3-4: Consider adding class docblock.Adding a class-level docblock would improve documentation and clarify the purpose of this test suite.
+/** + * Test suite for Activities REST API endpoint. + */ class PM_Activities_API_Test extends PM_API_Test_Case {tests/api/class-pm-api-test-case.php (4)
3-8: Consider adding class docblock.A class-level docblock would improve documentation and help developers understand this base test case's purpose.
+/** + * Base test case for PM REST API tests. + * Provides REST server initialization and test users. + */ class PM_API_Test_Case extends WP_Test_REST_TestCase {
10-29: Add return type declarations for PHPUnit compatibility.The
setUpmethod should include a: voidreturn type declaration for better PHPUnit compatibility and to follow modern PHP testing practices.Apply this diff:
- public function setUp() { + public function setUp(): void { parent::setUp();Note: The same applies to
tearDownon line 31.
31-36: Consider using force delete for test cleanup.To ensure complete cleanup in tests, consider using the second parameter of
wp_delete_user()to force delete and reassign posts.public function tearDown() { parent::tearDown(); - wp_delete_user($this->admin_user); - wp_delete_user($this->editor_user); - wp_delete_user($this->subscriber_user); + wp_delete_user($this->admin_user, true); + wp_delete_user($this->editor_user, true); + wp_delete_user($this->subscriber_user, true); }
5-5: Unused property$namespaces.The
$namespacesproperty is declared but never used in this class or the test files. Consider removing it if not needed, or document its intended purpose if it's for future use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.json(1 hunks)phpunit.xml(1 hunks)tests/api/class-pm-api-test-case.php(1 hunks)tests/api/test-activities.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/api/test-activities.php (1)
tests/api/class-pm-api-test-case.php (1)
PM_API_Test_Case(3-37)
🔇 Additional comments (3)
phpunit.xml (2)
10-14: LGTM!The testsuite configuration correctly targets the API tests directory with appropriate file naming patterns.
3-3: Verify thattests/bootstrap.phpexists in the repository.The PHPUnit configuration references
tests/bootstrap.php, but this file was not included in the PR. Confirm this file exists in the repository before merging.tests/api/class-pm-api-test-case.php (1)
13-15: LGTM!REST server initialization follows WordPress testing best practices.
| "phpunit/phpunit": "^7.5", | ||
| "yoast/phpunit-polyfills": "^1.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for information about PHPUnit 7.5 and security advisories
# Check if composer.lock or other files provide context
if [ -f "composer.lock" ]; then
echo "=== composer.lock exists ==="
cat composer.lock | head -100
fi
# Check the full composer.json context
echo "=== Full composer.json ==="
cat composer.jsonRepository: weDevsOfficial/wp-project-manager
Length of output: 210
🌐 Web query:
PHPUnit 7.5 security vulnerabilities advisories
💡 Result:
Short answer — yes:
- A high‑severity remote code execution issue tied to PHPUnit’s eval-stdin.php (originally CVE‑2017‑9841 / related GHSA) was found to affect some PHPUnit lines including 7.5 series up through 7.5.19. Remediation was to remove/fix eval‑stdin.php; the file was removed in PHPUnit 7.5.20 (released 2020‑01‑08). [1][2][3]
Details / action:
- Affected: phpunit versions >=5.6.3 (and legacy 4.x/5.x) and phpunit >=5.6.3 up to 7.5.19 (per advisory mapping). [1][3]
- Fixed/removed: PHPUnit 7.5.20 (and 8.5.2) removed the vulnerable eval‑stdin.php. Upgrade to >=7.5.20 (or later 7.x/8.x/9.x maintained releases) or remove PHPUnit from web‑accessible/vendor directories. [2][3]
Sources:
- OSV / GitHub advisory (GHSA) — vulnerability mapping and affected/fixed versions. [turn1search8]
- PHPUnit history / removal note (removal of eval‑stdin.php; 7.5.20 release info). [turn1search6]
- Snyk / vulnerability database summarizing affected versions and fixes. [turn1search7]
🌐 Web query:
PHPUnit 7.5 PHP compatibility requirements
💡 Result:
PHPUnit 7.5 requires PHP ^7.1 — i.e. PHP >= 7.1 and < 8.0 (not compatible with PHP 8+). [1][2]
Sources:
- PHPUnit supported-versions table. [1]
- Composer/issue examples showing phpunit/phpunit 7.5 requires php ^7.1. [2]
🌐 Web query:
yoast/phpunit-polyfills 1.0 compatibility
💡 Result:
Short answer: The 1.x series of yoast/phpunit-polyfills was intended to support PHPUnit 4.8 through PHPUnit 9.x (forward‑compatibility polyfills for older PHPUnit releases). WordPress upgraded to require at least 1.0.1 (1.0.0 can trigger a version‑mismatch). See official package README/Packagist and WP Core notes. [1][2][3]
Sources:
- yoast/phpunit-polyfills (Packagist / README) — notes on PHPUnit support (1.x → PHPUnit 4.8–9.x). [1]
- Yoast/PHPUnit-Polyfills GitHub README (polyfills / compatibility description). [2]
- WordPress Core change / Trac referencing required minimum 1.0.1 and version‑check behavior. [3]
References:
[1] https://packagist.org/packages/yoast/phpunit-polyfills
[2] https://github.com/Yoast/PHPUnit-Polyfills (README)
[3] https://core.trac.wordpress.org/changeset/51598 and ticket #46149
Update PHPUnit constraint to exclude known vulnerable versions.
The constraint "^7.5" allows PHPUnit versions 7.5.0–7.5.19, which are affected by CVE-2017-9841, a remote code execution vulnerability in eval-stdin.php. This issue was fixed in PHPUnit 7.5.20 (released January 2020). Change to "^7.5.20" to ensure only patched versions are installed. Alternatively, consider upgrading to PHPUnit 8.5+ or 9.x+ if your PHP version support allows.
🤖 Prompt for AI Agents
In composer.json around lines 45-46, the PHPUnit version constraint " ^7.5"
permits vulnerable 7.5.x releases (pre-7.5.20); update the constraint to require
only patched releases by changing it to "^7.5.20" (or, if your PHP baseline
allows, prefer upgrading to a supported major such as PHPUnit 8.5+ or 9.x) so
Composer will install a non-vulnerable PHPUnit version.
| public function test_get_activities() { | ||
| wp_set_current_user($this->admin_user); | ||
|
|
||
| $request = new WP_REST_Request('GET', '/pm/v2/projects/1/activities'); | ||
| $response = $this->server->dispatch($request); | ||
|
|
||
| $this->assertEquals(200, $response->get_status()); | ||
| } |
There was a problem hiding this comment.
Test assumes project ID 1 exists without creating it.
The test hardcodes project ID 1 but doesn't create the project in setUp or the test method. This will cause the test to fail unless fixture data exists elsewhere.
Consider creating the project in setUp or in the test method:
public function test_get_activities() {
wp_set_current_user($this->admin_user);
+ // Create test project
+ $project_id = $this->factory->post->create([
+ 'post_type' => 'pm_project',
+ 'post_status' => 'publish'
+ ]);
+
- $request = new WP_REST_Request('GET', '/pm/v2/projects/1/activities');
+ $request = new WP_REST_Request('GET', "/pm/v2/projects/{$project_id}/activities");
$response = $this->server->dispatch($request);
$this->assertEquals(200, $response->get_status());
}🤖 Prompt for AI Agents
In tests/api/test-activities.php around lines 5 to 12, the test calls GET
/pm/v2/projects/1/activities but assumes project ID 1 exists; instead create a
project and use its actual ID: in setUp() (or at the start of this test) insert
a project (e.g. use the same factory/helper used elsewhere or
wp_insert_post/wp_create_post with required meta), capture the returned ID, and
replace the hardcoded 1 in the request path with that ID (build the endpoint
string dynamically). Ensure the created project has any required fields/author
and is accessible to the admin user set by wp_set_current_user before
dispatching the request.
| public function test_get_activities_unauthorized() { | ||
| wp_set_current_user(0); | ||
|
|
||
| $request = new WP_REST_Request('GET', '/pm/v2/projects/1/activities'); | ||
| $response = $this->server->dispatch($request); | ||
|
|
||
| $this->assertEquals(401, $response->get_status()); | ||
| } |
There was a problem hiding this comment.
Same hardcoded project ID issue.
This test has the same hardcoded project ID 1 issue as test_get_activities. Apply a similar fix to ensure the project exists before testing.
🤖 Prompt for AI Agents
In tests/api/test-activities.php around lines 14 to 21, the test uses a
hardcoded project ID (1) which can fail if that project doesn't exist; create a
project at the start of the test (using the test suite's project
factory/helper), capture its ID into a variable, then use that variable when
building the request path instead of the literal "1". Keep the unauthorized
setup (wp_set_current_user(0)) and ensure the project creation is minimal (no
extra permissions) so the rest of the test still asserts a 401 response.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.