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

test: match_ingredient_origin unit test #8174

Merged
merged 4 commits into from
Mar 13, 2023

Conversation

alexgarel
Copy link
Member

Testing match_ingredient_origin as a way to understand bug cited on pro platform

part of:

@alexgarel alexgarel requested a review from a team as a code owner March 8, 2023 11:35
@@ -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]);
}
Copy link

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!

@github-actions github-actions bot added 🥗 Ingredients 🥗🔍 Ingredients analysis https://wiki.openfoodfacts.org/Ingredients_Extraction_and_Analysis 🧪 tests labels Mar 8, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #8174 (74c87ce) into main (2b5fb55) will increase coverage by 0.02%.
The diff coverage is 91.66%.

@@            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     
Impacted Files Coverage Δ
lib/ProductOpener/Ingredients.pm 90.66% <ø> (ø)
tests/unit/match_ingredient_origin.t 91.66% <91.66%> (ø)

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

@alexgarel
Copy link
Member Author

@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 !

Comment on lines 1 to 4
{
"lc" : "fr",
"origin_fr" : "Sucre France."
}
Copy link
Member Author

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

"noisettes" => "Italie",
"fèves de cacao" => "Côte d’Ivoire",
"Framboises lyophilisées" => "Espagne" . "arôme naturel de framboise" => "France",
},
Copy link
Member Author

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)

@stephanegigandet
Copy link
Contributor

@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?

@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@alexgarel
Copy link
Member Author

Very good @stephanegigandet !

But as I am the one who opened the PR you must approve it :-)

@stephanegigandet stephanegigandet merged commit aae0385 into main Mar 13, 2023
@stephanegigandet stephanegigandet deleted the test-match-ingredient-origin branch March 13, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🥗🔍 Ingredients analysis https://wiki.openfoodfacts.org/Ingredients_Extraction_and_Analysis 🥗 Ingredients 🧪 tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants