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

durationformat.format coverage #3510

Closed

Conversation

romulocintra
Copy link
Member

@romulocintra romulocintra commented May 2, 2022

Incremental coverage for DurationFormat.format

cc @FrankYFTang @ryzokuken @sffc

@romulocintra romulocintra marked this pull request as ready for review May 2, 2022 13:22
"PT1H50M": {
"long": "1 hour and 50 minutes",
"short": "1 hr, 50 min",
"narrow": "1h 50m",
Copy link
Member Author

@romulocintra romulocintra May 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if hr or hr. and same with min vs min.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we normally put the localized strings in the Test262 suite? Adding and removing . is a common thing that changes across CLDR updates

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some cases at relative time format :

https://github.com/tc39/test262/blob/main/test/intl402/RelativeTimeFormat/prototype/format/en-us-numeric-auto.js#L29

I'm happy to only check the resolvedOptions of those methods instead avoiding depending on CLDR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was asking about the general policy of Test262 and CLDR-dependent output. I think it's useful to have at least a few tests for the expected output of the format function, but we just need to be aware that these tests are data-dependent, and the expectations could drift.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what the locale key in the front matter is for

@romulocintra romulocintra requested review from ptomato and Ms2ger May 2, 2022 16:46
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

"narrow": "2h 46m 40s",
"digital": "2:46:40",
},
// Negative durations in Intl.DurationFormat https://github.com/tc39/proposal-intl-duration-format/issues/29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that these tests will be added later? From my reading of the issue, it says that tests should verify that the output is different between positive and negative. (Or should that be a separate test that isn't dependent on locale?)

In any case, I think the tests will currently fail as written because stylelist includes the keys that are missing from these objects? Maybe it's better to have something like

if (!(style in styletestData))
  continue;

Comment on lines 16 to 20
assert.throws(TypeError, () => { df.format(undefined) });
assert.throws(TypeError, () => { df.format(-12) });
assert.throws(TypeError, () => { df.format(-12n) });
assert.throws(TypeError, () => { df.format(1) });
assert.throws(TypeError, () => { df.format(2n) });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming tc39/proposal-intl-duration-format#92 gets merged, these will be RangeErrors because they will convert to a string which is not a valid ISO 8601 duration string.

I'd also suggest testing things like {}, null, true, and "bad string".

romulocintra and others added 2 commits June 29, 2022 10:16
Co-authored-by: Philip Chimento <philip.chimento@gmail.com>
@romulocintra romulocintra marked this pull request as draft June 29, 2022 08:56
@romulocintra romulocintra force-pushed the duration-format-format-coverage branch from 3bcb80c to 8c0c8a7 Compare June 29, 2022 15:57
@romulocintra romulocintra force-pushed the duration-format-format-coverage branch from 8c0c8a7 to 90f8254 Compare June 29, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants