From 753f65b4327c956416707d7fd9028e708a4ba52d Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 5 Jun 2025 02:36:22 +0000 Subject: [PATCH] Improve code coverage for HttpTransport This commit addresses code coverage gaps in `src/Transport/HttpTransport.php`. Key changes include: - **receive method**: I added tests to cover invalid item structures within batch JSON-RPC requests. I ensured that if an item in a batch is not a valid JSON object (e.g., a string or a list), a `TransportException` is thrown. - **send method**: I investigated an `else` branch previously thought to be reachable. I determined that PHP's runtime type hinting for method parameters (`JsonRpcMessage|array|null`) makes this branch effectively unreachable. I removed the unreachable `else` block. - **isStreamOpen method**: I added a test to confirm it correctly returns `false`, as the simplified transport does not support streaming. - **isClosed method**: I added tests to verify its state: - Returns `false` immediately after transport instantiation. - Returns `true` after the `send()` method has been called. - **getResponse method**: I added a test using reflection to cover a fallback scenario where the internal `$this->response` property might be `null`. This test ensures the method generates a proper error response in such a case. All new tests pass, and existing tests continue to pass. Linters (PHPCS, PHPStan) also pass with these changes. The overall goal was to increase test coverage for the specified methods and conditions. --- src/Transport/HttpTransport.php | 19 +----- tests/Transport/HttpTransportTest.php | 84 +++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 17 deletions(-) diff --git a/src/Transport/HttpTransport.php b/src/Transport/HttpTransport.php index 97e1387..9059599 100644 --- a/src/Transport/HttpTransport.php +++ b/src/Transport/HttpTransport.php @@ -174,24 +174,9 @@ public function send(JsonRpcMessage|array|null $messageOrPayload): void // it might result in a parsing error on the client if it's not valid JSON-RPC (e.g. just "null"). // For now, allow encoding null, which results in JSON "null". $payloadToEncode = null; - } else { - // Should not be reached due to type hints. Internal error if it does. - // Prepare a JSON-RPC error response. - $errorPayload = [ - 'jsonrpc' => '2.0', - 'error' => [ - 'code' => JsonRpcMessage::INTERNAL_ERROR, - 'message' => 'Server error: Invalid message type for sending.' - ], - 'id' => null - ]; - $encodedErrorString = json_encode($errorPayload); - $this->response = $this->responseFactory->createResponse(500) - ->withHeader('Content-Type', 'application/json') - ->withBody($this->streamFactory->createStream($encodedErrorString !== false ? $encodedErrorString : '{"jsonrpc":"2.0","error":{"code":-32000,"message":"Server JSON encoding error"}}')); - $this->responsePrepared = true; - return; } + // The 'else' block that handled non-JsonRpcMessage/array/null types has been removed + // as it was deemed unreachable due to PHP parameter type hints. $jsonPayloadString = json_encode($payloadToEncode); diff --git a/tests/Transport/HttpTransportTest.php b/tests/Transport/HttpTransportTest.php index ecd96b8..8bfb1ce 100644 --- a/tests/Transport/HttpTransportTest.php +++ b/tests/Transport/HttpTransportTest.php @@ -252,4 +252,88 @@ public function testSendHandlesJsonEncodeFailureSimplified(): void $this->assertEquals(JsonRpcMessage::INTERNAL_ERROR, $decodedBody['error']['code']); $this->assertStringContainsString('Failed to encode JSON response', $decodedBody['error']['message']); } + + /** + * @dataProvider invalidBatchItemProvider + * @param list $invalidBatchPayload + */ + public function testReceiveBatchWithInvalidItemStructure(array $invalidBatchPayload, string $caseName): void + { + $this->expectException(TransportException::class); + $this->expectExceptionCode(JsonRpcMessage::INVALID_REQUEST); + $this->expectExceptionMessage('Invalid JSON-RPC: Request must be a JSON object or an array of JSON objects.'); + + $jsonRpcString = json_encode($invalidBatchPayload); + + $this->mockRequest->method('getMethod')->willReturn('POST'); + $this->mockRequest->method('getHeaderLine')->with('Content-Type')->willReturn('application/json'); + + // Configure the mockStream instance that was created in setUp() + // getBody() on mockRequest already returns $this->mockStream from setUp. + $this->mockStream->method('__toString')->willReturn($jsonRpcString); + + $transport = $this->createTransport(); + $transport->receive(); + } + + /** + * @return array, string}> + */ + public static function invalidBatchItemProvider(): array + { + // Using the case name as part of the dataset helps in identifying the failing case from test output. + return [ + 'string_in_batch' => [[['method' => 'foo'], "not_an_object"], 'string_in_batch'], + 'array_in_batch' => [[['method' => 'foo'], [1, 2, 3]], 'array_in_batch'], + ]; + } + + public function testIsStreamOpenReturnsFalse(): void + { + $transport = $this->createTransport(); + $this->assertFalse($transport->isStreamOpen()); + } + + public function testIsClosedInitialState(): void + { + $transport = $this->createTransport(); + $this->assertFalse($transport->isClosed(), "isClosed() should be false before send() is called."); + } + + public function testIsClosedAfterSend(): void + { + $transport = $this->createTransport(); + $transport->send(null); // Call send() to change the state + $this->assertTrue($transport->isClosed(), "isClosed() should be true after send() is called."); + } + + public function testGetResponseWhenResponseIsNull(): void + { + $transport = $this->createTransport(); + + // Use Reflection to set the private $response property to null + $reflection = new \ReflectionClass(HttpTransport::class); + $responseProperty = $reflection->getProperty('response'); + $responseProperty->setAccessible(true); // Allow modification of private property + $responseProperty->setValue($transport, null); + + $response = $transport->getResponse(); + + $this->assertInstanceOf(ResponseInterface::class, $response); + $this->assertEquals(500, $response->getStatusCode()); + $this->assertStringContainsString('application/json', $response->getHeaderLine('Content-Type')); + + $expectedErrorPayload = [ + 'jsonrpc' => '2.0', + 'error' => [ + 'code' => JsonRpcMessage::INTERNAL_ERROR, // -32603 + 'message' => 'Response not prepared or available.' + ], + 'id' => null + ]; + $this->assertJsonStringEqualsJsonString( + json_encode($expectedErrorPayload, JSON_THROW_ON_ERROR), + (string) $response->getBody() + ); + } }