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

chore: user authentication page testing #3233

Merged
merged 14 commits into from
Nov 2, 2022

Conversation

abughalib
Copy link
Contributor

@abughalib abughalib commented Oct 29, 2022

What

Golden test for Authentication Pages.

Comments

  • tooltip message localization
  • Sign Up Page
  • Login Page
  • Forgot Password Page

Part of

#1176

@abughalib abughalib requested a review from a team as a code owner October 29, 2022 17:36
@github-actions github-actions bot added 🧪 Tests 👥 User management Account login, signup, signout labels Oct 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2022

Codecov Report

Merging #3233 (e2222b6) into develop (a3608be) will increase coverage by 2.39%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #3233      +/-   ##
===========================================
+ Coverage     7.69%   10.08%   +2.39%     
===========================================
  Files          249      249              
  Lines        12248    12251       +3     
===========================================
+ Hits           942     1236     +294     
+ Misses       11306    11015     -291     
Impacted Files Coverage Δ
...ib/pages/preferences/user_preferences_account.dart 20.97% <ø> (ø)
...ib/generic_lib/widgets/smooth_text_form_field.dart 87.17% <100.00%> (+87.17%) ⬆️
...th_app/lib/pages/user_management/sign_up_page.dart 52.96% <100.00%> (+52.50%) ⬆️
...ckages/smooth_app/lib/widgets/smooth_scaffold.dart 67.79% <0.00%> (+23.72%) ⬆️
...ib/pages/user_management/forgot_password_page.dart 67.10% <0.00%> (+65.78%) ⬆️
...ooth_app/lib/pages/user_management/login_page.dart 67.21% <0.00%> (+66.39%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @abughalib!
Please have a look at my comments.
Beyond that, I cannot picture the Tooltip(child: InkWell...) combination: who wins if you tap?

@abughalib
Copy link
Contributor Author

@monsieurtanuki
I had to add a label to pass labeledTapTargetGuideline or I was getting Expected: Tappable widgets should have a semantic label on golden tests.
I had two options either ignore that guideline or add a label to every onTap widget.
If you got any other idea, I would implement that.

Copy link
Member

@AshAman999 AshAman999 left a comment

Choose a reason for hiding this comment

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

Well just some more suggestions besides what was already suggested

Also, the pages are not entirely rendered like the bottom screen is cropped,
perhaps you can also fix that. I remember there were some scroll features in the testing library as well

@monsieurtanuki
Copy link
Contributor

I had to add a label to pass labeledTapTargetGuideline or I was getting Expected: Tappable widgets should have a semantic label on golden tests.
I had two options either ignore that guideline or add a label to every onTap widget.
If you got any other idea, I would implement that.

What about Semantics(label: 'xxx', child: InkWell(...))? Or better, putting directly a semanticLabel, e.g. to Icon or TextSpan?

@abughalib
Copy link
Contributor Author

Well just some more suggestions besides what was already suggested

Also, the pages are not entirely rendered like the bottom screen is cropped, perhaps you can also fix that. I remember there were some scroll features in the testing library as well

How to generate golden files for that?
I used flutter test --update-goldens

Copy link
Member

@AshAman999 AshAman999 left a comment

Choose a reason for hiding this comment

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

Looks okay to me

@AshAman999 AshAman999 requested a review from M123-dev November 1, 2022 04:53
Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Heyy @abughalib looks great, just one little comment

Copy link
Member

@AshAman999 AshAman999 left a comment

Choose a reason for hiding this comment

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

LGTM as well

@M123-dev M123-dev merged commit 45aa97f into openfoodfacts:develop Nov 2, 2022
@M123-dev M123-dev added this to the v4 milestone Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants