Migre à core v25 - Githubissues

sandcha commented 5 years ago

Correctif des tests associé au passage à Core v25

Migre à la syntaxe openfisca-core v25
Corrige la KeyError générée par compute_cotisation à l'exécution des tests
Renomme les chemins de paramètres
- Renomme cotisations_sociales/rsa/sal en cotisations_sociales/rsa/cotisations_salarie
- Renomme cotisations_sociales/rsaa/sal en cotisations_sociales/rsaa/cotisations_salarie
Corrige la syntaxe des valeurs de reforms dans les tests YAML

sandcha commented 5 years ago

@benjello Il ne resterait qu'un test au comportement étrange : sauf confusion de ma part, il y a une différence de résultat entre le test seul ou lancé avec les autres. 🙃

openfisca test -c openfisca_tunisia tests/

Dit :

.................................ERROR:openfisca_core.tools.test_runner:basic.yaml: De net a salaire_de_base pour Enseignant à net à payer de 1240 TND - 2016-04
F.
======================================================================
FAIL: unittest.case.FunctionTestCase (check)
----------------------------------------------------------------------
Traceback (most recent call last):
(...)
AssertionError: In test 'De net a salaire_de_base pour Enseignant à net à payer de 1240 TND', in file '/.../openfisca-tunisia/tests/reforms/de_net_a_salaire_de_base/basic.yaml', salaire_de_base@2016-04: [0.] differs from 1702.0 with an absolute margin [1702.] > 1

----------------------------------------------------------------------
Ran 35 tests in 0.661s

FAILED (failures=1)

Quand :

openfisca test -c openfisca_tunisia tests/reforms/de_net_a_salaire_de_base/basic.yaml

Répond :

.
----------------------------------------------------------------------
Ran 1 test in 0.331s

OK

À tout hasard, il y a aussi un build pre-bump qui est passé alors que cela ne passait pas en local. 🙄

benjello commented 5 years ago

Merci @sandcha je regarde ASAP.

benjello commented 5 years ago

@sandcha: je confirme que quand on fait tourner les tests un à un cela passe mais pas quand on lance tout en bloc (et j'ai bien utilisé un virtualenv et fait un make clean etc). Peut-être que @Morendil ou @fpagnoux auraient une idée sur ce qui plante ?

sandcha commented 5 years ago

@benjello Cause trouvée :

openfisca test -c openfisca_tunisia tests/reforms/de_net_a_salaire_de_base/basic.yaml

Ne charge pas la réforme alors qu'en groupe, elle semble prise en compte. Cela affecte la formule de calcul de salaire_imposable.

benjello commented 5 years ago

Super @sandcha . mais du coup c'est une erreur de core ?

Morendil commented 5 years ago

Pour moi ça passe dans les deux cas… @sandcha on peut regarder ça sur un environnement où tu reproduis le problème ?

sandcha commented 5 years ago

Étrange @Morendil ; merci d'avoir testé ! Oui, disponible pour te montrer ça dans un environnement virtuel neuf. J'ai le même résultat que CircleCI.

Morendil commented 5 years ago

On a pairé là dessus avec @sandcha, le TLDR du problème est que la clé reforms dans les tests YAML n'admet pas une valeur chaîne mais seulement une valeur liste; cette clé n'est par ailleurs pas documentée, et relevait donc du folklore et de la tradition orale ;)

Points de sortie:

en correctif court terme, cette PR passe après correction des clés reforms;
pérenne et moyen terme: documenter la clé reforms
pérenne et long terme: investir dans la possibilité de valider les tests YAML par un schéma

benjello commented 5 years ago

Merci @sandcha et @Morendil ! Pour abonder la tradition orale: l'utilité des listes réside dans la volonté d'enchaîner des réformes !

benjello commented 5 years ago

@sandcha : you can merge if you are done !

Morendil commented 5 years ago

Autres détails d'apprentissage que je n'ai pas eu le temps de noter plus haut:

on a constaté que le test runner n'exécutait pas les fichiers de test dans le même ordre sur des machines différentes (même avec la même image docker, mêmes branches et versions)
c'est une feature, pas un bug ! les tests ne devraient pas dépendre de leur ordonnancement
mais ça entraînait des différences de comportement entre l'environnement de @sandcha et le mien…
…car malheureusement nos tests ne sont pas complètement indépendants de l'ordre d'exécution, malgré des efforts dans ce sens dans le test runner;
en particulier on réutilise les objets TaxBenefitSystem d'un test à l'autre
ces objets sont réputés être "le même" s'ils ont la même baseline, même spécification de réforme, et même spécification d'extension, et sont donc mis en cache avec une clé dérivée de ces trois données
ce calcul de la clé pouvait être incorrect quand on passait une string au lieu d'une liste de strings dans le champ reforms, car ce qui est utilisé pour le hash c'est le frozenset des éléments de la valeur, soit pour une chaîne les caractères de la chaîne; or par (mal)chance les deux réformes avaient des noms composés des mêmes caractères

sandcha commented 5 years ago

Merci @Morendil pour le bilan ! J'ajouterais : l'ordre des tests exécutés par le test runner semble être l'ordre d'affichage des fichiers de la commande bash ls -U tests/ (plus lisible avec ls -Ul tests/).

sandcha commented 5 years ago

En lien avec openfisca/openfisca-core#825

@benjello Ci-dessus, tu disais "l'utilité des listes réside dans la volonté d'enchaîner des réformes". D'où la question : l'ordre d'enchaînement est-il bien important ?

benjello commented 5 years ago

Pas forcément mais oui en général

openfisca / openfisca-tunisia

Migre à core v25 #82