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

Temporal: Tests for Duration rounding fix & consolidation #4041

Merged
merged 5 commits into from
May 14, 2024

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Apr 2, 2024

This PR contains test coverage for tc39/proposal-temporal#2758 which was a normative change that reached consensus at the February 2024 TC39 meeting.

Note that we still expect some review comments from implementors on that PR, so these tests are currently marked as a draft.

@ptomato ptomato requested review from a team as code owners April 2, 2024 23:57
@ptomato ptomato marked this pull request as draft April 2, 2024 23:58
@ptomato ptomato force-pushed the temporal-2742 branch 3 times, most recently from 12a9cd8 to ad07213 Compare May 2, 2024 17:57
@ptomato ptomato marked this pull request as ready for review May 7, 2024 22:55
@ptomato
Copy link
Contributor Author

ptomato commented May 7, 2024

This is ready now.

@ptomato ptomato changed the title Draft: Temporal: Tests for Duration rounding fix & consolidation Temporal: Tests for Duration rounding fix & consolidation May 8, 2024
@ptomato
Copy link
Contributor Author

ptomato commented May 8, 2024

The few test files that are actually relevant to review, not just trivial adjustments, are:

  • */dst-balancing-result.js
  • */dst-rounding-result.js
  • round/largestunit-smallestunit-combinations.js
  • round/largestunit-smallestunit-combinations-relativeto.js
  • total/dst-day-length.js
  • total/no-dst-day-length.js
  • total/total-of-each-unit.js
  • total-of-each-unit-relativeto.js
  • */roundingincrement-addition-out-of-range.js (added)

And note the change to weeks rounding in the various roundingmode-*.js tests. See tc39/proposal-temporal#2728 for discussion on that topic.

A MoveRelativeZonedDateTime step was missing, causing incorrect results.
See tc39/proposal-temporal#2742
ptomato added 4 commits May 13, 2024 16:12
This should produce all the same results (except for a change to weeks
balancing in round(), which is now more consistent with since()/until())
but leads to different observable user code calls.

See tc39/proposal-temporal#2742
Including tests for every possible combination of largest and smallest
unit, for each type of relativeTo (undefined, PlainDate, ZonedDateTime).
In ZonedDateTime.p.since/until, it's possible for AddDateTime to hit the
limit if the rounding increment is very high, even if the resulting
rounded duration isn't outside of the limit. Add a test covering this
case.
…ding

This covers an edge case that we hit, where 24 hours would not balance up
to one day in a 25-hour day if only largestUnit was specified, but would
erroneously balance up if rounding was also performed by specifying
smallestUnit.
@Ms2ger Ms2ger merged commit a7e796a into tc39:main May 14, 2024
8 checks passed
@ptomato ptomato deleted the temporal-2742 branch May 14, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants