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 les tests non exécutés #730

Merged
merged 12 commits into from
Apr 19, 2017
Merged

Supprime les tests non exécutés #730

merged 12 commits into from
Apr 19, 2017

Conversation

fpagnoux
Copy link
Member

@fpagnoux fpagnoux commented Apr 14, 2017

Connected to #397
Connected to #722
Connected to #451
Depends on openfisca/openfisca-core#487


  • Amélioration technique
  • Détails :
    • Supprime les tests non exécutés par la CI (voir discussion précédente)
      • Ceux ignorés via le prefixe IGNORE_ ou la propriété ignore
      • Ceux qui n'étaient pas mentionnés dans test_yaml.py
    • Exécute dans la CI tous les tests YAML présents dans le répertoire tests
    • Déplace les tests hors du package principal
    • Déplace les configurations par dossier des tests YAML vers les fichiers individuels.
    • Simplifie en conséquence test_yaml.py

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

  • Modifient des éléments non fonctionnels de ce dépôt

Le diff est difficile à lire, pour vérifier qu'aucun test n'a été supprimé à tord, on peut plutôt comparer le nombre de tests exécutés avant et après la PR.

A priori, il devrait y avoir 6 tests de plus après cette PR qu'avant (tests anciennement ignorés alors qu'ils passaient).

@fpagnoux fpagnoux requested review from cbenz and benjello and removed request for cbenz April 14, 2017 14:23
@fpagnoux fpagnoux changed the title Tests Supprime les tests non exécutés Apr 14, 2017
@fpagnoux fpagnoux force-pushed the tests branch 2 times, most recently from 38d8e54 to ee94dc9 Compare April 14, 2017 15:20
@fpagnoux
Copy link
Member Author

Note aux contributeurs

Seuls les tests YAML exécutés par la CI ont été conservés. Cela correspond à ceux qui se situaient dans les répertoires fiches_de_paie, formulas, mes-aides.gouv.fr, ui.openfisca.fr, et scipy, et qui n'étaient pas ignorés.

Si parmi les tests supprimés se trouvent des tests qui passent et qui doivent donc rester dans la code base, vous pouvez soit les ré-introduire en ouvrant une PR sur la branchetests tant que cette PR n'est pas mergée, ou sur master après le merge.

Merci de votre compréhension !

@benjello
Copy link
Member

Je vais regarder mais il me semble qu'une grande partie des tests ont été ignorés car ils surchargeaint Travis.

@MattiSG
Copy link
Member

MattiSG commented Apr 19, 2017

Ok, on est à la veille de la fin d'itération, je propose qu'on avance. Les tests sont toujours versionnés, si @benjello en identifie certains qui doivent être intégrés, on revertera leur suppression.

@benjello
Copy link
Member

@MattiSG : a priori tous ces tests étaient à garder mais ils surchargeaient Travis, ils ont donc été ignorés (pour faire de la place aux tests mes-aides, pour accélérer l'exécution des tests) en attendant une solution définitive. Les virer tout court me semble très dommageable. J'ai accepté qu'ils soient ignorés pour être arrangeant et maintenant on les vire et c'est à moi de les réintégrer. J'ai un peu l'impression d'être couillonné ;-)

@fpagnoux
Copy link
Member Author

fpagnoux commented Apr 19, 2017

tous ces tests étaient à garder mais ils surchargeaient Travis, ils ont donc été ignorés (pour faire de la place aux tests mes-aides, pour accélérer l'exécution des tests) en attendant une solution définitive.Les virer tout court me semble très dommageable. J'ai accepté qu'ils soient ignorés pour être arrangeant et maintenant on les vire et c'est à moi de les réintégrer. J'ai un peu l'impression d'être couillonné ;-)

Je comprends ta frustration @benjello . Si les tests passaient et avaient de la valeur pour fiabiliser les formules, c'est vraiment dommage qu'on les ait ignorés.
Par contre, un test ignoré n'a pas beaucoup plus de valeur qu'un test supprimé. Il n'est jamais exécuté, tend à périmer, et n'apporte plus de "filet de sécurité" contre les régressions. Pire, il induit en erreur : je peux penser à tord que telle formule est garantie par un test, alors qu'elle est fausse.

Le mécanisme proposé ici te garantit que tous les tests présents dans le dépôt sont au vert. Certes, il y a aura un coût à ré-introduire les tests dans le dépôt, mais en échange tu as la garantie qu'ils passeront tout le temps, sans régression possible.

Pour mieux cerner le problème:

  • Exécutes-tu régulièrement ces tests ignorés ?
  • A-t-on une idée de la proportion de ces tests qui passent aujourd'hui si on les exécute ?

@fpagnoux
Copy link
Member Author

Dans tous les cas @cbenz @MattiSG parlons-en demain.

@fpagnoux fpagnoux merged commit 39ad621 into master Apr 19, 2017
@fpagnoux fpagnoux deleted the tests branch April 19, 2017 16:34
@MattiSG
Copy link
Member

MattiSG commented Apr 20, 2017

Comme vu au téléphone avec @benjello, on va :

  • Tenter de faire passer tous les tests supprimés et réintégrer ceux qui sont au vert.
  • Passer sur CircleCI si la performance est un frein.
  • Supprimer des tests par partionnement de l'espace des résultats si la performance est un frein.
  • Créer une branche contenant les tests ne passant pas pour faciliter le travail de correction associé.

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