tests: fix i18n-test bugs in Node 13 #10595
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brendankenny
requested review from
paulirish
and removed request for
connorjclark
|
R: @paulirish since he was also looking at these failures |
brendankenny
commented
Apr 16, 2020
| // user-facing strings and it will eventually correct itself. | ||
| const formattedNaNStr = i18n.getFormatted(helloNaNStr, 'en-US'); | ||
| expect(formattedNaNStr === 'Hello NaN World' || formattedNaNStr === 'Hello -NaN World') | ||
| .toBe(true); |
There was a problem hiding this comment.
originally I had this as a conditional
if (({a: NaN}).a.toLocaleString() === '-NaN') {
expect(helloNaNStr).toBeDisplayString('Hello -NaN World');
} else {
expect(helloNaNStr).toBeDisplayString('Hello NaN World');
}but the conditional was always going to the else clause and then failing on the Hello NaN World assertion, so I gave up on figuring that out :) I assume some codegen thing.
paulirish
approved these changes
Apr 16, 2020
| @@ -11,27 +11,27 @@ const lhr = require('../../results/sample_v2.json'); | |||
|
|
|||
| /* eslint-env jest */ | |||
| describe('swap-locale', () => { | |||
| it('can change golden LHR english strings into spanish', () => { | |||
| it('can change golden LHR english strings into german', () => { | |||
Co-Authored-By: Paul Irish <paulirish@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
helps check off some of #10582
Two test issues here:
Node 13 has a
Intl.NumberFormatbug where aNaNthat was stored in a property on an object will be formatted as-NaN, e.g.({a: NaN}).a.toLocaleString() === '-NaN'. This also happens in V8 7.9/8.0 (~Chrome 79 and 80) but works correctly before and after these versions.Formatting
NaNs isn't exactly important to our report, we added the test to exercise the corner cases and make sure they wouldn't cause a crash, so this PR adds a narrow workaround to the test that can be removed later. Try not to putNaNs in the report or they'll be negative for Node 13 users :)The thousands separator for Spanish was removed starting in Node 13, causing the swap-locale formatted-number test to fail. Looking for NaN bugs I ran across this crbug, where it turns out that "Linguist experts in CLDR require two digits before the group separator for Spanish" (so a number has to be ≥ 10000 before getting a separator) which was fixed in V8 by an ICU update at some point.
Unfortunately we don't have any 5+ digit formatted number in
sample_v2.jsonto get the separator back, so this just switches the test to swap the locale tode, which does have 1 as itsminimumGroupingDigits.