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: normalisation de l'HTML des snapshots #5169

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xavfernandez
Copy link
Contributor

🤔 Pourquoi ?

C'est une proposition, suite à une réflexion que je me suis faite après avoir fait du réusinage (cf #5145 & #5160 qui ne changent normalement rien à l'HTML produit).

A peu de frais, on pourrait avoir des snapshots plus stable & plus facile à relire.

Il y a le soucis classique que comme prettify rajoute des sauts de ligne il peut changer la sémantique du HTML produit mais j'imagine que c'est assez rare pour nous ?

🍰 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 ?

🏝️ 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

@xavfernandez xavfernandez added the no-changelog Ne doit pas figurer dans le journal des changements. label Nov 28, 2024
@xavfernandez xavfernandez self-assigned this Nov 28, 2024
@xavfernandez xavfernandez marked this pull request as draft November 28, 2024 13:34
@EwenKorr
Copy link
Contributor

Les snapshots sont plus lisibles, je suis plutôt pour ce changement !

@rsebille
Copy link
Contributor

Il y a le soucis classique que comme prettify rajoute des sauts de ligne il peut changer la sémantique du HTML produit mais j'imagine que c'est assez rare pour nous ?

Vu qu'on utilise pas le HTML produit dans les snapshots dans un navigateur je pense que c'est pas trop grave, et je doute qu'une relecture de snapshots ai déjà permis de choper ce genre d'erreur.
Par contre si ça permet de réduire le bruit lié au ligne vide ça me semble plutôt utile, et ça va aussi sans doute améliorer la lisibilité des attributs qui sont je pense plus importants que les lignes vides

@francoisfreitag
Copy link
Contributor

-0: Je préfère éviter le post-processing sur les valeurs attendues en général. Ça évite les problèmes de la lib qui passe derrière notre dos et normalise les caractères de la réponse à sa sauce, modifie légèrement le markup, etc. Le problème des espaces significatifs est réel, et utiliser le pretty print empêche d’écrire un test de régression.

Ne pas faire le post-processing évite aussi le travail assez coûteux pour pretty print les éléments. Ils indiquent:

If you pass in formatter=None, Beautiful Soup will not modify strings at all on output. This is the fastest option, but it may lead to Beautiful Soup generating invalid HTML/XML

J’en déduis que les autres options sont plus lentes (et donc coûteuses).

La lisibilité des snapshots est effectivement sympa, mais je doute qu’on les lise de manière attentive dans tous les cas, surtout s’il y a plus de 10 lignes. L’intérêt des snapshots pour moi est surtout le diff avec la version précédente, donc les espaces et indentations importent peu.

Copy link
Contributor

@tonial tonial left a comment

Choose a reason for hiding this comment

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

Ça m'a l'air cool pour éviter les mise à jour de templates à cause de l'indentation suite à l'ajout d'un if :)

On pourra toujours revenir en arrière si un jour on voit que ça pose problème

@celine-m-s
Copy link
Collaborator

Pour moi, le grand intérêt des snapshots est surtout le diff avec les versions précédentes. Le rendu actuel ne me dérange pas car je passais outre l'indentation. Je ne suis donc ni pour, ni contre. À voir si ça semble utile aux autres.
Cela étant, ça ajoute effectivement une étape de postprocessing et du code (donc de la maintenance) ainsi qu'une nouvelle étape à penser (importer pretty_indented et l'utiliser). Cela ne me semble pas indispensable aujourd'hui, mais si c'est utile pour le reste de l'équipe allons-y car je n'ai pas d'avis ferme sur le sujet.

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.

6 participants