Imposer la présence de tests pour accepter les PRs ?

Bonsoir,

Ça fait plusieurs PRs que je QA et je remarque que souvent, il n’y a pas de tests qui vérifient le bon fonctionnement de ce qu’apporte la PR. Il me semble qu’on accepte les PRs sans être trop regardant sur la présence ou l’absence de tests. Note: je parle ici de tests back-ends; les tests front-ends étant bien plus compliqués à mettre en place et sont peut-être un peu moins importants (ils ne reflètent pas le bon fonctionnement interne du site).

Est-ce qu’il ne faudrait pas qu’on impose la présence de tests back-end pour accepter une PR ?

Imposer la présence de tests nous permettrait d’avoir une meilleure confiance dans notre code et facilitera (légèrement) les QAs (si les tests de la CI passent, c’est déjà un bon point). L’inconvénient est que ça demande (un peu) plus de temps, et que ça peut rebuter les contributeurs débutants…

Qu’en pensez-vous ?

Salut,

Je pense que c’est une bonne idée d’encourager à avoir des tests.

Par contre, je suis assez réservé sur le fait de les rendre obligatoires, parce que ça limiterait fortement les évolutions du code plus ancien pour lequel il y a peu de tests ou qui est difficilement testable. Ce serait dommage de se priver d’une petite évolution sous prétexte qu’elle n’est pas testée, alors que la tester rendrait la contribution d’un tout autre calibre.

Hello, ce n’est pas déjà le cas, l’imposition des tests ? Quand j’ai commencé à développer ma PR, j’ai vu dans le process qu’il fallait faire ou mettre à jour les tests.

On peut effectivement inciter plus fortement à ajouter des tests lors des corrections de bugs ou ajouts de fonctionnalités (voire même rendre l’ajout de tests obligatoire pour l’ajout de fonctionnalité importante et complexe) ainsi que proposer notre aide (car c’est loin d’être trivial), mais à mon avis on se tirerait une balle dans le pied à le rendre obligatoire pour toutes les PRs (car on rendrait les petites contributions, qui sont souvent les premières, beaucoup plus compliquées).

Hello, ce n’est pas déjà le cas, l’imposition des tests ? Quand j’ai commencé à développer ma PR, j’ai vu dans le process qu’il fallait faire ou mettre à jour les tests.

Actuellement, on est obligé de mettre à jour les tests lorsqu’ils ne passent pas, mais on n’impose pas d’ajouter de nouveaux tests (en règle générale, peut-être que cela a été imposé dans certains cas par le passé, je ne sais pas).

Je suis d’accord.

Oui. J’avoue que c’est pour les premières contributions que ça m’embête le plus: dire "en l’état, on n’accepte pas ta PR, parce qu’il manque les tests" peut être un peu frustrant… Dans ce cas, que fait-on (en se plaçant dans le cas où un dev plus expérimenté aurait fait des tests pour la PR en question): on accepte la PR sans test, celui qui fait la QA (ou autre) rajoute les tests, on guide l’auteur de la PR pour ajouter les tests, un mélange de ces propositions ?

Ce que je propose:

• les tests back-ends sont obligatoires par défaut pour accepter une PR
• sauf si (j’aime bien comment le dit Aabu) rajouter les tests rendrait la contribution d’un tout autre calibre; ce qui est laissé à l’appréciation de ceux qui font la QA / l’équipe technique
• on précise dans le template des PRs que les tests back sont obligatoires
• on rajoute une section dans la documentation de ZdS qui précise cette politique et explique succinctement comment ajouter des tests: dans quel fichier, le fonctionnement de base des tests, comment lancer les tests en ligne de commande, etc
• le cas des premières contributions reste à discuter.

Je pense dans ce cas que le mieux est de proposer de soumettre la PR sans les tests pour que la personne puisse recevoir un peu d’aide. Et aussi de rappeler qu’il est toujours permis de demander de l’aide à n’importe quel moment.

Je suis d’accord avec ce que propose @philippemilink. Cela rendra nos attentes plus explicites et enouragera aussi les contributeurs plus réguliers à être exigeants avec eux-mêmes à ce niveau-là.

Perso je suis assez réservé sur le fait de rendre des tests back obligatoires.

On peut garder cette culture sans toutefois l’écrire dans le marbre. Parce que c’est typiquement une n-ième barrière à la contribution (même pour les plus avancés). Je me souviens de certaines PR ou les auteurs n’arrivaient pas à écrire les tests et on a du les rajouter parce que la feature était trop grosse pour être mergée sans tests.

Il y a aussi le fait que pas mal de tests (au hasard les notification) ont besoin d’une bonne refacto pour pouvoir rendre l’écrire de tests simple et "harmonieuse" dans le code. On manque de norme sur comment on sépare/écrit nos tests et tant que cela n’est pas encore fait, il me parait douteux d’imposer ça en l’état a toutes les contributions.