-
Notifications
You must be signed in to change notification settings - Fork 464
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
Update Intl.DurationFormat tests #3983
Conversation
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.
For future reference, I'm not at all a fan of the approach of essentially putting a polyfill into the testIntl
helper file and comparing the results from the polyfill with the results from the implementation. It makes these tests opaque, difficult to understand what's being tested, and unclear whether the polyfill or the implementation is at fault when the tests fail.
However, the tests were already set up that way, so in the absence of clear recommendations about what to do otherwise (#3786) I would say that changing it is out of scope for this PR.
Nonetheless I would suggest that numeric-hour-with-zero-minutes-and-non-zero-seconds.js was clearer using NumberFormat and ListFormat directly, as it previously did, so maybe we should drop the commit ' Use formatDurationFormatPattern for "/numeric-hour-with-zero-minutes-and-non-zero-seconds"'.
Could @ryzokuken, @romulocintra, or @ben-allen take a look at the polyfill changes please?
This isn't actually possible, because of tc39/proposal-intl-duration-format#180. Before that PR, the test expected the string |
With regard to the "numeric-hour-with-zero-minutes-and-non-zero-seconds" question: in recent tests involving digital-like styles (see: #4034) I've been handling them by stitching together the unit values with ":" as a hard-coded separator. My theory is that this is slightly better than using a fully hard-coded string, since:
|
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.
oh my goodness, thank you for catching this. I'm actively embarassed...
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.
It's all great -- thank you for all of your work here.
Thoughts in general:
- I'm wondering if it's worthwhile to have a TG2 discussion on the behaviour when seconds (or the relevant subsecond unit) has its display option set to
"auto"
and has a non-zero fractional component that's nevertheless not displayed due to use of thefractionalDigits
option. I can see arguments for changing this behaviour, but also arguments for leaving it as-is - I share @ptomato's skepticism about the use of the
partitionDurationFormatPattern
quasi-polyfill. That said, given that these tests are great and given that DurationFormat is nearly done, I'm wondering if, in the interest of testing locales other than"en"
, it would be useful for me to extendpartitionDurationFormatPattern
to handle hour/minute/second separators other than ":", decimal separators other than ".", and numbering systems other thanlatn
.
Sync `partitionDurationFormatPattern` with the latest spec draft and change it to use an `Intl.DurationFormat` object as the input, so it's easier to compare it against the spec text and because it allows to test more inputs. Includes the fixes for: - tc39/proposal-intl-duration-format#183 - tc39/proposal-intl-duration-format#184
…and-non-zero-seconds" The changes from the first commit allow to use `formatDurationFormatPattern` for this test.
…fault display of sub-hours units Numeric "minute" and "seconds" units now default to "always" display, so we have to add an additional test to cover when "auto" display is used. Additionally add more inputs to cover all possible test combinations.
Update
Intl.DurationFormat
tests to match the current draft spec and add tests to cover: