test(date): Add Intl-Based Tests for Date Definitions to Ensure Completeness of Weekdays and Months #2912
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2912 +/- ##
==========================================
- Coverage 99.95% 99.95% -0.01%
==========================================
Files 2987 3051 +64
Lines 216052 217787 +1735
Branches 603 948 +345
==========================================
+ Hits 215952 217686 +1734
- Misses 100 101 +1
|
|
Is this PR ready for review yet? |
ST-DDT
added
p: 1-normal
|
@ST-DDT it('should use Intl.DateTimeFormat to get the month name in the correct locale', () => {
const date = new Date(2020, 0, 1);
const intlMonth = new Intl.DateTimeFormat(locale, {
month: 'long',
}).format(date);
expect(localizedFaker.definitions.date.month.wide).toContain(
intlMonth
);
});
it('should use Intl.DateTimeFormat to get the abbreviated month name in the correct locale', () => {
const date = new Date(2020, 0, 1);
const intlMonth = new Intl.DateTimeFormat(locale, {
month: 'short',
}).format(date);
expect(localizedFaker.definitions.date.month.abbr).toContain(
intlMonth
);
}); |
|
I'm currently on mobile and thus cannot run the tests myself. |
|
@ST-DDT AssertionError: expected [ 'Luni', 'Marți', 'Miercuri', 'Joi', 'Vineri', 'Sâmbătă', 'Duminică' ] to include 'sâmbătă' Failure factors can range from missing data to capitalization, dots, etc. describe.each(Object.entries(allLocales))(
'for locale %s',
(locale, fakerLocale) => {
let localizedFaker: Faker;
const weekdays = Array.from(
{ length: 7 },
(_, i) => new Date(2020, 0, i + 4)
); // January 4-10, 2020 are Sunday to Saturday
beforeAll(() => {
localizedFaker = new Faker({ locale: [fakerLocale, en, base] });
});
it(`should return random value from date.weekday.wide_context array for context option in ${locale}`, () => {
if (localizedFaker.definitions.date.month.wide_context) {
const weekday = localizedFaker.date.weekday({ context: true });
expect(
localizedFaker.definitions.date.weekday.wide_context
).toContain(weekday);
}
});
it(`should return random value from date.weekday.abbr_context array for abbreviated and context option in ${locale}`, () => {
if (localizedFaker.definitions.date.month.abbr_context) {
const weekday = localizedFaker.date.weekday({
abbreviated: true,
context: true,
});
expect(
localizedFaker.definitions.date.weekday.abbr_context
).toContain(weekday);
}
});
it('should use Intl.DateTimeFormat to get the weekday name in the correct locale', () => {
for (const date of weekdays) {
const intlWeekday = new Intl.DateTimeFormat(locale, {
weekday: 'long',
}).format(date);
expect(localizedFaker.definitions.date.weekday.wide).toContain(
intlWeekday
);
}
});
it('should use Intl.DateTimeFormat to get the abbreviated weekday name in the correct locale', () => {
for (const date of weekdays) {
const intlWeekday = new Intl.DateTimeFormat(locale, {
weekday: 'short',
}).format(date);
expect(localizedFaker.definitions.date.weekday.abbr).toContain(
intlWeekday
);
}
});
}
);
This is my current test code. |
|
Please note: We want the test to ensure that we have the correct values, currently the values might be invalid.
If I insert |
|
Could you please fix the invalid values and replace it with correct ones? Maybe using a onetime script like: for [locale] in allLocales {
const weekdayPath = resolve("src/locales/${locale}/date/weekday.ts");
const storedWeekdays = await import(weekdayPath) as DateDefinition["weekday"];
const long = [];
const abbr = [];
for (const i=0;i<7;i++) {
long.push(new Intl...);
abbr.push(new Intl....);
}
storedWeekdays.long = long;
storedWeekdays.abbr = abbr;
writeFileSync(weekdayPath, "export default "+JSON.toString(storedWeekdays));
}And run EDIT: Please post the script here, so that we can use it during the review. |
|
Please merge |
If you post the script here, we can later integrate it in our generate locale script, so that all locales always have the correct values. Or if you want to do that directly you can add it after faker/scripts/generate-locales.ts Lines 414 to 422 in 7dc8a18 |
|
@ST-DDT I am a novice developer, so please understand my embarrassing generator code. |
|
Rather than just overwriting the existing data, could we output a table showing where the data produced by Intl differs from what we already have in Faker? That would make it easier to see discrepancies, and loop in native speakers if needed to see why. |
|
@matthewmayer |
|
Here are the discrepancies i could find, for "wide" data (the abbr/short data has more variation), ignoring case-only differences and ignoring data where Intl doesnt seem to provide correct data eg dv and sr_RS_latin.
i think you really need a language speaker to check these manually. For example, in Chinese which I do know, both 星期天 and 星期日 are both perfectly valid words for "Sunday". In Vietnamese it looks like we are spelling out the month numbers versus using digits. In Uzbek there seems to be some debate about the correct transliteration for September and October - see https://daryo.uz/2018/01/15/sentabr-yoziladimi-yoki-sentyabr , perhaps @Mirazyzz can help clarify! |
Month names are derived from Russian, and therefore IMO the proper usage would be "sentYAbr" to reflect the Russian "сентЯбрь." Since there is no direct equivalent of the letter "Я" in our language, we use the combination of "Y" and "A." However, the debate on this matter is still ongoing, and no official approach has been decided yet. Interestingly, some official school books use both variations on different pages. Given this uncertainty, I suggest we choose the variation that minimizes our workload and simplifies our process. |
|
With a big thanks to @sosost for their work so far, but I'm starting to feel trying to do this as a script that generates everything at once is maybe a mistake. Instead perhaps we can modify it so it's run manually and you give it a single locale as a parameter, and it attempts to generate the days and months for that locale only. That way we don't need to code in lots of edge cases, and it's easy to run it for missing locales, and review the diff for one locale at a time. Once generated, it's not like the days of the week or months of the year will ever change ... |
…update locale format
…ate-Intl-based-tests-for-date-definitions
…ttps://github.com/sossost/faker into test/create-Intl-based-tests-for-date-definitions
|
Sorry, for the delay. I will tackle this once I have some free time. |
|
Ready for review. The locales can be more easily reviewed using: rm -rdf src/locales
git reset next -- src/locales
git restore src/locales
pnpm run preflight
git add src/locales |
There was a problem hiding this comment.
I dont think this should be part of preflight
As we learned in #2906 ICU data is not stable so we could easily have the situation where this generate different dates on different versions of node.
I think it'll be better to have this as a separate standalone script that can be run on demand to check the data and fill in missing locales, but any discrepancies in existing locales should be checked by a native speaker.
|
Added to the meeting notes for discussion. |
|
I tested with Node 20.0.0 and 22.2.0. There are differences in data for en-AU, eo, es-MX, mk, uk and vi. Mostly minor things like tweaks to the shortened versions, capitalization etc. But enough to cause havoc with people running preflight with different node versions. |
|
Team Notice We talked about it in the team meeting, but we haven't been able to decide anything yet. |
|
Team Decision
Thanks for your contribution anyway ❤️ |
#2904
This PR is about Intl-based tests for the date definitions in Faker.js. These tests ensure that the weekdays and months are correctly defined and returned across different locales, enhancing the accuracy and completeness of the data.