Improve the test for the TraceableHttpClient::stream() method

getsentry · Jul 6, 2022 · c6d5f2b · c6d5f2b
1 parent e732d59
commit c6d5f2b
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 44 deletions.
diff --git a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php
@@ -30,7 +30,7 @@ abstract class AbstractTraceableHttpClient implements HttpClientInterface, Reset
     /**
      * @var HubInterface
      */
-    private $hub;
+    protected $hub;
 
     public function __construct(HttpClientInterface $client, HubInterface $hub)
     {
@@ -45,6 +45,7 @@ public function request(string $method, string $url, array $options = []): Respo
     {
         $span = null;
         $parent = $this->hub->getSpan();
+
         if (null !== $parent) {
             $headers = $options['headers'] ?? [];
             $headers['sentry-trace'] = $parent->toTraceparent();
@@ -71,8 +72,6 @@ public function stream($responses, float $timeout = null): ResponseStreamInterfa
     {
         if ($responses instanceof AbstractTraceableResponse) {
             $responses = [$responses];
-        } elseif (!is_iterable($responses)) {
-            throw new \TypeError(sprintf('"%s()" expects parameter 1 to be an iterable of TraceableResponse objects, "%s" given.', __METHOD__, get_debug_type($responses)));
         }
 
         return new ResponseStream(AbstractTraceableResponse::stream($this->client, $responses, $timeout));

diff --git a/src/Tracing/HttpClient/AbstractTraceableResponse.php b/src/Tracing/HttpClient/AbstractTraceableResponse.php
@@ -28,7 +28,7 @@ abstract class AbstractTraceableResponse implements ResponseInterface
     /**
      * @var Span|null
      */
-    private $span;
+    protected $span;
 
     public function __construct(HttpClientInterface $client, ResponseInterface $response, ?Span $span)
     {
@@ -54,7 +54,7 @@ public function __destruct()
                 $this->response->__destruct();
             }
         } finally {
-            $this->finish();
+            $this->finishSpan();
         }
     }
 
@@ -73,7 +73,7 @@ public function getContent(bool $throw = true): string
         try {
             return $this->response->getContent($throw);
         } finally {
-            $this->finish();
+            $this->finishSpan();
         }
     }
 
@@ -82,26 +82,28 @@ public function toArray(bool $throw = true): array
         try {
             return $this->response->toArray($throw);
         } finally {
-            $this->finish();
+            $this->finishSpan();
         }
     }
 
     public function cancel(): void
     {
         $this->response->cancel();
-        $this->finish();
+        $this->finishSpan();
     }
 
     /**
      * @internal
      *
+     * @param iterable<AbstractTraceableResponse> $responses
+     *
      * @return \Generator<AbstractTraceableResponse, ChunkInterface>
      */
     public static function stream(HttpClientInterface $client, iterable $responses, ?float $timeout): \Generator
     {
-        $wrappedResponses = [];
         /** @var \SplObjectStorage<ResponseInterface, AbstractTraceableResponse> $traceableMap */
         $traceableMap = new \SplObjectStorage();
+        $wrappedResponses = [];
 
         foreach ($responses as $response) {
             if (!$response instanceof self) {
@@ -112,16 +114,15 @@ public static function stream(HttpClientInterface $client, iterable $responses,
             $wrappedResponses[] = $response->response;
         }
 
-        foreach ($client->stream($wrappedResponses, $timeout) as $r => $chunk) {
-            if (null !== $traceableMap[$r]->span) {
-                $traceableMap[$r]->span->finish();
-            }
+        foreach ($client->stream($wrappedResponses, $timeout) as $response => $chunk) {
+            $traceableResponse = $traceableMap[$response];
+            $traceableResponse->finishSpan();
 
-            yield $traceableMap[$r] => $chunk;
+            yield $traceableResponse => $chunk;
         }
     }
 
-    private function finish(): void
+    private function finishSpan(): void
     {
         if (null !== $this->span) {
             $this->span->finish();

diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php
@@ -11,7 +11,6 @@
 use Psr\Log\NullLogger;
 use Sentry\SentryBundle\Tracing\HttpClient\AbstractTraceableResponse;
 use Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClient;
-use Sentry\SentryBundle\Tracing\HttpClient\TraceableResponse;
 use Sentry\State\HubInterface;
 use Sentry\Tracing\Transaction;
 use Sentry\Tracing\TransactionContext;
@@ -99,6 +98,48 @@ public function testRequest(): void
         ], $spans[1]->getTags());
     }
 
+    public function testStream(): void
+    {
+        $transaction = new Transaction(new TransactionContext());
+        $transaction->initSpanRecorder();
+
+        $this->hub->expects($this->once())
+            ->method('getSpan')
+            ->willReturn($transaction);
+
+        $decoratedHttpClient = new MockHttpClient(new MockResponse(['foo', 'bar']));
+        $httpClient = new TraceableHttpClient($decoratedHttpClient, $this->hub);
+        $response = $httpClient->request('GET', 'https://www.example.com/test-page');
+        $chunks = [];
+
+        foreach ($httpClient->stream($response) as $chunkResponse => $chunk) {
+            $this->assertSame($response, $chunkResponse);
+
+            $chunks[] = $chunk->getContent();
+        }
+
+        $spans = $transaction->getSpanRecorder()->getSpans();
+        $expectedTags = [
+            'http.method' => 'GET',
+            'http.url' => 'https://www.example.com/test-page',
+        ];
+
+        $this->assertSame('foobar', implode('', $chunks));
+        $this->assertCount(2, $spans);
+        $this->assertNotNull($spans[1]->getEndTimestamp());
+        $this->assertSame('http.client', $spans[1]->getOp());
+        $this->assertSame('HTTP GET', $spans[1]->getDescription());
+        $this->assertSame($expectedTags, $spans[1]->getTags());
+
+        $loopIndex = 0;
+
+        foreach ($httpClient->stream($response) as $chunk) {
+            ++$loopIndex;
+        }
+
+        $this->assertSame(1, $loopIndex);
+    }
+
     public function testSetLoggerShouldBeForwardedToDecoratedInstance(): void
     {
         $logger = new NullLogger();
@@ -157,34 +198,6 @@ public function testWithOptions(): void
         $this->assertSame('https://www.example.org/test-page', $response->getInfo('url'));
     }
 
-    public function testStream(): void
-    {
-        if (!method_exists(MockHttpClient::class, 'stream')) {
-            self::markTestSkipped();
-        }
-
-        $transaction = new Transaction(new TransactionContext());
-        $transaction->initSpanRecorder();
-
-        $mockResponse = MockResponse::fromRequest('GET', 'https://www.example.com', [], new MockResponse('test_body', ['http_code' => 201]));
-        $decoratedHttpClient = new MockHttpClient([$mockResponse], 'https://www.example.com');
-        $httpClient = new TraceableHttpClient($decoratedHttpClient, $this->hub);
-        $response = new TraceableResponse($decoratedHttpClient, $mockResponse, $transaction);
-
-        $stream = $httpClient->stream([$response]);
-        $chunks = iterator_to_array($stream, false);
-
-        $this->assertCount(3, $chunks);
-
-        $this->assertNotNull($transaction->getSpanRecorder());
-        $spans = $transaction->getSpanRecorder()->getSpans();
-        $this->assertCount(1, $spans);
-        $this->assertNotNull($spans[0]->getEndTimestamp());
-
-        $this->assertEquals('test_body', $response->getContent(false));
-        $this->assertEquals(201, $response->getStatusCode());
-    }
-
     private static function isHttpClientPackageInstalled(): bool
     {
         return interface_exists(HttpClientInterface::class);