Skip to content

Temporal: Add test for different combinations of DateTimeFormat options #4431

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

Merged
merged 5 commits into from
Apr 23, 2025

Conversation

catamorphism
Copy link
Contributor

This test file adds tests for all types of Temporal objects and various combinations of style options for Intl.DateTimeFormat.

@catamorphism catamorphism requested a review from a team as a code owner March 18, 2025 21:30
@catamorphism catamorphism force-pushed the temporal-objects-formatting branch from 13d614b to 28699e6 Compare March 18, 2025 21:31
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.

I have mixed feelings about this one. We are trying to reduce the number of tests that use "golden output" to test locale-dependent functions, because they tend to break anytime a browser updates its CLDR version. But, we do not yet have a policy of not accepting golden output tests.

So this could be merged as is, but it'd be really helpful if the assertions could be changed so that they assert more general properties of the output instead of the exact contents of the string. Would that be possible?

A few suggestions off the top of my head:

  • the date-only types could assert that the output of the dateStyle+timeStyle formatters was identical to the corresponding dateStyle formatter and that timeStyle was ignored.
  • For PlainDateTime you could use 2222-03-04T05:06:07.888999111 and assert that, for example, "2", "3", and "4" do not occur in any string output by a timeStyle formatter
  • For PlainYearMonth and PlainMonthDay you could create it with an unusual reference day or reference year respectively, and assert that it does not occur in any string output

(And above all, thanks for doing the work!)

More info:
#3786
http://ptomato.name/talks/tc39tg2-2024-10

/*---
esid: sec-datetime-format-functions
description: Different combinations of style options and Temporal types format correctly.
locale: [en]
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this'd need to be

Suggested change
locale: [en]
locale: [en-US]

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one got missed in the rewrite.

This test file adds tests for all types of Temporal objects and various
combinations of style options for Intl.DateTimeFormat.
@catamorphism catamorphism force-pushed the temporal-objects-formatting branch from ea0e022 to 27cfdd0 Compare April 22, 2025 22:47
@catamorphism
Copy link
Contributor Author

@ptomato I've tried to do what you suggested; I did keep the original tests as commented-out code, so it's easier to read the test and figure out what the expected output should (roughly) look like.

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, this looks exactly like I was hoping it'd look. Just some minor fixups

/*---
esid: sec-datetime-format-functions
description: Different combinations of style options and Temporal types format correctly.
locale: [en]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one got missed in the rewrite.

@catamorphism catamorphism enabled auto-merge (rebase) April 23, 2025 18:50
@catamorphism catamorphism merged commit 64237be into tc39:main Apr 23, 2025
11 checks passed
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.

2 participants