Nicolas Ruiz EN
← Retour

Tester du code qui n'a pas été pensé pour

10 min de lecture tests · php · legacy

Dans mon article InvoiceService ne devrait pas exister, j'ai fini sur un bout de code de test.
Un test hyper simple à mettre en place, sans aucune dépendance. Normal : le code était « bien fait ». Des interfaces, des entrées et des sorties claires, de l'inversion de dépendance…

Mais ce ne serait pas honnête de dire que c'est tout le temps comme ça en entreprise.
Avouons-le : 90 % du temps, on vit avec des InvoiceService, pas avec des Invoice.

Et généralement, soit on n'a pas de tests, soit on a de mauvais tests.

Ce que j'entends par mauvais test : au moindre mouvement dans le code, il faut bouger, voire réécrire les tests.

Un enfant retire un bloc d'une tour de Jenga géante, qui s'effondre

Ce n'est pas du tout le but d'un test.

Unitaire, vraiment ?

D'ailleurs, revenons sur le test unitaire.

Il existe deux écoles1 :

Pour l'école de Detroit, « unitaire » ne veut pas dire tester une fonction, ni une classe, mais un comportement.

On veut vérifier que notre comportement fonctionne, peu importe, finalement, comment le code a été écrit.

Au fond, l'étiquette compte peu : unitaire ou intégration.
La vraie question, c'est : est-ce que mon test vérifie un comportement, ou la façon dont le code est écrit ?

C'est ce que je vais essayer de décrire dans cet article : comment écrire des tests qu'on ne réécrit pas tous les jours, et qui nous couvrent justement le jour où on voudra refactorer.
Car c'est ça, le but du test : pouvoir refactorer autant de fois qu'il le faut, avec un retour hyper rapide.
Test vert, ou test rouge.

InvoiceService (encore lui 👀)

Tout le code de cet article, tests compris, est dans le repo article-invoice-legacy-test.

Analysons ce que fait notre InvoiceService.

public function __construct(
    private readonly InvoiceRepository $invoices,
    private readonly CustomerRepository $customers,
    private readonly TaxApiClient $taxApi,
    private readonly EntityManagerInterface $em,
    private readonly LoggerInterface $logger,
) {}

public function createInvoice(array $data): Invoice
{
    $invoice = new Invoice();
    $invoice->setCustomerId($data['customer_id'] ?? null);

    $total = 0;
    foreach ($data['lines'] ?? [] as $row) {
        $line = new InvoiceLine();
        $line->setLabel($row['label']);
        $line->setAmount($row['amount']);
        $invoice->getLines()->add($line);
        $total += $row['amount'];
    }

    $customer = $invoice->getCustomerId() !== null
        ? $this->customers->find($invoice->getCustomerId())
        : null;

    $invoice->setTotal($total);
    $invoice->setVatAmount($this->taxApi->computeVat($total, $customer?->getCountry() ?? 'FR'));

    $this->em->persist($invoice);
    $this->em->flush();
    $this->logger->info('Facture créée', ['invoice' => $invoice->getId()]);

    return $invoice;
}

En regardant cette fonction, assez commune dans les projets, on voit 4 dépendances : le repository des clients, l'API de TVA, le logger et l'entity manager.
Et même une cinquième : le repository des factures, inutilisé ici, mais demandé par le constructeur.

London style

Si je devais faire un test de création de facture avec des mocks (London style), ça donnerait :

public function testCreatesADraftInvoiceWithItsLinesTotalAndVat(): void
{
    $customers = $this->createMock(CustomerRepository::class);
    $taxApi = $this->createMock(TaxApiClient::class);
    $em = $this->createMock(EntityManagerInterface::class);
    $logger = $this->createMock(LoggerInterface::class);

    $customers->expects($this->once())->method('find')->with(42)->willReturn($this->customer('FR'));
    $taxApi->expects($this->once())->method('computeVat')->with(150_000, 'FR')->willReturn(30_000);
    $em->expects($this->once())->method('persist')->with($this->isInstanceOf(Invoice::class));
    $em->expects($this->once())->method('flush');
    $logger->expects($this->once())->method('info')->with('Facture créée');

    $service = new InvoiceService(
        $this->createStub(InvoiceRepository::class),
        $customers,
        $taxApi,
        $em,
        $logger,
    );

    $invoice = $service->createInvoice([
        'customer_id' => 42,
        'lines' => [
            ['label' => 'Développement', 'amount' => 100_000],
            ['label' => 'Recette', 'amount' => 50_000],
        ],
    ]);

    self::assertSame('draft', $invoice->getStatus());
    self::assertSame(150_000, $invoice->getTotal());
    self::assertSame(30_000, $invoice->getVatAmount());
    self::assertCount(2, $invoice->getLines());
}

Ce qu'on peut affirmer avec ce test :

Très bien. Maintenant, faisons un petit refactoring : au lieu de charger tout le client pour lire son pays, on ne charge que son pays, avec une nouvelle méthode findCountry() dans le repository.

$country = $this->customers->findCountry($invoice->getCustomerId()) ?? 'FR'; // changement ici

$invoice->setTotal($total);
$invoice->setVatAmount($this->taxApi->computeVat($total, $country)); // changement ici

Le code fonctionne toujours. Le test, lui :

 ✘ Creates a draft invoice with its lines total and vat
   Expectation failed for method name is "find" when invoked 1 time.
   Method was expected to be called 1 time, actually called 0 times.

Rien de très grave, me direz-vous. Mais imaginez ça sur x appels, x mocks.
Et oui, on peut aussi résoudre ce problème avec l'IA, pour corriger le test vite.

Mais soyons honnêtes deux minutes.
Le test aurait-il dû exploser pour ce simple changement ?
Le comportement a-t-il changé ?
A-t-on cassé ce qu'on avait déjà construit ?

Non.

Detroit style

Maintenant, faisons ce même test en Detroit style.

final class CreateInvoiceTest extends KernelTestCase
{
    private EntityManagerInterface $em;
    private InvoiceService $service;

    protected function setUp(): void
    {
        self::bootKernel();
        $this->em = self::getContainer()->get(EntityManagerInterface::class);
        $this->service = self::getContainer()->get(InvoiceService::class);

        $schema = new SchemaTool($this->em);
        $metadata = $this->em->getMetadataFactory()->getAllMetadata();
        $schema->dropSchema($metadata);
        $schema->createSchema($metadata);
    }

    public function testCreatesADraftInvoiceWithItsLinesTotalAndVat(): void
    {
        $customer = $this->customer('FR');

        $invoice = $this->service->createInvoice([
            'customer_id' => $customer->getId(),
            'lines' => [
                ['label' => 'Développement', 'amount' => 100_000],
                ['label' => 'Recette', 'amount' => 50_000],
            ],
        ]);

        $this->em->clear();
        $saved = $this->em->find(Invoice::class, $invoice->getId());

        self::assertSame('draft', $saved->getStatus());
        self::assertSame(150_000, $saved->getTotal());
        self::assertSame(30_000, $saved->getVatAmount());
        self::assertCount(2, $saved->getLines());
    }

    private function customer(string $country): Customer
    {
        $customer = (new Customer())->setName('ACME')->setCountry($country);
        $this->em->persist($customer);
        $this->em->flush();

        return $customer;
    }
}

Ici, on ne vérifie pas si find, persist ou flush ont été appelés.
On donne un input à createInvoice, et on interroge la base pour récupérer notre facture.
Ensuite, on vérifie ses valeurs.

Oui, avec une vraie base, c'est techniquement un test d'intégration. C'est voulu.

Avant chaque test, setUp recrée le schéma de la base2.
Pour créer le client, j'utilise un simple helper customer(). Ici, nous pourrions utiliser une factory3.

Étant donné que l'API de TVA est une dépendance externe à notre application, on fake ce qu'elle peut nous envoyer.
TaxApiClient passe par le HttpClient de Symfony : on peut donc lui brancher FakeTaxApi via la config, sans toucher au code :

# config/packages/framework.yaml
when@test:
    framework:
        http_client:
            mock_response_factory: App\Tests\FakeTaxApi
# config/services_test.yaml
services:
    App\Tests\FakeTaxApi: ~
final class FakeTaxApi
{
    private const RATES = ['FR' => 20, 'DE' => 19];

    public function __invoke(string $method, string $url, array $options): JsonMockResponse
    {
        $body = json_decode($options['body'], true);

        return new JsonMockResponse(['vat' => intdiv($body['amount'] * self::RATES[$body['country']], 100)]);
    }
}

Et notre findCountry() ? Ce test reste vert : il ne sait pas comment on récupère le pays, il vérifie seulement la TVA enregistrée en base.

Remarquez qu'on n'a pas du tout touché à notre InvoiceService pour écrire ce test !
Mais maintenant, grâce à lui, on est capable de le refactorer sans en avoir peur.

Donc, quelle différence avec le London style ?
Ici, on ne teste que le comportement. Je pourrais même dire qu'on teste l'enveloppe qui crée la facture.

Demain, vous pouvez modifier complètement l'intérieur de la fonction createInvoice, comme ci-dessous :

public function createInvoice(array $data): Invoice
{
    $lines = array_map(
        fn (array $row) => (new InvoiceLine())->setLabel($row['label'])->setAmount($row['amount']),
        $data['lines'] ?? [],
    );
    $total = array_sum(array_map(fn (InvoiceLine $line) => $line->getAmount(), $lines));

    $country = $this->em->createQueryBuilder()
        ->select('c.country')
        ->from(Customer::class, 'c')
        ->where('c.id = :id')
        ->setParameter('id', $data['customer_id'] ?? 0)
        ->getQuery()
        ->getOneOrNullResult()['country'] ?? 'FR';

    $invoice = (new Invoice())
        ->setCustomerId($data['customer_id'] ?? null)
        ->setTotal($total)
        ->setVatAmount($this->taxApi->computeVat($total, $country));
    array_walk($lines, fn (InvoiceLine $line) => $invoice->getLines()->add($line));

    $this->em->wrapInTransaction(fn () => $this->em->persist($invoice));
    $this->logger->info('Facture créée', ['invoice' => $invoice->getId()]);

    return $invoice;
}

Je ne dis pas que ce code est meilleur que le précédent. Je dis juste qu'il peut être écrit différemment, sans rien casser.

$ php bin/phpunit --testdox

Time: 00:00.499, Memory: 48.50 MB

Create Invoice
 ✔ Creates a draft invoice with its lines total and vat
 ✔ Vat depends on the customer country
 ✔ Refuses an invoice without customer only once the database complains

Create Invoice Mocked
 ✘ Creates a draft invoice with its lines total and vat
   Expectation failed for method name is "find" when invoked 1 time.
   Method was expected to be called 1 time, actually called 0 times.

 ✘ Vat depends on the customer country
   Expectation failed for method name is "computeVat" when invoked 1 time
   Parameter 1 for invocation App\Client\TaxApiClient::computeVat(100000, 'FR'): int does not match expected value.
   -'DE'
   +'FR'

 ✘ Creates an invoice without customer with french vat
   Expectation failed for method name is "flush" when invoked 1 time.
   Method was expected to be called 1 time, actually called 0 times.

Même comportement :

Et finalement, n'est-ce pas ce qu'on veut ?

Ce qu'il faut retenir

Je n'oppose pas le Detroit style au London style. Les deux ont des buts différents.

Detroit est plutôt le choix par défaut, et il est précieux pour du refactoring et du legacy.
London, plutôt pour faire émerger la conception, et commencer à travailler sans connaître ses dépendances.

Pour le cas de notre article, Detroit a plus de sens : comme on l'a vu, il nous fallait tester le comportement, peu importe comment la fonction est construite.

Maintenant, il n'y a plus qu'à refactorer tranquillement. Et pourquoi pas vers des Invoice, comme dans mon article précédent.

Précisions

Étant donné qu'on a besoin de vrais objets pour faire nos tests Detroit, il peut arriver que ce soit compliqué de les initialiser.
Pour cela, on peut utiliser des factories sous Symfony, et j'imagine que chaque framework a son lot d'outils de test pour ça.

Toutefois, si cela devient vraiment très compliqué, cela veut dire que la fonction fait beaucoup, beaucoup, beaucoup trop de choses. Et que le refactoring ne fera que du bien.

Notes

  1. Martin Fowler les compare dans Mocks Aren't Stubs. L'école de Londres a son livre de référence : Growing Object-Oriented Software, Guided by Tests, de Steve Freeman et Nat Pryce. ↩

  2. Recréer le schéma devient lent quand les tests se multiplient. DAMA DoctrineTestBundle ouvre une transaction avant chaque test et l'annule à la fin : la base reste propre, sans rien recréer. ↩

  3. Sous Symfony, la référence est Foundry : CustomerFactory::createOne(['country' => 'FR']) crée le client en base, avec des valeurs par défaut pour tous les autres champs. ↩