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

Utilise PyTest #1180

Merged
merged 1 commit into from
Oct 31, 2018
Merged

Utilise PyTest #1180

merged 1 commit into from
Oct 31, 2018

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Oct 19, 2018

Depends on openfisca/openfisca-core#746

  • Amélioration technique
  • Détails :
    • Permet d'utiliser pytest pour faire tourner les tests.
    • La librairie actuelle, nose, n'est plus en développement actif.

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

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

Quelques conseils à prendre en compte :

@bonjourmauko bonjourmauko force-pushed the use-pytest branch 3 times, most recently from 6bba209 to de2bebf Compare October 20, 2018 00:01
@Morendil
Copy link
Contributor

Morendil commented Oct 21, 2018

PyTest est cool. J'ai esquissé un petit script pour remplacer openfisca-run-tests. Ce n'est pas une réimplémentation iso, ça fait moins de choses, mais c'est suffisant pour la CI et ça nous permettrait de nous débarrasser de nose dans Core. PR dans la semaine peut-être.

Ce qui est vraiment bien c'est pytest.approx qui fait la même chose que assert_near mais en mieux, et qui supporte Numpy out of the box. Le seul KO pour l'instant est sur les énumérations.

@fpagnoux
Copy link
Member

fpagnoux commented Oct 23, 2018

PyTest est cool. J'ai esquissé un petit script pour remplacer openfisca-run-tests. Ce n'est pas une réimplémentation iso, ça fait moins de choses, mais c'est suffisant pour la CI et ça nous permettrait de nous débarrasser de nose dans Core. PR dans la semaine peut-être.

Attention, il y a un autre sujet actif sur les tests inscrit dans la planif. Ce n'est à 100% le même sujet (format vs moteur d'exécution) mais il y a des recoupements et de gros risques de conflits.

Sur le fond:

  • Si réécriture non-iso, il va falloir faire attention à ne pas supprimer des features qui sont activement utilisées. Elles sont souvent issues de demandes explicites. Mais il y a sans-doute du tri à faire (je ne suis pas sûr que --name_filter soit très utile)
  • Il est aussi important que ça reste un outil partagé et offert par Core, donc utilisable indépendamment de la localisation des tests ou autres spécificités d'un modèle en particulier.
  • Sauf preuve que faire l'inverse ne dégrade pas les performances, il faut a priori lire et exécuter les tests à la volée, et ne pas tout charger en mémoire upfront.

@Morendil
Copy link
Contributor

@fpagnoux Je voyais ça comme un remplaçant "drop-in" de openfisca-run-tests. Les questions de format sont entièrement orthogonales (traitées via scenarios.make_json_or_python_to_test).

Sur des essais informels, charger tous les YAML up-front ne pose aucun problème, et permet de décompter le nombre de tests et contrôler la granularité d'exécution mieux que ne semblait le faire le runner nose. Quel souci anticiperais-tu en termes de performance ?

@sandcha
Copy link
Contributor

sandcha commented Oct 23, 2018

Et mettre à jour le README.md qui référence encore nosetests?

@fpagnoux
Copy link
Member

Les questions de format sont entièrement orthogonales (traitées via scenarios.make_json_or_python_to_test).

Pas totalement, vu que:

  • on veut pas dépendre des scenarios (et de biryani) pour le parsing des tests. Il faudra donc appeler une autre fonction, avec une signature possiblement différente.
  • des changements sur la structure des outputs attendus peuvent changer un peu le code du test runner

@Morendil
Copy link
Contributor

Up ? Est-ce que tes réserves sont levées @fpagnoux ?

@pblayo
Copy link
Member

pblayo commented Oct 29, 2018

Il faut changer le numéro de version je pense :
Version 27.1.1 already exists in commit:
commit 8290f34

@bonjourmauko
Copy link
Member Author

Il faut changer le numéro de version

@pblayo Fait !

Et mettre à jour le README.md qui référence encore nosetests?

Fait !

Il est aussi important que ça reste un outil partagé et offert par Core, donc utilisable indépendamment de la localisation des tests ou autres spécificités d'un modèle en particulier.

Ces changements ne touchent pas openfisca-run-test, qui continue à utiliser nose.

`nose` et `nose2` sont en mode maintenance.
@Morendil Morendil merged commit b79e370 into master Oct 31, 2018
@Morendil Morendil deleted the use-pytest branch October 31, 2018 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants