D8AGE-487 Remove the DI container and fix test coverage. #22
Conversation
intelektron
changed the title
| */ | ||
| class ApiClient implements ApiClientInterface | ||
| { | ||
| use ConfigurationAwareTrait; |
There was a problem hiding this comment.
I moved all the configuration/token stuff to the factory so I wouldn't have to pass it back and forth.
| $httpClient, | ||
| $requestFactory, | ||
| $streamFactory | ||
| ); | ||
| ), $configuration); | ||
| } | ||
|
|
||
| public function requestToken(): Token |
There was a problem hiding this comment.
The interface of this file is not changed.
| } | ||
|
|
||
| public function setToken(Token $token): self | ||
| public function setToken(Token $token): ApiClient |
There was a problem hiding this comment.
This may seem a little controversial as it breaks the clean architecture, but on the other hand - it keeps things really simple. The token is passed to the factory class.
src/Contract/RestInterface.php
Outdated
| /** | ||
| * @return array<string, string> | ||
| */ | ||
| public function getAuthorizationHeadersFromToken(Token $token): array; |
There was a problem hiding this comment.
It was a difficult decision, but I finally moved this code here. Previously it was part of the TokenAwareTrait.
| * | ||
| * Serves as the base for endpoint classes with token support. | ||
| */ | ||
| abstract class AuthorizedEndpointBase extends EndpointBase |
There was a problem hiding this comment.
After removing the container, I noticed that the TokenAwareTrait does not make the code any simpler. On the contrary, it requires a lot of extra lines. I changed it to a shorter solution.
| use OpenEuropa\Tests\CdtClient\Traits\ClientTestTrait; | ||
| use OpenEuropa\CdtClient\Model\Response\Translation; | ||
| use OpenEuropa\Tests\CdtClient\Traits\ApiTestTrait; | ||
| use OpenEuropa\Tests\CdtClient\Traits\RequestModelTestTrait; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| /** | ||
| * @coversDefaultClass \OpenEuropa\CdtClient\ApiClient | ||
| */ | ||
| class ApiClientTest extends TestCase |
There was a problem hiding this comment.
This was the Achilles heel of the previous version. I've changed it to cover all methods with simple tests. The asserts are not sophisticated, but sufficient to detect any runtime errors.
|
|
||
| protected function setUp(): void | ||
| /** | ||
| * @covers \OpenEuropa\CdtClient\ApiClient |
There was a problem hiding this comment.
This is needed to cover the constructor code too,
| new Response(200, [], (string) file_get_contents(__DIR__ . '/../fixtures/json/simple_token_call_response.json')), | ||
| ]; | ||
| $client = $this->getTestingApiClient([], $responses, false); | ||
| self::assertInstanceOf(Token::class, $client->requestToken()); |
There was a problem hiding this comment.
The assertions do not care whether the expected results are identical, this type of checking is covered by endpoint testing.
| * @covers \OpenEuropa\CdtClient\Http\Rest | ||
| */ | ||
| public function testIdentifier(string $correlationId, array $clientConfig, array $responses, mixed $expectedResult): void | ||
| { | ||
| $token = (new Token())->setAccessToken('JWT_TOKEN') |
There was a problem hiding this comment.
I took a microservice-centric approach here. Instead of creating the client and modifying its reflection to access the endpoints illegally, I've just called the constructor of the endpoint. This is shorter and clearer, and makes the test more isolated.
The missing lines are now replaced by ApiClientTest and ApiFactoryTest.
| * | ||
| * Provides helper methods for testing classes that utilize ApiClient and Rest. | ||
| */ | ||
| trait ApiTestTrait |
There was a problem hiding this comment.
Some of this code was part of the ClientTestTrait which has been removed. This trait helps us keep the endpoint tests clean.
src/Http/Download.php
Outdated
| { | ||
| } | ||
|
|
||
| public function downloadFile(string $uri): string | ||
| { | ||
| try { | ||
| $response = $this->rest->get($uri, $this->getAuthorizationHeaders()); | ||
| $response = $this->rest->get($uri, $this->rest->getAuthorizationHeadersFromToken($this->token)); |
There was a problem hiding this comment.
y, I think the trait made sense from this point of view
|
|
||
| public function createEndpoint(string $class): EndpointBase | ||
| { | ||
| if ($class === TokenEndpoint::class) { |
There was a problem hiding this comment.
I've added this one to avoid any confusion in the future. Otherwise, PHP will throw an error about invalid configuration, which is misleading.
| */ | ||
| public function getAuthorizationHeaders(?Token $token): array | ||
| { | ||
| if (null === $token) { |
There was a problem hiding this comment.
This if is required by PHPStan. I had a lot of trouble re-implementing this method. In the end, I had to allow nullable tokens in the Download class and create additional tests for this exception.
|
I fixed the remaining stuff and added a new test for the After my changes there are 4 uncovered paths left. Unfortunately there seems to be a problem with XDebug, explained here. We should test each |
No description provided.