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

feat: #2386 - added tests about localization parameters #2412

Merged
merged 7 commits into from
Jun 30, 2022

Conversation

monsieurtanuki
Copy link
Contributor

Impacted file:

  • plural_translation_test.dart: added tests about localization parameters; simplified

What

  • Tests were added for all labels that currently need parameters.
  • If new labels with parameters appear, they need to be added there too.
  • Good news: the tests currently fail, for 6 languages.

Fixes bug(s)

Impacted file:
* `plural_translation_test.dart`: added tests about localization parameters; simplified
@teolemon
Copy link
Member

Not sure how we fail fil while having 0% translations: https://crowdin.com/project/openfoodfacts/fil

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: contains '2080706050'
Actual: 'One product'
When the exception was thrown, this was the stack:
#4 main.. (file:///home/runner/work/smooth-app/smooth-app/packages/smooth_app/test/plural_translation_test.dart:214:11)
#5 main.. (file:///home/runner/work/smooth-app/smooth-app/packages/smooth_app/test/plural_translation_test.dart:18:42)
#6 testWidgets.. (package:flutter_test/src/widget_tester.dart:170:29)


(elided one frame from package:stack_trace)
This was caught by the test expectation on the following line:
file:///home/runner/work/smooth-app/smooth-app/packages/smooth_app/test/plural_translation_test.dart line 214
The test description was:
plural test fil
════════════════════════════════════════════════════════════════════════════════════════════════════

… and tl

Impacted file:
* `plural_translation_test.dart`
@monsieurtanuki
Copy link
Contributor Author

Not sure how we fail fil while having 0% translations: https://crowdin.com/project/openfoodfacts/fil

I didn't understand either, I did double-check, and at least for this label it's exactly the same as in English.
There are 3 languages that fail for user_list_length.

After investigations, it's because there's a specific way to deal with plurals in fil, lv and tl.
cf. https://github.com/dart-lang/intl/blob/master/lib/src/plural_rules.dart#L132-L139

/// The integer part of [_n]
int _i = 0;
/// Number of visible fraction digits.
int _v = 0;
/// The visible fraction digits in n, with trailing zeros.
int _f = 0;

PluralCase _fil_rule() {
  if (_v == 0 && (_i == 1 || _i == 2 || _i == 3) ||
      _v == 0 && _i % 10 != 4 && _i % 10 != 6 && _i % 10 != 9 ||
      _v != 0 && _f % 10 != 4 && _f % 10 != 6 && _f % 10 != 9) {
    return ONE;
  }
  return OTHER;
}

And my crazy int was 2080706050 (_v == 0 && _i % 10 != 4 && _i % 10 != 6 && _i % 10 != 9 is true, therefore it's like 1).
I've changed the value in my PR, and it works! I mean, other values still fail, but for good reasons!

@teolemon
Copy link
Member

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: contains 'の中ழ்'
Actual: 'Open Food Facts でこの製品を見る:'

(ja)

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: contains 'の中ழ்'
Actual: 'процентне співвідношення'

(uk)

id
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: contains 'の中ழ்'
Actual: 'Selanjutnya { source_name}'

@codecov-commenter
Copy link

Codecov Report

Merging #2412 (d045569) into develop (2ea0da3) will decrease coverage by 1.33%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2412      +/-   ##
==========================================
- Coverage     8.86%   7.52%   -1.34%     
==========================================
  Files          161     204      +43     
  Lines         6623    9769    +3146     
==========================================
+ Hits           587     735     +148     
- Misses        6036    9034    +2998     
Impacted Files Coverage Δ
...kages/smooth_app/lib/widgets/attribute_button.dart 0.00% <0.00%> (-92.00%) ⬇️
...s/smooth_app/lib/data_models/user_preferences.dart 8.73% <0.00%> (-23.57%) ⬇️
packages/smooth_app/lib/themes/smooth_theme.dart 60.00% <0.00%> (-22.98%) ⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 15.29% <0.00%> (-18.92%) ⬇️
...mooth_app/lib/data_models/product_preferences.dart 24.65% <0.00%> (-6.78%) ⬇️
packages/smooth_app/lib/main.dart 14.40% <0.00%> (-3.49%) ⬇️
.../smooth_app/lib/pages/onboarding/welcome_page.dart 0.00% <0.00%> (-3.13%) ⬇️
.../smooth_app/lib/pages/onboarding/scan_example.dart 0.00% <0.00%> (-2.28%) ⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (-1.57%) ⬇️
...smooth_app/lib/pages/product/new_product_page.dart 0.00% <0.00%> (-0.88%) ⬇️
... and 204 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4aa3671...d045569. Read the comment docs.

@monsieurtanuki
Copy link
Contributor Author

@teolemon For the record, you fixed pct_match in uk as "".
Of course it failed, because we expect to see the percentage, like in English: "{percent}% match".
That's even the purpose of this PR.
You were testing me, fair enough ;)
In general, a no brainer fix would be to put the English translation.
Feel free to approve this PR.

);
expect(
appLocalizations.contact_form_body_android(
crazyInt, '', '', '', '', ''),
Copy link
Member

Choose a reason for hiding this comment

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

This and the following with empfehlen strings doesn't necessarily assure that all of the variables are still valid in all languages. We should check if crazyInt is in it 6 times if there is a check for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@M123-dev I don't agree with you: I test if each parameter is displayed. Good enough is good enough.
What you suggest would mean that in some cases, the translation tool mistakes a parameter for another, which is very unlikely.
Anyway, that test set is a start, and is open to evolution.

@monsieurtanuki monsieurtanuki merged commit 16d615d into openfoodfacts:develop Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add a test that placeholder are preserved in translations
4 participants