Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supprime un test sans valeur ajoutée #1350

Merged
merged 2 commits into from
Jul 1, 2019
Merged

Supprime un test sans valeur ajoutée #1350

merged 2 commits into from
Jul 1, 2019

Conversation

Morendil
Copy link
Contributor

  • Amélioration technique.
  • Périodes concernées : toutes.
  • Zones impactées : tests.
  • Détails :
    • Supprime le test test_api.py dont le temps d'exécution est prohibitif (10s), la valeur ajoutée très faible (aucune information actionable n'est fournie en cas d'erreur) et qui dégrade l'expérience des contributeurs (signalé par @alexsegura)

Ces changements (effacez les lignes ne correspondant pas à votre cas) :

  • Modifient des éléments non fonctionnels de ce dépôt (par exemple modification du README).

Quelques conseils à prendre en compte :

Et surtout, n'hésitez pas à demander de l'aide ! :)

@Morendil Morendil requested review from alexsegura and sandcha June 24, 2019 08:52
Copy link
Contributor

@alexsegura alexsegura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je ne suis pas forcément pour supprimer ce test ! 🙂
C'est vrai que ce n'est pas un "vrai" test, mais plutôt un script qui devrait être lancé avec une série de tests de l'API, pour vérifier qu'elle a bien démarré.
Pour faire des tests d'API en Python, il semble y avoir Tavern

@Morendil
Copy link
Contributor Author

@alexsegura Merci pour le pointeur :)

Un "bon" test s'évalue, à mon sens, en regard des conditions qui le mettraient en échec. Par exemple on sait que ce test peut échouer si wget n'est pas installé, si le port 5000 est déjà alloué, si Flask n'est pas installé, etc. et toutes ces conditions ne représentent pas un bug dans le code de France, un défaut actionnable au niveau de ce dépôt.

A mon sens ce test se justifie sur France si on peut imaginer une modification au code de France qui fasse échouer ce test et seulement lui. Sinon, il convient soit de le déplacer sur Core, où l'outil que tu suggères pourrait avoir sa place; soit de le supprimer sans autre forme de procès.

Tu irais changer quel fichier dans France pour obtenir ce résultat?

@alexsegura
Copy link
Contributor

@Morendil pour moi éventuellement, on peut convertir ce test en script utilitaire.
Par exemple dans docker-selenium, il y a un script wait_all_done qui permet de vérifier que Selenium a démarré.

Copy link
Contributor

@sandcha sandcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'accord sur le fait que ce test est mal calibré.
Je note néanmoins un risque de manque de couverture des tests de la web api de Core (vérification du contrat de ce que l'api web sert). À poursuivre côté Core donc.

@Morendil Morendil force-pushed the remove-useless-test branch from 42ebba6 to 48c52dc Compare July 1, 2019 08:02
@Morendil Morendil merged commit 68778f9 into master Jul 1, 2019
@Morendil Morendil deleted the remove-useless-test branch July 1, 2019 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants