Add Color locale tests; migrate color lists from commerce.color to color.name#1715
Add Color locale tests; migrate color lists from commerce.color to color.name#1715asolntsev merged 4 commits intodatafaker-net:mainfrom
Conversation
…AM, KR, PT, RU, SE, TH, TR, CH, BR, DK.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1715 +/- ##
============================================
- Coverage 92.41% 92.32% -0.09%
+ Complexity 3438 3434 -4
============================================
Files 337 337
Lines 6779 6779
Branches 670 670
============================================
- Hits 6265 6259 -6
- Misses 352 354 +2
- Partials 162 166 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
asolntsev
left a comment
asolntsev
left a comment
There was a problem hiding this comment.
Nice job, thank you @vicky-iv !
P.S. I have been thinking about all these localeProviderListTest (like ColorLocaleTest in this case). What exactly do they test? What type of error can they catch?
They seems useless for me...
At least,
- instead of listing all locales manually, we can generate the locales list automatically.
- instead of listing alll yml keys like "color.name" manunally, we could generate this list automatically (e.g. using annotations).
|
@asolntsev Thanks for your comment!
Some locales don't have a list of colors, so I thought it would make sense to add locales manually.
Automatically generating the list sounds like a good idea, but it won’t work in all cases. In some tests we might need to cover more complex entities with multiple parameters. A good example is |
I added a PR to your branch generalizing this |
|
@asolntsev I slightly improved Thanks @snuyanzin for the idea! |
asolntsev
left a comment
asolntsev
left a comment
There was a problem hiding this comment.
All good, thank you for the PR.
My doubt is more general. I don't understand what exactly does this test verify?
What type of error can it potentially catch? For me, it seems a useless test.
Not only this one, but all the previous similar tests.
@Override
protected Stream<Arguments> localeProviderListTest() {
return fakers.stream()
.map(faker -> arguments(TestSpec.of(() -> faker.color().name(), "color.name"), faker));
}
|
We’ve been through that discussion before. |
|
... And what was the answer? |
|
It was a few things:
If you need more you'll have to go looking. I don't want to rehash it. |
|
@kingthorin Sorry, I should not waste your time by asking this. I will try to prepare PR if I get some idea how to improve these tests. |
|
No worries, sorry if my response seemed negative or harsh; things weren't going well yesterday. |
5 participants
I noticed an inconsistency in how color values are stored across different languages.
In some files, color was part of the
сommerceentity, but I couldn’t find any usages ofcommerce.colorin the code, and theCommerceclass doesn’t have acolor()method.Therefore, I propose to normalize the color data and always store color as a separate entity in the
color.nameformat.PR description
ColorLocaleTestto verifyColor.name()localization across many locales.BaseFakerLocaleTestwith additionalBaseFakerinstancescommerce.colortocolor.namefor several locales to normalize where the Color provider reads from and to remove duplication.be.yml,by.yml,da-DK.yml,ru.yml,sv.yml,uk.yml(addcolor.nameblocks; removecommerce.colorwhere applicable), anduz.yml(removecommerce.coloras duplicate).