[BACK] Support running on sample input and run in CI#46
Conversation
acornet
force-pushed
the
back/ac/test-mode
branch
2 times, most recently
from
5bea548 to
039d8f0
Compare
acornet
force-pushed
the
back/ac/test-mode
branch
from
1f34898 to
ad281e9
Compare
|
@cgoudet qu'en penses-tu? c'était deux fois plus rapide sur mon mac, 5 minutes j'ai peur que cela soit un peu long. |
acornet
force-pushed
the
back/ac/test-mode
branch
from
4a3a582 to
5bcbe14
Compare
acornet
force-pushed
the
back/ac/test-mode
branch
from
5bcbe14 to
edcbfc2
Compare
|
@cgoudet j'ai fait quelques optimisation supplémentaires et désormais la CI passe en 3 minutes, ce qui acceptable selon moi :) |
acornet
force-pushed
the
back/ac/test-mode
branch
from
605ae65 to
cd80e73
Compare
|
note: splitting the workflow in two, to be more granular on when things are ran |
acornet
force-pushed
the
back/ac/test-mode
branch
3 times, most recently
from
6022559 to
2ea5fcb
Compare
cgoudet
left a comment
cgoudet
left a comment
There was a problem hiding this comment.
Je suis assez sceptique sur la strategie. Au final on a un code avec énormément de ìf TestHelper.usingFullInput()`. Cela nuit à la lisibilité du code et le code est très dépendant du fait d'être en phase de test.
Je pense qu'il faut réorganizer le test voir le code pour que le code lui meme soit agnostique d'être en phase test, dégradé ou prod.
Voici une contre proposition :
- le test se déclenche via pytest d'un fichier du type
tests/back/test_end_to_end.py - On fourni à l'entrée de la pipeline un fichier de configuration dédié : les fichiers d'input comme les chemins de sauvegarde.
- On mock les appels API : ton travail pour remplacer des appels web par des lectures de fichier est nickel pour ca.
- On mock get_data_path par un fichier temporaire avec tempfile.
A dispo pour échanger!
| @@ -0,0 +1,145 @@ | |||
| Exercice;Outre-mer;Code Insee 2023 Région;Nom 2023 Région;Code Insee 2023 Département;Nom 2023 Département;Code Siren 2023 EPCI;Nom 2023 EPCI;Strate population 2023;Commune rurale;Commune de montagne;Commune touristique;Tranche revenu par habitant;Présence QPV;Code Insee 2023 Commune;Nom 2023 Commune;Catégorie;Code Siren Collectivité;Code Insee Collectivité;Libellé Budget;Agrégat;Montant BP;Montant BA;Montant flux BP-BA;Montant;Montant en millions;Population totale;Montant en € par habitant;Compte 2023 Disponible;ordre_analyse1_section1;ordre_analyse1_section2;ordre_analyse1_section3;ordre_analyse2_section1;ordre_analyse2_section2;ordre_analyse2_section3;ordre_analyse3_section1;ordre_analyse3_section2;ordre_analyse3_section3;ordre_analyse4_section1;annee_join;Population totale du dernier exercice;siren | |||
There was a problem hiding this comment.
Je conseillerais de mettre ce fichier dans tests/back/fixtures/. La bonne pratique est de ne pas mélanger le code et les tests. Et techniquement ce fichier est un input pour un test.
back/scripts/loaders/base_loader.py
Outdated
| # Get the content type of the file from the headers | ||
| response = requests.head(file_url) | ||
| content_type = response.headers.get("content-type") | ||
| # logger.info(f"Content type : {content_type}") |
There was a problem hiding this comment.
This is dead code and can be removed.
|
|
||
| def normalize_column_names(df): | ||
| """This modify the dataframe in place.""" | ||
| df.columns = [ |
There was a problem hiding this comment.
C'est une mauvaise pratique de modifier des datasets en place.
Il faudrait plutot return df.rename(columns=lambda x : re.sub(r"[.-]", "_", c.lower()).
Après je valide le fait d'avoir cette logique dans une fonction dédiée.
Peux tu ajouter les type hint tand que l'on y est?
acornet
force-pushed
the
back/ac/test-mode
branch
from
2ea5fcb to
d811a87
Compare
acornet
force-pushed
the
back/ac/test-mode
branch
from
d811a87 to
02c0771
Compare
acornet
force-pushed
the
back/ac/test-mode
branch
from
02c0771 to
78a26bb
Compare
acornet
left a comment
acornet
left a comment
There was a problem hiding this comment.
merci beaucoup pour les retours @cgoudet , je suis d'accord avec toi que la solution n'était pas satisfaite et difficile à maintenir. j'ai changé d'approche et j'ai capitalisé sur le travail de @RiwsPy dans #47 en créant un fichier de config de test, ce qui minimise les changements apporté dans le code. dis moi ce que tu en penses!
acornet
force-pushed
the
back/ac/test-mode
branch
from
78a26bb to
1eb036c
Compare
cgoudet
left a comment
cgoudet
left a comment
There was a problem hiding this comment.
Minimal style comments.
| Path.cwd() / "back" / "data" / "test" / "ofgl-base-communes-consolidee.test.csv", | ||
| sep=";", | ||
| index=False, | ||
| ) |
There was a problem hiding this comment.
Maybe make this module callable by wrapping it into a function and add the entreypoint at the end.
2 participants
front/data-analystsave_csvfunction, which was modifying the dataframe