diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 98fd566..b144dc8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,34 +13,23 @@ jobs: uses: actions/setup-python@v5 with: python-version: "3.12" - # No caching — reinstalls all dependencies on every run - name: Install dependencies run: pip install -e ".[test]" - # Uses pip instead of uv; no lock file pinning - name: Start PostgreSQL run: | sudo systemctl start postgresql sudo -u postgres psql -c "CREATE USER bookstore WITH PASSWORD 'bookstore';" sudo -u postgres psql -c "CREATE DATABASE bookstore OWNER bookstore;" - # No wait/health check — tests may start before DB is ready - name: Run migrations run: alembic upgrade head - # Will fail silently if DB isn't up yet - name: Start app run: uvicorn app.main:app --host 0.0.0.0 --port 8000 & - # No health check — tests start immediately, race condition - name: Run tests run: pytest tests/ -v env: - # Payment API key hardcoded in the workflow — will fail without it PAYMENT_API_KEY: ${{ secrets.PAYMENT_API_KEY }} - # No DATABASE_URL override — uses the hardcoded default from config.py - - # No coverage reporting - # No test result artifacts - # No notification on failure diff --git a/app/api/orders.py b/app/api/orders.py index 5545ba9..ed0151c 100644 --- a/app/api/orders.py +++ b/app/api/orders.py @@ -11,7 +11,7 @@ @router.post("/", response_model=OrderResponse, status_code=201) def create_order(payload: OrderCreate, card_token: str, db: Session = Depends(get_db)): - service = OrderService() # new instance per request, no way to inject a fake + service = OrderService() try: order = service.create_order( user_id=payload.user_id, diff --git a/app/config.py b/app/config.py index ac3ff3e..b2aaa99 100644 --- a/app/config.py +++ b/app/config.py @@ -1,17 +1,15 @@ import os -# Config scattered across the module - some from env, some hardcoded DATABASE_URL = os.environ.get( "DATABASE_URL", "postgresql://bookstore:bookstore@localhost:5432/bookstore" ) -PAYMENT_API_URL = "https://api.fakepay.io/v1" # hardcoded, ignores env +PAYMENT_API_URL = "https://api.fakepay.io/v1" PAYMENT_API_KEY = os.environ.get("PAYMENT_API_KEY", "sk_test_changeme") EMAIL_SMTP_HOST = os.environ.get("EMAIL_SMTP_HOST", "localhost") EMAIL_SMTP_PORT = int(os.environ.get("EMAIL_SMTP_PORT", "1025")) -EMAIL_FROM = "shop@bookstore.local" # hardcoded +EMAIL_FROM = "shop@bookstore.local" CANCELLATION_WINDOW_HOURS = 1 MAX_ITEMS_PER_ORDER = 10 - diff --git a/app/models/order.py b/app/models/order.py index 7619774..a6026b0 100644 --- a/app/models/order.py +++ b/app/models/order.py @@ -1,5 +1,5 @@ import enum -from datetime import datetime +from datetime import datetime, UTC from sqlalchemy import Column, Integer, String, Numeric, DateTime, ForeignKey, Enum from sqlalchemy.orm import relationship from app.database import Base @@ -21,7 +21,9 @@ class Order(Base): total = Column(Numeric(10, 2), nullable=False) promo_code = Column(String(50), nullable=True) charge_id = Column(String(100), nullable=True) - created_at = Column(DateTime, nullable=False, default=datetime.utcnow) + created_at = Column( + DateTime(timezone=True), nullable=False, default=datetime.now(tz=UTC) + ) user = relationship("User") items = relationship( diff --git a/app/services/order_service.py b/app/services/order_service.py index cf1020e..50987e8 100644 --- a/app/services/order_service.py +++ b/app/services/order_service.py @@ -19,7 +19,6 @@ class OrderService: - # Instantiates its own dependencies - hard to override in tests def __init__(self): self._payment = PaymentClient() self._email = EmailClient() @@ -28,7 +27,7 @@ def __init__(self): def _check_connections(self) -> None: """Check that all the connections are responsive.""" - time.sleep(random.randint(1,10)) + time.sleep(random.randint(1, 10)) # NOTE: the sleep is to simulate slowness def create_order( self, @@ -37,7 +36,7 @@ def create_order( card_token: str, promo_code: str | None = None, ) -> Order: - db = SessionLocal() # creates its own session - cannot inject a test session + db = SessionLocal() try: user = db.query(User).filter(User.id == user_id).first() if not user: @@ -89,7 +88,7 @@ def create_order( total=total, promo_code=promo_code, charge_id=charge["id"], - created_at=datetime.utcnow(), # cannot control time in tests + created_at=datetime.utcnow(), ) order.items = order_items db.add(order) @@ -118,7 +117,6 @@ def cancel_order(self, order_id: int) -> Order: if order.status != OrderStatus.CONFIRMED: raise ValueError(f"Cannot cancel order in status {order.status}") - # Uses datetime.utcnow() directly - time-dependent logic is untestable deadline = order.created_at + timedelta(hours=CANCELLATION_WINDOW_HOURS) if datetime.utcnow() > deadline: raise ValueError("Cancellation window has expired (1 hour after order)") @@ -147,24 +145,3 @@ def cancel_order(self, order_id: int) -> Order: raise finally: db.close() - - def calculate_order_total( - self, items: list[dict], promo_code: str | None = None - ) -> Decimal: - # Pure calculation buried in a class that also does I/O - hard to test in isolation - db = SessionLocal() - try: - total = Decimal("0") - for item in items: - book = db.query(Book).filter(Book.id == item["book_id"]).first() - if not book: - raise ValueError(f"Book {item['book_id']} not found") - total += Decimal(str(book.price)) * item["quantity"] - - if promo_code: - discount = PROMO_CODES.get(promo_code.upper()) - if discount: - total = total * (1 - discount) - return total - finally: - db.close() diff --git a/article.md b/article.md deleted file mode 100644 index 9c1ac7c..0000000 --- a/article.md +++ /dev/null @@ -1,373 +0,0 @@ -# Sortir de l'enfer des tests - -## Intro - -La nécessité d'écrire des tests automatisés est maintenant bien établie chez les -développeurs. Il s'agit du seul moyen de tester systématiquement et complètement, pour -éviter les régressions ou tout simplement ne pas avancer à l'aveugle. - -Mais une fois posé ce consensus, les ennuis commencent. Écrire les tests est considéré -comme une corvée, souvent repoussée à la fin du projet, et qui n'attire pas les -volontaires. -Ensuite les tests sont trop souvent lents, fragiles, longs à maintenir, pas évident à -lancer, incomplets... et on finit par s'habituer à des rapports de test -négatifs ("la CI est tout le temps rouge, mais c'est normal"). -On appelle ça la normalisation de la déviance, et ça peut conduire à la catastrophe, -comme faire se crasher -[Challenger](https://fr.wikipedia.org/wiki/Accident_de_la_navette_spatiale_Challenger) -*et* [Columbia](https://fr.wikipedia.org/wiki/Accident_de_la_navette_spatiale_Columbia). - -Bon d'accord quand les tests auto sont inutiles ou absents, plutôt qu'une navette c'est -une app qui se crashe, mais le processus est le même. - -Alors, comment reprendre le contrôle de ses tests auto ? - -## Se poser les bonnes questions - -Plus que les tests eux-mêmes, l'objectif est d'assurer la qualité et la maitrise -du code, un code qu'on peut donc livrer et faire évoluer en confiance. -Les tests constituent un système logiciel, parallèle au projet principal, -dont l'objectif est d'en assurer la qualité : -fiabilité, répétabilité, maintenabilité, évolutivité. - -Comme tout système logiciel, il demande du temps de développement et des compétences -chères à acquérir. Ainsi, la suite de test constitue un investissement pour le projet -principal, qui doit donc en retirer un bénéfice sur la qualité. Il est donc crucial -de définir ce qui est attendu des tests. Comme il est toujours possible d'ajouter des -tests, il faut aussi définir les moyens alloués aux tests, qui permettront d'arbitrer -plus tard entre ajouter des tests, ou ajouter des fonctionnalités. - -Les tests automatisés, par opposition aux tests manuels, forment tout ou partie de -la suite de tests, et sont lancés automatiquement et systématiquement, -typiquement à chaque commit. -Ainsi, on s'assure contre la plupart des régressions, et cette information est apportée -rapidement au développeur. -Il s'agit donc d'une partie centrale du processus d'intégration continue (CI), au point -que "CI" désigne maintenant les pipelines de tests automatisés ! - -Certains tests restent le plus souvent manuels, par exemple -les tests utilisateurs (dont tests d'ergonomie, tests A/B...), les tests de charges, -les tests de performances, les tests de pénétration... - -Les tests automatisés sont là pour apporter un certain niveau de confiance sur la -qualité du code, afin d'atteindre les objectifs de qualité du projet, dans les moyens -alloués. Jamais oubliés, ils préviennent les bugs et empêchent la qualité de dériver, -mais sont souvent limités par la difficulté à automatiser. - -## Repartir de la base - -Les tests font partie de la boite à outil pour atteindre l'objectif de qualité, mais ils -ne sont pas seuls. Plusieurs couches successives d'outils sont nécessaires -pour intercepter les différents problèmes de qualité. - -La première couche est un formatteur de code, qui permet d'uniformiser le code. Les -avantages sont de faciliter la lecture du code (pour ceux qui lisent encore le code), -de diminuer la charge du développeur (pour ceux qui l'écrivent encore), et de nettoyer -les comparaisons de code de toutes les différences de caractères d'espacement, -ce qui diminue la taile des Pull Request / Merge Request. - -La seconde couche est constituée de logiciels d'analyse, et en premier lieu des outils -d'analyse statique. Ces derniers n'exécutent pas le code (d'où le nom de "statique") -mais cherchent des mauvaises pratiques en suivant des règles simples de reconnaissance -de texte, comme des expressions régulières. Leur gros avantage est de détecter des bugs -très rapidement et sans risque (le code ne tourne pas) et sans condition (pas besoin -d'environnement de test ou autre). - -On trouve aussi dans cette catégorie les compilateurs, surtout avec leurs warnings -activés, et les analyseurs de types (pour les codes interprétés), qui permettent -de s'assurer de la cohérence des types, toujours sans avoir exécuté une ligne du code. - -Enfin, les analyseurs dynamiques vont étudier le code pendant son fonctionnement, -souvent au prix d'une perte en performances (mémoire et CPU), pour chercher des erreurs -logiques plus difficiles à détecter (typiquement les erreurs de mémoire). - -La dernière couche est bien sûr d'automatiser tous les outils choisis, au plus près -du développeur. Déjà, un lanceur comme [`doit`](https://pydoit.org/) ou -[`just`](https://just.systems/man/en/introduction.html) permet de lancer tous les outils -de qualité choisis en une commande simple. Ensuite, nous pouvons tirer parti des hooks -de Git pour lancer automatiquement ces outils lors de certains commandes, et typiquement -lors du commit. Des utilitaires comme le fameux [`pre-commit`](https://pre-commit.com/) -ou le plus récent [`prek`](https://prek.j178.dev/) permettent de configurer facilement -les hooks de Git. Pour éviter de perdre le fil à attendre la fin de tests longs au -moment du commit, n'utiliser lors du pre-commit que des outils dont le temps d'exécution -est court (jusqu'à quelques secondes). - -## Nettoyer les tests - -Continuons par nettoyer les tests existants, car on peut bien sûr supprimer des tests. -D'abord, les tests non fiables doivent être désactivés ou supprimées, parce qu'ils -n'apportent pas l'information dont l'équipe a besoin : que le test passe ou pas, on n'en -sait pas plus sur la qualité du code testé. Ces tests pourront être réactivés une fois -stabilisés, ce qui suppose d'identifier la cause de l'instabilité. Le tirage de nombre -aléatoire, le déclenchement à partir de l'heure réelle, la (non-)synchronisation de code -exécuté en parallèle (multi threading, asynchrone...), la dépendance à des processus ou -des API externes sont les causes les plus probables. - -Par exemple, ce test échoue de façon imprévisible, car il dépend de l'heure réelle : - -```python -# Fragile : dépend de l'horloge système... et reste bloqué une heure ! -def test_reporting_each_hour(): - manager = Manager() # crée un rapport à chaque heure pile - assert manager.report is None # faux si le test est lancé à une heure pile - time.sleep(3600) # c'est long... - assert manager.report is not None -``` - -La solution est d'injecter l'horloge pour la contrôler depuis le test, ce qui le rend -à la fois fiable et instantané : - -```python -# Fiable : le temps est injecté, donc maîtrisé -def test_reporting_each_hour(): - clock = FakeClock(now=datetime(2025, 1, 1, 12, 1)) - manager = Manager(clock) # refacto de Manager - assert manager.report is None # démarrage 1 minute après midi : pas de rapport - clock.advance(timedelta(hours=1)) # on avance le temps sans attendre - assert manager.report is not None # nouveau rapport -``` - -Ensuite, les tests trop longs n'apportent pas l'information à temps aux développeurs. -Ils peuvent être supprimés, accélérés, ou séparés dans une seconde suite de -tests si vraiment le test est nécessaire et le temps d'exécution vraiment incompressible. -Cette seconde suite de tests sera toujours automatisée, mais pas lancée à chaque commit, -à une fréquence plus faible, comme une fois par jour (souvent la nuit), -ou seulement juste avant fusion sur la branche principale. L'important est que le -développeur n'ait pas besoin du résultat immédiat du test pour continuer à travailler, -mais que l'information apportée par cette suite de tests longs ne soit nécessaire -par exemple que pour une nouvelle version. - -Enfin, il ne faut pas hésiter à élaguer la suite de tests : certains tests ne sont pas -pertinents, parce que trop vieux, trop spécifiques, trop couplés ou qui testent trop peu -de code. - -## Rendre le code testable - -Les problèmes de test les plus sérieux sont le plus souvent dus au code à tester : -trop couplé et pas assez observable, il oblige le test à mettre en place tout un -environnement, à lancer tout le code d'un coup, à jouer tout un scénario complexe pour -atteindre l'état initial dans lequel le test pourra enfin être lancé, et à passer par -des moyens détournés ou fragiles (analyse des logs...) pour savoir si le test a réussi. - -La première chose est d'isoler au maximum les effets de bord, en particulier les -entrées/sorties, pour pouvoir tester le code spécifique au projet et la logique métier -indépendamment. Pour tous les projets qui ne sont pas de simples "passe-plats", un -maximum de code doit être testable sans préparation particulière, juste en l'exécutant. - -Trions ensuite les effets de bord en 3 catégories : trous noirs, fontaine blanche, et -le reste. Un trou noir est en écriture seule, on ne peut qu'y jeter des données qui sont -immédiatemement perdues. On trouve dans cette catégorie les systèmes de log et de -télémétrie. Une fontaine blanche, l'inverse théorique d'un trou noir, est en lecture -seule et ne peut être qu'initialisée, une seule fois. On trouve dans cette catégorie -le système de configuration, qui réconcilie variables d'environnement, fichiers, -ligne de commande... et met le résultat à disposition du reste du code. - -Trous noirs et fontaines blanches peuvent avoir une durée de vie équivalente à celle -du programme entier et une portée globale. Ce sont les seuls effets de bord qu'on peut -tolérer à travers toute la base de code. Bien identifiés, une mise en place commune à -tous les test permettra de les gérer une fois pour toute. - -Tous les autres effets de bords doivent être isolés, et si possible injectés dans le -code métier propre à leur utilisation, plutôt que détenus par le code métier. En effet, -une dépendance injectée donne toute liberté au test pour déconnecter l'effet de bord -et isoler le code, alors qu'un couplage fort rend le test difficile, surtout avec -les langages statiques. Avec ces langages, il faut par contre prévoir un type approprié -pour les membres de classes qui seront remplacés par un substitut. La testabilité doit -alors être prévue à la conception. - -```c++ -// Difficile à tester, instancie toujours une vraie connexion pas substituable -class MyClassWithInternalConnection { -private: - Client client; -public: - MyClassWithInternalConnection(const char* host, int port) { client = Client(host, port); } -}; -// Facile à tester, peut instancier un substitut comme une vraie connexion. Mais plus -// complexe avec un langage statique, propriété et cycle de vie du client à définir. -class MyClassWithInvertedDependency { -private: - // Nouveau type pour contenir la vraie connexion ou un substitut - AbstractClient& client; -public: - // Dans cet exemple l'instance de classe prend la propriété du client - MyClassWithInvertedDependency(AbstractClient&& client_): client(client_) {} -}; -``` - -## Prendre en main son framework de test - -Tous les principaux langages ont un ou plusieurs frameworks de test, qui apportent -leur lot de fonctionnalités en plus de faciliter l'écriture des tests, comme : - -- lancement depuis un point unique -- organisation en suite de tests, suite de suites... -- gestion de l'environnement, de l'OS, des signaux -- fichiers et dossiers temporaires -- interception de l'entrée standard et des sorties standards -- rassemblement et structuration des résultats des tests -- setup/teardown : une paire de fonction spéciale pour respectivement créer et détruire - l'état initial d'un test -- fixture : une fonction qui "fixe" l'état initial pour un test (généralisation du - setup) -- assertions pour comparer des résultats à des valeurs attendues, ou vérifier un - comportement attendu (exception, appel de fonction...) -- injection de substitut ou mock -- parallélisation, plugins... - -Utiliser un framework de test apporte donc énormément, au prix du temps vite rentabilisé -de prise en main du framework. - -L'injection de substitut mérite un point d'attention : il s'agit de remplacer -sélectivement du code difficile à tester (souvent avec un effet de bord) par du code -propre au test, en réimplémentant l'interface du code original. -Par exemple, un objet de connexion à une base de données peut être remplacé par un -objet ne faisant rien, en renvoyant immédiatement une valeur fixée. - -Le substitut peut être développé spécifiquement pour émuler un comportement complexe -ou aléatoire, mais avec un retour instantané et répétable. -Il peut aussi être complètement vide, mais permettre de vérifier que l'interface a été -appelée, avec quels arguments... -Certains frameworks de langages dynamiques permettent même de créer des substituts sans -déclarer l'interface du tout, l'interface étant créée à la volée, pour juste débrancher -un effet de bord. -De nombreux plugins ou librairies proposent des substituts clés en main pour les cas -les plus courants : connexion, temps, aléatoire, base de données, logs... - -En combinant ces fonctionnalités, un test reste court et lisible. La structure -given/when/then (« étant donné / quand / alors », ou encore arrange/act/assert) -sépare nettement la préparation, l'action testée et la vérification, ce qui en fait -une documentation exécutable : - -```python -def test_alerte_envoyee_si_solde_negatif(): - # given : un compte à découvert et un service d'envoi de mail substitué - mailer = FakeMailer() - compte = Compte(solde=-50, mailer=mailer) - - # when : on déclenche la vérification du découvert - compte.verifier_decouvert() - - # then : une alerte, et une seule, a été envoyée au titulaire - assert mailer.envois == [("alerte_decouvert", compte.titulaire)] -``` - -Ici le substitut `FakeMailer` n'envoie aucun vrai mail : il se contente d'enregistrer -les appels, ce qui permet d'affirmer que l'alerte a bien été déclenchée, et une seule -fois, sans dépendre d'un serveur de messagerie. - -## Accélérer les tests - -De nombreuses pistes peuvent permettre d'accélérer les tests : - -- modulariser le code et éventuellement le séparer en plusieurs librairies. Ainsi, il y - a moins de code, d'entrées, de cas limites à tester, et chaque librairie est validée - indépendamment. -- préparer les artefacts (libs, exécutables, images...) pour le test (et la compilation - si nécessaire) pour qu'ils soient prêts à l'emploi et en cache. Utiliser une lib - précompilée est plus rapide que la recompiler avec tout le projet. Si un test a besoin - d'un conteneur, l'image doit être disponible en cache et prête à l'emploi dès que le - conteneur est lancé (pas d'entrypoint qui finit la mise en place par exemple). -- profiler ses tests, pour d'abord avoir le temps par test, et ensuite savoir où les - tests longs passent leur temps. - Cette étape est essentielle pour ne pas avancer à l'aveugle. -- paralléliser les tests (mais les workers de CI peuvent n'avoir qu'un seul coeur) -- factoriser la mise en place et le nettoyage entre plusieurs tests -- éviter d'attendre pendant un test (`sleep(...)`), à la place réagir quand l'action est - terminée. Il faut parfois modifier le code pour qu'il reporte son état (code de - retour, log, variable interne...) ou permette au test de l'obtenir avec une nouvelle - API. -- réduire la quantité de valeurs différentes testées, pour se concentrer sur les valeurs - attendues et les cas limites. -- focaliser les tests sur une séquence claire given/when/then, en évitant d'enchainer - trop d'actions -- optimiser le code dont le test est irréductiblement long. Parfois le problème vient du - code. -- activer sélectivement certaines options qui ralentissent les tests, comme la mesure - de la couverture ou la vérification de l'intégrité de la mémoire -- lancer sélectivement les tests selon le code modifié, facile quand le code est bien - structuré et modulaire - -Le remplacement d'une attente fixe par une attente conditionnelle illustre bien ce -gain : au lieu de dormir « assez longtemps » en espérant que l'action soit terminée -(lent, et fragile si la machine est chargée), on rend la main dès que la condition est -remplie. - -```python -# Lent et fragile : on attend une durée arbitraire -start_job() -time.sleep(5) -assert result_file.exists() - -# Rapide et fiable : on réagit dès que la condition est vraie -job = start_job() -wait_until(lambda: job.is_done(), timeout=5) # sort dès que c'est prêt -assert result_file.exists() -``` - -## Compléter les tests - -Une fois les tests existants remis en ordre de marche, il faut vérifier que la suite de -test atteint son objectif, et en particulier qu'il ne reste pas de gros trou dans la -couverture de test. Si ce n'est pas déjà fait, il faut ajouter la mesure de la -couverture à son framework. Attention, mesurer la couverture a tendance à ralentir les -tests, parfois beaucoup, et il faut alors prévoir une option pour l'activer -sélectivement. - -La couverture de test doit déjà être à peu près homogène sur la base de code, et donc la -première chose à rechercher est une partie du code non testée (fichier complet, classe, -fonction...). Ensuite, la couverture doit permettre de vérifier que la logique métier -(sans effets de bord) est très bien testée, avec une couverture tendant vers 100%. -Enfin, la couverture doit être analysée par rapport aux objectifs et ressources : est-ce -que le code non testé peut l'être facilement ? Est-il réutilisé massivement dans toute -la base de code ? Est-il critique ? Est-ce de la logique métier ? - -Une fois la couverture analysée, c'est le moment de revenir sur les tests des -entrées/sorties, dont la couverture de test n'est pas la métrique pertinente. Il est en -effet facile de les couvrir par des tests qui déconnectent tellement d'effets de bord -que le test n'apporte plus beaucoup de garantie. Il faut plutôt vérifier que les cas -normaux, limites et d'erreurs sont testés, vérifier que le lien avec le reste du système -soit réellement testé à un moment ou un autre (connexion à l'API, base de données...), -ou envisager un contrat de communication à valider (OpenAPI par exemple). - -Il n'y a pas de chiffre précis de taux de couverture à atteindre, surtout qu'il est -relativement facile de biaiser ce chiffre en déclarant des lignes hors couverture ou en -abusant des substituts. De plus, les derniers pourcents sont bien plus difficiles à -atteindre que les premiers. Un chiffre de 80% est une bonne base de réflexion, les -projets de qualité étant au-dessus. Il est plus important de suivre la tendance : au fur -et à mesure que le projet évolue, la couverture ne doit pas reculer, mais monter (même -lentement) au fur et à mesure que des bugs sont corrigés et donc des tests ajoutés. -Ajouter à ses pipelines de test la vérification que la couverture ne recule pas -empêche en particulier d'ajouter des fonctionnalités sans leurs tests. - -## Utiliser l'IA à bon escient - -Pour toutes ces tâches, plutôt rébarbatives et vues comme une perte de temps par rapport -à l'avancée du projet, l'IA est un auxiliaire précieux. Il peut être très tentant de -déléguer tout le problème à l'IA (et les derniers modèles feront un bon travail). -Mais les tests sont aussi le système qui garantit le fonctionnement du code, -et qui permettent au développeur d'engager sa responsabilité sur le code livré. -Est-ce que le test généré teste vraiment les points importants ? Ou est-ce qu'il s'agit -juste d'un spaghetti de substituts qui ne teste en fait rien du tout ? Dans tous les cas -la responsabilité est sur les épaules du développeur. - -Selon les développeurs et les projets, plusieurs stratégies sont possibles : générer -mais relire soigneusement, travailler par étape à partir des spécifications, écrire -manuellement les tests ou au moins les tests principaux, utiliser une IA vérificatrice -après la génération... - -Par délà l'appropriation des tests générés par le développeur, l'IA est surtout une -opportunité en or d'aller plus loin dans les tests : test de performances et -optimisation, test d'interface, fuzz testing, mutation testing... Autant de stratégies -de test avancées qui deviennent accessibles à tous les projets. - -## Conclusion - -Une fois repris en main, les tests apportent la confiance indispensable pour avancer, -et deviennent un outil de développement qui permet de livrer vite et bien. -À ce moment, l'équipe a acquis toute une gamme de nouvelles compétences, -de la prise en main du framework à l'optimisation, de la refactorisation du code à -l'ajout des tests manquant, qui rendent l'écriture de nouveaux tests banale. -Devenu un outil du quotidien, le test peut être écrit d'abord, dans certains cas en -partant des spécifications, faisant d'une corvée repoussée en fin de projet une étape -préparatoire qui signalera la fin du développement et garantira la qualité. diff --git a/docker-compose.yml b/docker-compose.yml index 923b835..ac3da29 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,20 +1,20 @@ services: db: - image: postgres:16 + image: docker.io/postgres:16 environment: POSTGRES_USER: bookstore POSTGRES_PASSWORD: bookstore POSTGRES_DB: bookstore ports: - - "5432:5432" + - "127.0.0.1:5432:5432" volumes: - pgdata:/var/lib/postgresql/data mailpit: - image: axllent/mailpit:latest + image: docker.io/axllent/mailpit:latest ports: - - "1025:1025" # SMTP - - "8025:8025" # Web UI + - "127.0.0.1:1025:1025" # SMTP + - "127.0.0.1:8025:8025" # Web UI volumes: pgdata: diff --git a/solution/.github/workflows/ci.yml b/solution/.github/workflows/ci.yml index 4db9fd3..b0e9b41 100644 --- a/solution/.github/workflows/ci.yml +++ b/solution/.github/workflows/ci.yml @@ -28,7 +28,7 @@ jobs: uv-${{ runner.os }}- - name: Install dependencies - run: uv sync --extra solution + run: uv sync --group test --group solution - name: Run tests with coverage run: | diff --git a/solution/README.md b/solution/README.md index 87d426e..75aeedc 100644 --- a/solution/README.md +++ b/solution/README.md @@ -23,11 +23,11 @@ This directory contains the refactored code and improved tests you'll build duri ## Running the solution tests ```bash -uv sync --extra solution +uv sync --group solution uv run pytest solution/tests/ -v ``` -The `solution` extra is defined in `pyproject.toml` and bundles everything the reference tests need: +The `solution` group is defined in `pyproject.toml` and bundles everything the reference tests need: ```toml [project.optional-dependencies] diff --git a/solution/app/api_orders.py b/solution/app/api_orders.py index 9b563db..93a6753 100644 --- a/solution/app/api_orders.py +++ b/solution/app/api_orders.py @@ -9,7 +9,7 @@ from fastapi import APIRouter, Depends, HTTPException from solution.app.clients.email_client import EmailClient -from solution.app.clients.payment_client import PaymentClient +from solution.app.clients.payment_client import PaymentClient, PaymentError from solution.app.order_service import OrderService from solution.app.settings import Settings, get_settings from sqlalchemy.orm import Session @@ -58,7 +58,7 @@ def create_order( card_token=card_token, promo_code=payload.promo_code, ) - except ValueError as exc: + except PaymentError as exc: raise HTTPException(status_code=400, detail=str(exc)) from exc diff --git a/solution/app/clients/payment_client.py b/solution/app/clients/payment_client.py index e68a1c9..d083a48 100644 --- a/solution/app/clients/payment_client.py +++ b/solution/app/clients/payment_client.py @@ -1,4 +1,5 @@ """Payment client configured via the :class:`~solution.app.settings.Settings`.""" + from decimal import Decimal from http import HTTPStatus diff --git a/solution/app/order_service.py b/solution/app/order_service.py index dcb7ab6..a93393f 100644 --- a/solution/app/order_service.py +++ b/solution/app/order_service.py @@ -5,11 +5,12 @@ 2. `datetime.utcnow` replaced by an injectable `now` callable 3. `compute_total()` extracted as a pure function - no I/O """ + import logging import random import time from collections.abc import Callable -from datetime import datetime, timedelta +from datetime import datetime, timedelta, UTC from decimal import Decimal from solution.app.clients.email_client import EmailClient @@ -32,6 +33,7 @@ log = logging.getLogger(__name__) + def compute_total( prices: list[tuple[Decimal, int]], promo_code: str | None = None, @@ -56,7 +58,7 @@ def __init__( db: Session, payment: PaymentClient, email: EmailClient, - now: Callable[[], datetime] = datetime.utcnow, + now: Callable[[], datetime] = lambda: datetime.now(tz=UTC), ) -> None: """Store the injected DB session, payment/email clients and clock.""" self._db = db @@ -68,7 +70,7 @@ def __init__( def _check_connections(self) -> None: """Check that all the connections are responsive.""" - time.sleep(random.randint(1,10)) + time.sleep(random.randint(1, 10)) log.info(f"Checking connections at {self._now()}: OK") def create_order( diff --git a/solution/docs/test_pyramid_after.excalidraw.png b/solution/docs/test_pyramid_after.excalidraw.png new file mode 100644 index 0000000..837a502 Binary files /dev/null and b/solution/docs/test_pyramid_after.excalidraw.png differ diff --git a/solution/docs/test_pyramid_before.excalidraw.png b/solution/docs/test_pyramid_before.excalidraw.png new file mode 100644 index 0000000..80d94a3 Binary files /dev/null and b/solution/docs/test_pyramid_before.excalidraw.png differ diff --git a/solution/docs/testing_hell.excalidraw.png b/solution/docs/testing_hell.excalidraw.png new file mode 100644 index 0000000..3028486 Binary files /dev/null and b/solution/docs/testing_hell.excalidraw.png differ diff --git a/solution/docs/testing_hell_getting_out.excalidraw.png b/solution/docs/testing_hell_getting_out.excalidraw.png new file mode 100644 index 0000000..120501b Binary files /dev/null and b/solution/docs/testing_hell_getting_out.excalidraw.png differ diff --git a/solution/tests/conftest.py b/solution/tests/conftest.py index 7dc95bd..df7f307 100644 --- a/solution/tests/conftest.py +++ b/solution/tests/conftest.py @@ -77,12 +77,13 @@ def api_client(db): # --- Fakes shared by service-level and API-level tests --- + @pytest.fixture(autouse=True) def no_connection_check(monkeypatch): """Replace OrderService._check_connections with a no-op mock. - The real implementation sleeps 1-10s on every instantiation - fine to - exercise once, useless to pay for on every test. + The real implementation sleeps 1-10s on every instantiation - fine to + exercise once, useless to pay for on every test. """ monkeypatch.setattr(OrderService, "_check_connections", MagicMock()) @@ -109,7 +110,7 @@ def service(db, fake_payment, fake_email): @pytest.fixture -def orders_api_client(db, payment, email): +def orders_api_client(db, fake_payment, fake_email): """Return a TestClient built around the *refactored* orders router. Depends-based injection, so payment + email are real fakes and the DB is the @@ -121,8 +122,8 @@ def orders_api_client(db, payment, email): test_app.include_router(orders_router) test_app.dependency_overrides[get_db] = lambda: db - test_app.dependency_overrides[get_payment_client] = lambda: payment - test_app.dependency_overrides[get_email_client] = lambda: email + test_app.dependency_overrides[get_payment_client] = lambda: fake_payment + test_app.dependency_overrides[get_email_client] = lambda: fake_email with TestClient(test_app) as client: yield client diff --git a/solution/tests/test_cancellation.py b/solution/tests/test_cancellation.py index 47899f2..13fea78 100644 --- a/solution/tests/test_cancellation.py +++ b/solution/tests/test_cancellation.py @@ -42,7 +42,11 @@ class TestCancellationWindow: """Cancellation is allowed up to the deadline and rejected after it.""" def test_just_before_deadline_succeeds( - self, db, fake_payment, fake_email, existing_order, + self, + db, + fake_payment, + fake_email, + existing_order, ): """Cancelling one second before the deadline succeeds.""" just_before = ORDER_TIME + WINDOW - timedelta(seconds=1) @@ -53,7 +57,11 @@ def test_just_before_deadline_succeeds( assert cancelled.status == OrderStatus.CANCELLED def test_exactly_at_deadline_succeeds( - self, db, fake_payment, fake_email, existing_order, + self, + db, + fake_payment, + fake_email, + existing_order, ): """Cancelling exactly at the deadline succeeds (strict comparison).""" # The implementation uses `now > deadline` (strict), so equality is allowed. @@ -64,7 +72,11 @@ def test_exactly_at_deadline_succeeds( assert cancelled.status == OrderStatus.CANCELLED def test_one_second_after_deadline_raises( - self, db, fake_payment, fake_email, existing_order, + self, + db, + fake_payment, + fake_email, + existing_order, ): """Cancelling one second after the deadline raises an expiry error.""" just_after = ORDER_TIME + WINDOW + timedelta(seconds=1) @@ -74,7 +86,11 @@ def test_one_second_after_deadline_raises( svc.cancel_order(existing_order.id) def test_hours_after_deadline_raises( - self, db, fake_payment, fake_email, existing_order, + self, + db, + fake_payment, + fake_email, + existing_order, ): """Cancelling many hours after the deadline raises an expiry error.""" much_later = ORDER_TIME + timedelta(hours=24) @@ -88,11 +104,18 @@ class TestCancellationSideEffects: """Cancelling an order refunds, restores stock and notifies the buyer.""" def test_refunds_the_original_charge( - self, db, fake_payment, fake_email, existing_order, + self, + db, + fake_payment, + fake_email, + existing_order, ): """Cancelling refunds the charge made when the order was created.""" svc = _service_at( - db, fake_payment, fake_email, ORDER_TIME + timedelta(minutes=5), + db, + fake_payment, + fake_email, + ORDER_TIME + timedelta(minutes=5), ) svc.cancel_order(existing_order.id) @@ -123,7 +146,10 @@ def test_restores_stock_for_every_item( ) _service_at( - db, fake_payment, fake_email, ORDER_TIME + timedelta(minutes=5), + db, + fake_payment, + fake_email, + ORDER_TIME + timedelta(minutes=5), ).cancel_order( order.id, ) @@ -134,11 +160,18 @@ def test_restores_stock_for_every_item( assert book_b.stock == 8 def test_sends_cancellation_email( - self, db, fake_payment, fake_email, existing_order, + self, + db, + fake_payment, + fake_email, + existing_order, ): """Cancelling sends a single cancellation email to the buyer.""" svc = _service_at( - db, fake_payment, fake_email, ORDER_TIME + timedelta(minutes=10), + db, + fake_payment, + fake_email, + ORDER_TIME + timedelta(minutes=10), ) svc.cancel_order(existing_order.id) @@ -166,7 +199,10 @@ def test_already_cancelled_order_cannot_be_cancelled_again( ): """Cancelling an already-cancelled order raises a 'Cannot cancel' error.""" svc = _service_at( - db, fake_payment, fake_email, ORDER_TIME + timedelta(minutes=5), + db, + fake_payment, + fake_email, + ORDER_TIME + timedelta(minutes=5), ) svc.cancel_order(existing_order.id) diff --git a/solution/tests/test_order_service.py b/solution/tests/test_order_service.py index 3d659c0..051a4fe 100644 --- a/solution/tests/test_order_service.py +++ b/solution/tests/test_order_service.py @@ -9,6 +9,7 @@ import pytest from app.models.order import OrderStatus from solution.app.order_service import OrderService, compute_total +from app.clients.payment_client import PaymentError from .fakes import FakeEmailClient, FakePaymentClient @@ -102,7 +103,7 @@ def test_applies_promo_code(self, service, make_user, make_book): assert order.total == Decimal("90.0") - def test_sends_confirmation_email(self, service, make_user, make_book, email): + def test_sends_confirmation_email(self, service, make_user, make_book, fake_email): """Creating an order sends one confirmation email to the buyer.""" user = make_user(email="buyer@example.com") book = make_book() @@ -113,11 +114,11 @@ def test_sends_confirmation_email(self, service, make_user, make_book, email): card_token="tok_visa", ) - assert len(email.sent) == 1 - assert email.last_email.to == "buyer@example.com" - assert "confirmed" in email.last_email.subject.lower() + assert len(fake_email.sent) == 1 + assert fake_email.last_email.to == "buyer@example.com" + assert "confirmed" in fake_email.last_email.subject.lower() - def test_charges_payment(self, service, make_user, make_book, payment): + def test_charges_payment(self, service, make_user, make_book, fake_payment): """Creating an order charges the payment client for the order total.""" user = make_user() book = make_book(price=Decimal("25.00"), stock=2) @@ -128,8 +129,8 @@ def test_charges_payment(self, service, make_user, make_book, payment): card_token="tok_visa", ) - assert payment.last_charge is not None - assert payment.last_charge.amount == 5000 # €50.00 in cents + assert fake_payment.last_charge is not None + assert fake_payment.last_charge.amount == 5000 # €50.00 in cents def test_raises_for_unknown_user(self, service): """Ordering for an unknown user raises a 'not found' error.""" @@ -165,7 +166,7 @@ def test_payment_failure_rolls_back_stock(self, db, make_user, make_book): failing_payment = FakePaymentClient(fail_on_token="tok_decline") svc = OrderService(db=db, payment=failing_payment, email=FakeEmailClient()) - with pytest.raises(ValueError): # noqa: PT011 + with pytest.raises(PaymentError): # noqa: PT011 svc.create_order( user_id=user.id, items=[{"book_id": book.id, "quantity": 2}], diff --git a/solution/tests/test_orders_api.py b/solution/tests/test_orders_api.py index 490cbb1..469bbd1 100644 --- a/solution/tests/test_orders_api.py +++ b/solution/tests/test_orders_api.py @@ -47,7 +47,9 @@ def test_returns_201_with_confirmed_order(self, orders_api_client, user, book): assert Decimal(data["total"]) == Decimal("60.00") assert data["items"][0]["quantity"] == 2 - def test_charges_the_payment_client(self, orders_api_client, user, book, payment): + def test_charges_the_payment_client( + self, orders_api_client, user, book, fake_payment + ): """Creating an order charges the payment client for the right amount.""" orders_api_client.post( "/orders/", @@ -58,11 +60,13 @@ def test_charges_the_payment_client(self, orders_api_client, user, book, payment }, ) - assert payment.last_charge is not None - assert payment.last_charge.card_token == "tok_visa" - assert payment.last_charge.amount == 3000 # €30 in cents + assert fake_payment.last_charge is not None + assert fake_payment.last_charge.card_token == "tok_visa" + assert fake_payment.last_charge.amount == 3000 # €30 in cents - def test_sends_a_confirmation_email(self, orders_api_client, user, book, email): + def test_sends_a_confirmation_email( + self, orders_api_client, user, book, fake_email + ): """Creating an order sends one confirmation email to the buyer.""" orders_api_client.post( "/orders/", @@ -73,8 +77,8 @@ def test_sends_a_confirmation_email(self, orders_api_client, user, book, email): }, ) - assert len(email.sent) == 1 - assert email.last_email.to == "buyer@example.com" + assert len(fake_email.sent) == 1 + assert fake_email.last_email.to == "buyer@example.com" def test_decreases_book_stock(self, orders_api_client, user, book, db): """Creating an order decreases the ordered book's stock.""" @@ -180,7 +184,7 @@ def test_unknown_order_returns_404(self, orders_api_client): class TestCancelOrderEndpoint: """End-to-end behaviour of POST /orders/{id}/cancel.""" - def test_cancels_within_window(self, orders_api_client, user, book, payment): + def test_cancels_within_window(self, orders_api_client, user, book, fake_payment): """Cancelling within the window returns 200 and refunds the charge.""" created = orders_api_client.post( "/orders/", @@ -195,7 +199,7 @@ def test_cancels_within_window(self, orders_api_client, user, book, payment): assert response.status_code == 200 assert response.json()["status"] == OrderStatus.CANCELLED.value - assert payment.charges[0].refunded is True + assert fake_payment.charges[0].refunded is True def test_unknown_order_returns_400(self, orders_api_client): """Cancelling an unknown order returns 400 (mapped from ValueError).""" diff --git a/tests/test_orders.py b/tests/test_orders.py index 3895085..96fce43 100644 --- a/tests/test_orders.py +++ b/tests/test_orders.py @@ -3,8 +3,6 @@ - A real PostgreSQL at localhost:5432 - The app running at localhost:8000 - A real payment API (or specific env vars) - -Many tests will silently skip or lie if the environment isn't set up correctly. """ import os @@ -14,14 +12,12 @@ BASE_URL = "http://localhost:8000" DB_DSN = "postgresql://bookstore:bookstore@localhost:5432/bookstore" -# State shared across test file - fragile _user_id = None _book_id = None _order_id = None def setup_test_data(): - """Called manually from some tests, not others - inconsistent.""" global _user_id, _book_id user_resp = requests.post( f"{BASE_URL}/users/", json={"email": "tester@example.com", "name": "Tester"} @@ -36,7 +32,7 @@ def setup_test_data(): def test_create_order_success(): global _order_id - setup_test_data() # sets up data - but also contaminates DB permanently + setup_test_data() response = requests.post( f"{BASE_URL}/orders/", @@ -46,25 +42,22 @@ def test_create_order_success(): "items": [{"book_id": _book_id, "quantity": 2}], }, ) - # Will fail if payment API is not reachable - no indication it's an integration test + assert response.status_code == 201 _order_id = response.json()["id"] assert response.json()["status"] == "confirmed" def test_order_reduces_stock(): - # Directly queries DB - bypasses the API entirely conn = psycopg2.connect(DB_DSN) cursor = conn.cursor() cursor.execute("SELECT stock FROM books WHERE id = %s", (_book_id,)) row = cursor.fetchone() conn.close() - # Magic number - 3 because we started with 5 and ordered 2 assert row[0] == 3 def test_order_with_promo_code(): - # Calls setup again - creates duplicate user (409) and duplicate book, silently ignores errors setup_test_data() response = requests.post( @@ -77,27 +70,25 @@ def test_order_with_promo_code(): }, ) if response.status_code != 201: - return # Silently passes if it fails! + return data = response.json() assert data["promo_code"] == "SAVE10" - # Does not check that the discount was actually applied to the total def test_cancel_order(): - # Depends on test_create_order_success having populated _order_id response = requests.post(f"{BASE_URL}/orders/{_order_id}/cancel") assert response.status_code == 200 def test_cancel_expired_order(): """Test that cancellation fails after the 1-hour window.""" - # This test actually waits - commented out in practice because it takes too long + # This test actually waits - commented out because it takes too long # Uncomment to run (takes ~3700 seconds): # time.sleep(3700) # response = requests.post(f"{BASE_URL}/orders/{_order_id}/cancel") # assert response.status_code == 400 # assert "expired" in response.json()["detail"] - pass # always passes, tests nothing + pass def test_order_not_found(): @@ -118,12 +109,12 @@ def test_order_invalid_user(): def test_db_state_after_tests(): - """Verify the DB has the expected rows - highly environment-dependent.""" + """Verify the DB has the expected rows.""" if not os.getenv("CHECK_DB_STATE"): - return # Skip silently unless env var is set + return conn = psycopg2.connect(DB_DSN) cursor = conn.cursor() cursor.execute("SELECT COUNT(*) FROM orders") count = cursor.fetchone()[0] conn.close() - assert count > 0 # Checks almost nothing + assert count > 0 diff --git a/tests/test_services.py b/tests/test_services.py index d249747..a4921e5 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -1,6 +1,5 @@ """ -Service-layer tests. These look like unit tests but are secretly coupled -to the real database and use overly-patched mocks that test nothing real. +Service-layer tests. """ from decimal import Decimal @@ -11,31 +10,23 @@ from app.services.order_service import OrderService, PROMO_CODES -# --- Tests that look like unit tests but require a real DB --- - - class TestOrderServiceCalculation: - def test_promo_code_save10(self): - # Calls calculate_order_total which opens a real DB connection + @patch("app.services.order_service.SessionLocal") + @patch("app.services.order_service.PaymentClient") + @patch("app.services.order_service.EmailClient") + def test_promo_code_save10( + self, mock_email_cls, mock_payment_cls, mock_session_cls + ): service = OrderService() - # This will raise OperationalError if no DB is running - no clear error message - total = service.calculate_order_total( - items=[{"book_id": 1, "quantity": 1}], - promo_code="SAVE10", - ) - # Only checks it's not None - would pass even if discount wasn't applied - assert total is not None + order = service.create_order(1, [], "tok_visa", "SAVE10") + assert order.promo_code == "SAVE10" def test_promo_codes_are_defined(self): - # Tests a dict constant, not any behavior assert "SAVE10" in PROMO_CODES assert "SAVE20" in PROMO_CODES assert PROMO_CODES["SAVE10"] == Decimal("0.10") -# --- Tests that mock so much they verify nothing --- - - class TestOrderCreation: @patch("app.services.order_service.SessionLocal") @patch("app.services.order_service.PaymentClient") @@ -43,7 +34,6 @@ class TestOrderCreation: def test_create_order_calls_payment( self, mock_email_cls, mock_payment_cls, mock_session_cls ): - # Build a maze of mocks mock_session = MagicMock() mock_session_cls.return_value = mock_session @@ -78,7 +68,6 @@ def test_create_order_calls_payment( card_token="tok_visa", ) - # Only verifies the mock was called - not that business logic is correct mock_payment.charge.assert_called_once() @patch("app.services.order_service.SessionLocal") @@ -87,7 +76,6 @@ def test_create_order_calls_payment( def test_create_order_sends_email( self, mock_email_cls, mock_payment_cls, mock_session_cls ): - # Copy-paste of the above setup - no shared fixture mock_session = MagicMock() mock_session_cls.return_value = mock_session @@ -120,7 +108,6 @@ def test_create_order_sends_email( card_token="tok_visa", ) - # Checks the mock was called - does not check email content or recipient mock_email.send.assert_called_once() @patch("app.services.order_service.SessionLocal") @@ -145,11 +132,6 @@ def test_inactive_user_raises( service.create_order(user_id=1, items=[], card_token="tok_visa") -# --- Test that never fails --- - - def test_order_service_instantiation(): - # Instantiating the service tries to connect to the payment API URL - but doesn't fail yet - # This test always passes and asserts nothing meaningful service = OrderService() assert service is not None