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: Améliorer les tests du menu de navigation #5564

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Conversation

tonial
Copy link
Contributor

@tonial tonial commented Feb 7, 2025

🤔 Pourquoi ?

tester ave reverse() ne permet pas de mettre des vus qui prennent des paramètres dans l'url.

🍰 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

@tonial tonial added the no-changelog Ne doit pas figurer dans le journal des changements. label Feb 7, 2025
@tonial tonial self-assigned this Feb 7, 2025
@tonial tonial force-pushed the alaurent/nav_tests branch 2 times, most recently from 3999743 to 947449e Compare February 7, 2025 16:21
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.

Ça marche pour moi.

Pour info, https://github.com/gip-inclusion/les-emplois/pull/5017/files#diff-124930b6a5a16fd9482cd9757e37e3f7c88d1773cc278e60ce9e55692bcd3f9aR83 est une autre implémentation de la même chose qui évite les doublons.

@francoisfreitag
Copy link
Contributor

Il manque un peu de contexte, pourquoi est-ce que le test naïf pose problème maitenant ?

@tonial
Copy link
Contributor Author

tonial commented Feb 10, 2025

En effet, le context c'est #5567 : je voudrais que la page de détail soit aussi sans le nav-item GPS

@francoisfreitag
Copy link
Contributor

Sur l’histoire des doublons, j’ai fait casser avec l’assertion suivante :

diff --git a/tests/utils/test_templatetags.py b/tests/utils/test_templatetags.py
index a427af024..02bf6524c 100644
--- a/tests/utils/test_templatetags.py
+++ b/tests/utils/test_templatetags.py
@@ -104,6 +104,7 @@ class TestNav:
                         url_names.append(f"{namespace}:{name}")
                     else:
                         url_names.append(name)
+        assert sorted(url_names) == sorted(set(url_names))
         return url_names
 
     def test_active_view_names(self):

@tonial
Copy link
Contributor Author

tonial commented Feb 10, 2025

Du coup je reprends plutôt ton implementation ?

@francoisfreitag
Copy link
Contributor

En effet, le context c'est #5567 : je voudrais que la page de détail soit aussi sans le nav-item GPS

Ça mérite tout à fait sa place dans le message de commit 😁

@tonial tonial force-pushed the alaurent/nav_tests branch 3 times, most recently from 7c7bc0c to 1e0a305 Compare February 10, 2025 09:38
using reverse prevents us from adding views with parameters in
active_view_names, which is needed to add GPS views in the nav menu

the named_urls function was cherry-picked from #5017
@tonial tonial force-pushed the alaurent/nav_tests branch from 1e0a305 to 89b8c50 Compare February 10, 2025 09:39
@tonial
Copy link
Contributor Author

tonial commented Feb 10, 2025

Je t'ai mis en auteur du commit du coup, vu que c'est ton code qui est dedans ^^'

@francoisfreitag
Copy link
Contributor

Je ne tenais pas particulièrement à mon implémentation, mais l’unicité des noms de vues me paraît intéressante.

@tonial tonial enabled auto-merge February 10, 2025 09:44
@tonial
Copy link
Contributor Author

tonial commented Feb 10, 2025

Je trouve ton implémentation plus propre que la mienne :)

@tonial tonial added this pull request to the merge queue Feb 10, 2025
Merged via the queue into master with commit 2932139 Feb 10, 2025
11 checks passed
@tonial tonial deleted the alaurent/nav_tests branch February 10, 2025 09:59
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.

2 participants