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

(PC-14143)[PRO] fix: SignupForm: fix warnings in test #1877

Closed

Conversation

rlecellier
Copy link
Contributor

Lien vers le ticket Jira : https://passculture.atlassian.net/browse/PC-14143

Afin de débuger un comportement différent dans le test et dans l'application (voir cette PR)
J'ai besoin de nettoyer les warning du test SignupForm:

$ yarn test:unit components/pages/Signup/SignupForm/__specs__/SignupForm.spec.jsx 2> test_output.txt
$ cat test_output.txt | grep Warning | wc -l
563
$ cat test_output.txt | grep console.error | wc -l
282

Cette PR les éliminent tous.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 31, 2022

Debt collector report:

Files with same debt :

File Prev Current trend
src/components/pages/Signup/SignupForm/SignupForm.jsx 26.25 26.25 =
src/components/pages/Signup/Si....orm/SirenField/SirenField.jsx 14 14 =
src/components/pages/Signup/Si....specs/SignupForm.spec.jsx 7 7 =

null

null

Previous debt : 47.25
Current debt : 47.25

Debt decreased by 0 Points

To get a file by file report, please run debt-collector check --changed-since="[REVISION]"

@rlecellier rlecellier force-pushed the rlecellier/PC-14143_SignupForm_spec_warnings branch from ca3778e to ed4158b Compare March 31, 2022 12:53
screen.getByRole('textbox', {
name: /Prénom/,
}),
'Prénom'
)
userEvent.type(
userEvent.paste(
Copy link
Contributor

@mariedestandau mariedestandau Mar 31, 2022

Choose a reason for hiding this comment

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

paste est destiné à coller depuis le presse-papier, sa signature a changé dans la dernière version de userEvent et son utilisation à la place de type empêche la mise à jour de la lib https://github.com/testing-library/user-event/releases/tag/v14.0.0

Il va falloir nettoyer toute la codebase, alors je ne suis pas très enthousiaste à l'idée d'en rajouter, d'autant que cela fonctionne avec input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nous avions décidé de ne plus utiliser userEvent a cause de type qui pouvait significativement rallonger la durée des tests.
L'utilisation de paste permet de tester un trigger unique de changement de valeurs.

Si on ne peu plus utiliser paste, nous pouvons repasser e fireEvent pour les input text.
Sinon, on doit pouvoir utiliser la nouvelle api de paste mise à disposition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

si je comprend bien il faudra remplacer

userEvent.paste(myInput, 'string')

par

userEvent.paste('myString')
userEvent.paste(myInput)

testing-library/user-event#785

Copy link
Contributor

Choose a reason for hiding this comment

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

non, en fait si tu veux coller un texte dans un champ c'est userEvent.type(myInput, 'string') le paste c'est vraiment comme si ton user faisait Pomme + V mais tu peux plus lui dire où coller
le lien avec la nouvelle doc est là, sorry, je pensais avoir collé celui-là https://testing-library.com/docs/user-event/clipboard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

paste(clipboardData?: DataTransfer|string): Promise<void>

When called without clipboardData, the content to be pasted is read from the Clipboard.

Il semble qu'on puisse garder notre utilisation actuel avec l'ajout de la clef clipboardData.

userEvent.paste(monElement, 'mon text')

devient

userEvent.paste(monElement, {
    clipboardData: {
      getData: () => 'mon text',
    })

J'ai vraiment peur que le typing nous trigger plein d'evenement dont nous n'avons pas besoin dans les test.

Copy link
Contributor

@mariedestandau mariedestandau Apr 14, 2022

Choose a reason for hiding this comment

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

Pour répondre à ta question dans slack hello, tu lis ce que j'ecris dans mes commentaire ?, oui je confirme que j'ai bien lu ton commentaire, mais je ne l'ai peut-être pas bien compris: pourquoi userEvent.type pose des problèmes de typing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nous pouvons en discuter par meet ou sur slack, je pense avoir été claire lorsque j'ai répondu à ta question la première fois ici.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je trouve des issu tel que celle ci : testing-library/user-event#577
Des chaînes d’effet sur des changements de valeurs dans des inputs peuvent allongé de façons significative la durée de nos test.
Nous avons décidé il y à un bon moment d'utilisé paste afin de ne trigger les effet qu'une seul fois lorsque nous n'avons pas besoin de tester un comportement "lettre par lettre".

Copy link
Contributor

@mariedestandau mariedestandau Apr 14, 2022

Choose a reason for hiding this comment

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

Tu me dis de relire ton 2ème commentaire, donc je comprends que c'est la durée des tests qui pose problème ?

Le userEvent.type en lui-même ne devrait pas trigger plein d'évènements, mais notre implémentation fait peut-être que c'est le cas, on a énormément de cycles de rendus, on retraite le contenu des champs à plusieurs niveaux (dans le champ lui-même + dans des décorateurs). A mon sens c'est notre implémentation qu'il faut corriger, ces cycles de rendu sont un problème en front, et si les tests nous alertent je suis davantage d'avis de régler le problème que de hacker les tests pour faire taire l'alerte.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mon point de vu est que si nous testons la réaction du changement de valeur une fois, autant limiter les boucle de code inutile.
Je suis par ailleurs tout à fait d'accord sur le fait de développer plus propre.

A nouveau, je ne considère pas github comme une plateforme de chat, et je t'invite à rouvrir le dialogue sur un moyen de communication plus adapté.

@rlecellier rlecellier force-pushed the rlecellier/PC-14143_SignupForm_spec_warnings branch from ed4158b to e22e6a9 Compare April 14, 2022 13:09
@rlecellier rlecellier closed this Oct 24, 2022
@dbaty dbaty deleted the rlecellier/PC-14143_SignupForm_spec_warnings branch March 27, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants