fix: test: Smart/Origin unit tests and various fixes#2116
fix: test: Smart/Origin unit tests and various fixes#2116
Conversation
…of filters in database
…tant importing it in function
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Nayor
left a comment
Nayor
left a comment
There was a problem hiding this comment.
Super travail
Il manque pour moi juste à régler le fait que la variable d'environement soit nécesssaire pour lancer les tests (à régler au niveau du test, pas au niveau de la pipeline / du code)
|
|
||
| image_backend_url="http://images.demov6.camptocamp.org" | ||
| image_url="https://sos.exo.io/c2corg_demov6_active/" | ||
| # TODO Not working? |
There was a problem hiding this comment.
je l'ai laissé en me disant que si ça fonctionne à nouveau un jour il serait bon de changer, et pour se rappeler que le backend de démo est pour le moment en panne, mais je peux enlever si tu veux
| - run: curl -v http://localhost:9200 | ||
| - name: Run tests | ||
| run: export $(cat .env | grep -v "^#" | xargs) && pytest --cov-report term --cov-report xml --cov=c2corg_api | ||
| - name: Run tests (with secrets) |
There was a problem hiding this comment.
Les tests devraient tourner de la même manière, qui que ce soit qui fasse la PR
Il faudrait trouver une manière de lancer les tests navitia sans que la pipeline nécessite le secret de navitia
There was a problem hiding this comment.
ce n'est pas vraiment possible de tester l'API sans clef d'accès... Et cette différence est due à votre bot qui pour une raison que j'ignore ne peut pas avoir accès au secret. On peut toujours enlever les tests qui nécessitent la clef si vous le demandez.
|
|
||
| NAVITIA_GET_PATH = "c2corg_api.views.navitia.navitia_get" | ||
| NAVITIA_REDIS_CLIENT_PATH = "c2corg_api.views.navitia.redis_client" | ||
| NAVITIA_KEY = os.getenv("NAVITIA_API_KEY", "test-key") |
There was a problem hiding this comment.
ça me paraît être une mauvaise idée de reposer sur une variable d'environnement pour que le test fonctionne (d'autant que tout le monde n'y a pas accès)
--> Mocker ou utiliser un setter avant / après le test pour simuler une variable d'environnement correcte (ou incorrecte en fonction du test)
There was a problem hiding this comment.
on ne se base pas sur la valeur par défaut définie ci-dessus en pratique, mais sur la valeur dans le .env qui, contrairement au cas où on a accès au gh secret, n'est pas ignoré (cf la ligne export $(cat .env | grep -v "^#" | xargs)).
Je ne vois pas vraiment quelle meilleure solution on pourrait adopter ici ? (voir ma réponse aux commentaires précédents et suivants).
| # When GHCI runs without access to GH secrets | ||
| # (eg. for PR triggered by dependabot) | ||
| # return immediatly by matching test key from .env | ||
| api_key = os.getenv("NAVITIA_API_KEY") |
There was a problem hiding this comment.
Cela me paraît être une mauvaise pratique de changer le comportement du code en fonction de:
- De qui le lance
- S'il s'agit de tests ou pas
ça me paraît préférable de gérer ça au niveau des tests
There was a problem hiding this comment.
c'est exactement ce bout de code qui permet de gérer ça au niveau des tests : la fonction dans laquelle on se trouve est un trigger, et il n'est donc pas possible de l'exclure des tests directement puisqu'elle est appelée par alembic sur la création de waypoint.
C'est pour cela qu'on se base sur une "magic value" pour ne pas exécuter la fonction dans le cas où on est en mode test.
L'avantage, c'est que si on a par exemple une mauvaise clef d'API, la fonction va quand même s'exécuter et lever une erreur. On exclut une unique valeur.
Sans ce bout de code, on aurait du mal à gérer ça au niveau des tests puisque justement ce n'est pas un test. À moins d'enlever les tests qui créent des waypoints ou d'ajouter une autre "mock" value quelque part.
There was a problem hiding this comment.
je précise que l'inconvénient, c'est que les tests avec la vraie clef consomment quelques requêtes navitia, même si c'est normalement assez peu
| @@ -0,0 +1,1530 @@ | |||
| from unittest import mock | |||
There was a problem hiding this comment.
Il manque peut-être des tests pour vérifier si la clef d'API est invalide ou si Navitia ne répond pas ?
There was a problem hiding this comment.
bonne question ! mais par contre je ne sais pas si on peut réellement exécuter les tests de ce fichier (et là c'est différent du cas mentionné ci-dessus avec le trigger) sans la clef.
|
en tout cas merci @Nayor pour les retours ! Dites-nous ce qui vous convient le mieux : est-ce qu'on retire les tests navitia ? est-ce qu'on sépare en deux PR le temps de trouver une solution pour les tests tout en permettant de déployer rapidement le fix du timeout ? |
|
de notre côté, on va étudier la question de faire des tests sans exécuter de vraies requêtes, ça enlèverait la dépendance à la clef, mais les tests seraient aussi moins utiles puisqu'ils ne testeraient plus la chaîne complète avec les réponses de l'API |
|
la PR va être séparée en deux parties. |
2 participants
This PR groups several fixes and new tests:
views/navitia.py