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

Tests : vérifier le contenu de la modale confirmant un candidat trouvé par son NIR ou son email #5523

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

EwenKorr
Copy link
Contributor

@EwenKorr EwenKorr commented Feb 3, 2025

🤔 Pourquoi ?

Une modale s'affiche lorsqu'on trouve un compte candidat à partir de son NIR ou de son adresse email.
Cette modale s'affiche dans différents contextes (candidature, GPS, …) et devient assez complexe.
Testons son contenu.

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?
  • Ajouter l'étiquette « Bug » ?

🏝️ Comment tester ?

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

@EwenKorr EwenKorr added the no-changelog Ne doit pas figurer dans le journal des changements. label Feb 3, 2025
@EwenKorr EwenKorr self-assigned this Feb 3, 2025
@EwenKorr EwenKorr force-pushed the ewen/test-modale branch 2 times, most recently from 61a7ae8 to b282d16 Compare February 3, 2025 13:15
@EwenKorr EwenKorr marked this pull request as ready for review February 3, 2025 13:19
Copy link
Contributor

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toujours cool d’augmenter la couverture de tests. 🙏

test_submit.py est affreux, mais ce n’est pas de ton fait.

tests/gps/test_create_beneficiary.py Outdated Show resolved Hide resolved
tests/gps/test_create_beneficiary.py Outdated Show resolved Hide resolved
tests/gps/test_create_beneficiary.py Outdated Show resolved Hide resolved
tests/www/apply/test_submit.py Outdated Show resolved Hide resolved
tests/www/apply/test_submit.py Outdated Show resolved Hide resolved
tests/www/apply/test_submit.py Outdated Show resolved Hide resolved
response = client.get(check_nir_url)
assert response.status_code == 200
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dans l’idéal de l’idéal, ce retrait d’assertions en doublon mériterait un autre commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allez, c'était l'argument qu'il me fallait pour que je me note de faire une passe sur ces tests et supprimer ces assertions inutiles 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️, ecaf16c se faisait vieux.

@francoisfreitag
Copy link
Contributor

Petite idée encore plus maline pour l’agencement des commits. Actuellement, le premier commit améliore le rendu sans tests, qui sont ajoutés avec le second commit.
Tu pourrais inverser les deux commits, pour que le premier ajoute les tests (constat du problème existant) et avoir le second commit qui corrige, et met à jour les « snapshots » créés dans le premier commit.

@EwenKorr
Copy link
Contributor Author

EwenKorr commented Feb 3, 2025

Oh, c'est élégant !

@@ -21,6 +22,67 @@
from tests.utils.test import KNOWN_SESSION_KEYS


def assert_contains_nir_modal_gps(response, job_seeker):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: j'aurais nommé différemment : assert_contains_nir_modal_gpsassert_modal_gps_contains_nir

C'est "plus anglais" et introduit une hiérarchie : cible > action > donnée

Dans tous les cas on n'est pas sur la méthode unittest ni sur les helpers de pytest-django (et on ne camelize pas nous 😈)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ idem pour les autres méthodes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert_modal_gps_contains_nir me fait penser à "la modale contient le NIR"

Et là c'est plutôt : "la réponse contient la modale du NIR" (dans le cas GPS).

Qu'en penses-tu ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oui je vois où tu veux en venir.

Disons que l'on n'a qu'une modale par step de ce que j'ai vu donc ça en revient à vérifier le contenu de la modale. Mais par rapport à ton test, qui s'assure bien de la présence d'une modale avec tel contenu, ton nommage est sémantiquement plus juste en effet. Laissons tel quel ou bien : assert_contains_apply_nir_modal / assert_contains_gps_nir_modal / assert_contains_gps_email_modal

tests/www/apply/test_submit.py Outdated Show resolved Hide resolved
This modal is displayed when a job seeker is found with a NIR or an
email. It is more and more complex and we need to test its content.
When finding a job seeker with their email address, and if the NIR was
filled in the previous step, a sentence is added to the "User found"
modal. This sentence is now better rendered (the final `.` was after a
newline, creating a space between the last word and the `.`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Ne doit pas figurer dans le journal des changements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants