[TECH] Suppression du wrapper sur le visit de ember/test-helpers.
#4114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lisequesnel
added
👀 Func Review Needed
👀 Tech Review Needed
cross-team
lisequesnel
force-pushed
the
tech-try-to-remove-transition-aborted
branch
from
192e998 to
a5c1b83
Compare
|
I'm deploying this PR to these urls:
Please check it out! |
jonathanperret
approved these changes
Feb 17, 2022
There was a problem hiding this comment.
Bien joué d'avoir réussi à isoler les transitions des méthodes asynchrones !
Bon tout ça paraît quand même assez fragile, si quelqu'un ajoute le mot-clé async sur le afterModel on aura un test qui échoue mais pour une raison assez mystérieuse. J'ai mis une suggestion qui irait je pense dans le sens d'un peu mieux documenter pourquoi on fait comme ça.
Après, globalement cette route est beaucoup trop complexe et semble notamment contenir la moitié d'une implémentation OIDC, mais c'est un autre sujet bien sûr…
nlepage
force-pushed
the
tech-try-to-remove-transition-aborted
branch
from
e9f10c7 to
f3314b0
Compare
nlepage
approved these changes
Feb 21, 2022
nlepage
force-pushed
the
tech-try-to-remove-transition-aborted
branch
from
f3314b0 to
e9f10c7
Compare
lisequesnel
force-pushed
the
tech-try-to-remove-transition-aborted
branch
from
e9f10c7 to
c0eb3e6
Compare
|
bon, j'ai un comportement bizarre en faisant la revue fonctionnelle :
|
lisequesnel
added
Rework needed after Func Review
and removed
👀 Func Review Needed
labels
nlepage
force-pushed
the
tech-try-to-remove-transition-aborted
branch
from
c0eb3e6 to
115ef7b
Compare
lisequesnel
force-pushed
the
tech-try-to-remove-transition-aborted
branch
from
115ef7b to
41489da
Compare
lisequesnel
added
👀 Func Review Needed
and removed
Rework needed after Func Review
labels
corrigé, et la revue fonctionnelle est maintenant validée 🎉 |
lisequesnel
added
Func Review OK
pix-service-auto-merge
force-pushed
the
tech-try-to-remove-transition-aborted
branch
from
41489da to
a463b32
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🦄 Problème
Il a quelques temps, nous avons wrappé le
visitde ember/test-helpers, car énormément de tests échouaient à cause d'une transition avortée (transition aborted) - cf l'issue toujours non résolue à ce jour. Or ce wrapper nous empêche d'utiliser testing-library sur Pix App.🤖 Solution
Supprimer le wrapper et corriger les tests cassants. Bonus : un seul test cassait véritablement désormais 🎉
🌈 Remarques
Il reste des
transition aborteddans l'application, il faudrait également les supprimer.💯 Pour tester
Non régression sur le process Pôle emploi.