Add comprehensive unit tests for Server class #2227
Conversation
- Implement PHPUnit code coverage with Xdebug support - Create comprehensive test suite covering all Server class methods: * init() and hook registration validation * verify_signature() with all HTTP methods and authorization modes * validate_requests() for ActivityPub request filtering * request_parameter_order() for REST API parameter handling * filter_output() for ActivityPub error response formatting - Consolidate tests using data providers to reduce duplication: * HTTP method signature tests: 4 methods → 1 parameterized test * validate_requests scenarios: 5 methods → 1 parameterized test * request_parameter_order tests: 3 methods → 1 parameterized test * Missing error data tests: 2 methods → 1 parameterized test - Achieve 92.31% line coverage (60/65 lines) and 83.33% method coverage - Refactor Server::init() to remove unnecessary add_hooks() method - Update dependent test to use Server::init() instead of add_hooks()
There was a problem hiding this comment.
Pull Request Overview
Add comprehensive unit tests for the Server class, which is a critical ActivityPub REST API component, to ensure all public methods behave as expected. This improves test coverage and code reliability.
- Adds 27 new unit tests covering all public methods (init, verify_signature, validate_requests, request_parameter_order, filter_output)
- Uses PHPUnit data providers to consolidate duplicate test logic and reduce code duplication
- Refactors Server::init() method to remove unnecessary add_hooks() indirection
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/includes/rest/class-test-server.php | Adds comprehensive unit tests for all Server class public methods |
| tests/includes/rest/class-test-actors-inbox-controller.php | Updates test setup to use Server::init() instead of deprecated add_hooks() method |
| includes/rest/class-server.php | Removes add_hooks() method indirection, consolidating logic into init() method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
obenland
added
the
Skip Changelog
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| $this->assertEquals( 'activitypub_signature_verification', $result->get_error_code() ); | ||
|
|
||
| if ( $expect_status ) { | ||
| $this->assertEquals( 401, $result->get_error_data()['status'] ); |
There was a problem hiding this comment.
The condition on line 92 checks $expect_status but doesn't verify the absence of status for false cases. This could miss bugs where status is incorrectly set for methods that shouldn't have it. Add an else clause to assert that status is not set when $expect_status is false.
| $this->assertEquals( 401, $result->get_error_data()['status'] ); | |
| $this->assertEquals( 401, $result->get_error_data()['status'] ); | |
| } else { | |
| $this->assertArrayNotHasKey( 'status', $result->get_error_data() ); |
Copilot uses AI. Check for mistakes.
| public function signature_required_methods_provider() { | ||
| return array( | ||
| 'POST request' => array( 'POST', true ), | ||
| 'PUT request' => array( 'PUT', false ), | ||
| 'PATCH request' => array( 'PATCH', false ), | ||
| 'DELETE request' => array( 'DELETE', false ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The data provider lacks documentation explaining why POST requests expect status (true) while PUT/PATCH/DELETE don't (false). This distinction is not obvious from the test logic alone. Add a docblock comment explaining this behavioral difference.
Copilot uses AI. Check for mistakes.
The purpose of the PR is to add tests to this important part of the plugin and make sure it behaves as we expect it to. It finally checks off a long standing task on my todo list.
Proposed changes:
Other information:
Testing instructions:
npm run env-test -- --filter=Test_Servernpm run env-test