LGTM, et elle avait raison

Pas d'incident cette fois, pas de dashboard rouge, personne à réveiller. Juste une question qui traîne depuis trois articles : comment est-ce que tout ça est passé en revue ?

"Deux approbations en vingt minutes, dont une avec un emoji."

C'est une phrase que j'ai écrite au sujet d'une revue qui a laissé passer onze mois de données fausses. Relue à froid, elle sonne comme un reproche : voilà des gens qui ont approuvé vite, distraitement, entre deux réunions.

Elle n'en est pas un. J'aurais approuvé aussi. Vous aussi. Et le plus gênant, c'est qu'on aurait tous eu raison.

Ce qu'une revue voit très bien

Rendons-lui d'abord ce qui lui appartient, parce que la liste est longue.

Une revue de code attrape le nom qui ment, la méthode qui fait deux choses, le catch (Exception) posé là pour faire taire quelque chose, la faute d'orthographe dans le message de log qu'on lira dans deux ans, le bloc recopié qui existait déjà vingt lignes plus haut, l'appel asynchrone auquel il manque un await.

Elle est même excellente à ça, et la raison est simple : toutes ces choses tiennent dans le diff. Elles sont écrites, elles sont là, et un oeil entraîné les voit passer.

C'est précisément pour ça qu'on lui fait confiance pour le reste. Un relecteur qui repère systématiquement les petites choses donne l'impression rassurante de repérer aussi les grandes. La compétence est réelle ; l'extrapolation, un peu moins.

Ce qu'elle ne peut pas voir

Trois exemples, un par article précédent. Tous approuvés, et ce qui les rend invisibles n'est jamais le même.

Le coût n'est pas dans le diff.

c.Documents.Count

Un accès de propriété. Le relecteur a raison de le lire ainsi, parce que c'en est un, syntaxiquement. Que ce soit aussi une requête SQL ne s'écrit nulle part : ça dépend d'une option activée dans un autre fichier, quinze sprints plus tôt, et du volume d'une table qui n'existe pas encore. Personne ne relit une ligne en imaginant la production de l'année prochaine.

La ligne est visible, la question ne l'est pas.

db.Contracts.Add(contract);
await db.SaveChangesAsync(cancellationToken);
// les prêts, puis :
await db.SaveChangesAsync(cancellationToken);

Celle-là, tout le monde la voit. Deux sauvegardes dans le même handler, c'est même plutôt lisible. Encore faut-il savoir que c'est une question à poser, et rien dans le fichier ne le signale. Le relecteur qui n'a pas encore rencontré ce piège lit une méthode bien découpée qui fait exactement ce qu'elle annonce, parce que c'est exactement ce qu'elle est.

Il n'y a rien à relire.

public async Task<IActionResult> Ranking(int year)

Le CancellationToken manquant n'est pas une ligne mal écrite, c'est une absence. On relit du code ajouté, du code supprimé, du code déplacé. On ne relit pas du code qui n'a pas été écrit : il n'apparaît dans aucun diff, d'aucune couleur.

Le point commun des trois, et c'est tout le sujet : le diff était correct. Ce qui manquait était ailleurs. Dans le temps, dans le volume, ou dans un fichier que personne n'avait de raison d'ouvrir ce jour-là.

Le vendredi à 17 h

Il y a une seconde catégorie d'angles morts, moins technique et nettement plus embarrassante.

Une revue de quarante lignes reçoit six commentaires. Une revue de quatre cents lignes en reçoit deux, dont un sur un nom de variable. Ce n'est pas de la paresse, c'est une capacité : passé un certain volume, on ne relit plus, on parcourt. Et personne n'écrit "je n'ai pas vraiment lu la seconde moitié", parce que ça ne se dit pas.

S'ajoute un biais très humain : on relit beaucoup mieux le code qu'on aurait écrit soi-même. Il se lit vite, il tombe juste, il rassure. Ce n'est pas la même chose que bien le relire, c'est même à peu près le contraire.

Reste la position sociale du relecteur. Demander "pourquoi deux SaveChanges ici ?" quand on ne sait pas encore que c'en est un, c'est accepter de passer pour celui qui n'a pas compris le ticket. Le pouce levé, lui, ne coûte rien, et il est presque toujours justifié. Presque.

Ce qu'on devrait cesser de lui demander

Rien de tout ça ne se corrige avec de la volonté. "Relisez mieux" n'est pas un plan, c'est un voeu.

Le partage, en revanche, est net. Ce qui est mécanique revient à la machine :

# .editorconfig
dotnet_diagnostic.CA2016.severity = error

options.ConfigureWarnings(w => w.Throw(CoreEventId.NavigationLazyLoading));

Un token non propagé, un lazy load, une signature qui sort de la convention d'équipe : ce sont des motifs, pas des jugements. Une machine les trouve toujours, à toute heure, y compris dans les quatre cents lignes que personne n'a lues.

Ce qui reste à l'humain est plus intéressant, et c'est la bonne nouvelle de l'article. Est-ce que ce nom dit la vérité ? Est-ce que cette classe a le droit de connaître celle-là ? Qu'est-ce qui se passe, dans le métier, entre ces deux écritures ? Aucun analyseur ne répondra jamais à la troisième, et c'est précisément celle qui aurait épargné onze mois de données fausses.

Une revue qui passe sa journée à chercher un token oublié ne fait pas mal son travail. Elle fait celui d'un autre.

Ce qu'un test fait entrer dans le diff

Les analyseurs trouvent des motifs. Du comportement, ils ne savent rien : une méthode parfaitement conventionnelle peut renvoyer un classement faux sans qu'aucune règle ne s'en émeuve. Pour ça, il y a les tests, et leur vertu la moins citée est la plus utile ici : un test est une ligne de diff. Ce que la revue ne pouvait pas voir, il l'écrit.

Le coût devient une assertion.

[Fact]
public async Task Listing_fifty_contracts_costs_one_query()
{
    await SeedContracts(50);
 
    await client.GetAsync("/contracts");
 
    Assert.Single(sqlCommands);
}

Le DbCommandInterceptor de l'article sur le lazy loading remplit sqlCommands, et cinquante contrats suffisent à faire exister le volume que personne n'imaginait en relisant. Le jour où quelqu'un réintroduit une requête par contrat, par un lazy load ou par une boucle, ce test casse, et il casse dans la pull request.

La question devient un nom.

A_contract_is_not_saved_when_its_loans_fail. Le corps compte moins que le nom : la seconde écriture échoue, un intercepteur qui lève suffit, et le test vérifie que la première n'a rien laissé derrière elle. Encore faut-il le lancer contre une vraie base. Le provider en mémoire d'EF Core ignore les transactions : il ne peut ni confirmer ni infirmer une atomicité. Une base jetable dans un conteneur, Testcontainers le fait très bien, coûte quelques secondes et dit la vérité.

L'absence devient une exigence.

On ne relit pas ce qui n'a pas été écrit, mais un test peut le réclamer. Le test de convention qui parcourt les points d'entrée et exige un CancellationToken est le seul des trois à regarder le code qui manque.

Photographier avant de toucher.

Reste le code dont plus personne ne sait exactement ce qu'il fait, c'est-à-dire celui qu'on modifie le plus volontiers. Avant de le changer, un test de caractérisation enregistre ce qu'il fait aujourd'hui, juste ou faux, sans le juger :

[Fact]
public Task Ranking_2025_is_unchanged() => Verify(ranking.Compute(2025));

La première exécution écrit un fichier, versionné à côté du test, et les suivantes comparent. Le jour où un refactoring "sans changement de comportement" en change un, la pull request contient soudain trois lignes de classement qui ont bougé. Le relecteur n'a plus à deviner l'effet d'un changement : il le lit.

Peu de bout en bout, mais aux bons endroits.

Les angles morts de la revue logent souvent entre deux fichiers : une option activée ici, consommée là. Un test qui traverse l'application entière, de la requête HTTP à la base, est le seul à ouvrir tous les fichiers à la fois. Il est lent et fragile, alors on en garde peu : les deux ou trois parcours qui, cassés, réveilleraient quelqu'un.

Un dernier angle mort, et il est de taille : un test ne vérifie que ce que son auteur a pensé à vérifier. Le développeur du portefeuille ne testait pas le contrat sans prêt, parce que ce cas n'existait pas. Ses tests partageaient exactement son point aveugle. C'est là que la revue reprend la main, avec une question qu'aucun outil ne pose à sa place : quel test aurait échoué si ce code était faux ? S'il n'y en a pas, c'est le premier commentaire à écrire.

Ce qu'il faut retenir