-
-
Notifications
You must be signed in to change notification settings - Fork 406
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
test: match_ingredient_origin unit test #8174
Conversation
@@ -93,6 +93,9 @@ BEGIN { | |||
|
|||
&has_specific_ingredient_property | |||
|
|||
&init_origins_regexps | |||
&match_ingredient_origin | |||
|
|||
); # symbols to export on request | |||
%EXPORT_TAGS = (all => [@EXPORT_OK]); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, @alexgarel! The addition of init_origins_regexps
and match_ingredient_origin
functions seems like a good way to address the bug cited on the pro platform. However, for production readiness, it would be good to add some documentation to these new functions and also update the existing documentation to reflect these changes. Additionally, it would be helpful to include some unit tests for these new functions to ensure they are working as expected. Overall, great work!
Codecov Report
@@ Coverage Diff @@
## main #8174 +/- ##
==========================================
+ Coverage 46.86% 46.88% +0.02%
==========================================
Files 102 103 +1
Lines 20384 20396 +12
Branches 4646 4647 +1
==========================================
+ Hits 9552 9563 +11
Misses 9684 9684
- Partials 1148 1149 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@stephanegigandet I did this PR to show the bug mainly :-D, as in French it's not working as expected. I don't know if we should keep only one test or both, we can discuss it ! |
{ | ||
"lc" : "fr", | ||
"origin_fr" : "Sucre France." | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug, isn't it @stephanegigandet ?
We should have specific_ingredients
tests/unit/expected_test_results/parse_origins_from_text/real-list-fr.json
Show resolved
Hide resolved
tests/unit/match_ingredient_origin.t
Outdated
"noisettes" => "Italie", | ||
"fèves de cacao" => "Côte d’Ivoire", | ||
"Framboises lyophilisées" => "Espagne" . "arôme naturel de framboise" => "France", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's exactly this, but something like it. (right now it fails, of course)
@alex There were a few issues. One was the lacking support for different single quotes (’ vs ') I added support for "Ingredient Country" (e.g. "Sugar France"), but only if Sugar is a known ingredient. Is this ok for you? |
Kudos, SonarCloud Quality Gate passed! |
Very good @stephanegigandet ! But as I am the one who opened the PR you must approve it :-) |
Testing match_ingredient_origin as a way to understand bug cited on pro platform
part of: