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

Extend datemath tests to cover 'gregory' calendar #1768

Closed
ptomato opened this issue Aug 24, 2021 · 5 comments · Fixed by #1761
Closed

Extend datemath tests to cover 'gregory' calendar #1768

ptomato opened this issue Aug 24, 2021 · 5 comments · Fixed by #1761

Comments

@ptomato
Copy link
Collaborator

ptomato commented Aug 24, 2021

In order to avoid bugs like #1761 we should extend the tests in datemath.mjs to cover at least the gregory calendar (chosen because it's a non-ISO calendar but can be easily debugged because the results should be the same as the ISO calendar.)

I've done this in a branch but there is at least one bug with dateUntil: https://github.com/tc39/proposal-temporal/tree/test-non-iso-datemath

one = Temporal.PlainDate.from('2020-02-25').withCalendar('gregory');
two = Temporal.PlainDate.from('2020-01-05').withCalendar('gregory');
one.since(two, { largestUnit: 'years' });

This should return P1M20D (and does so if you omit the withCalendar) but it currently throws.

When converting the tests to test262, we should ensure that the iso8601 tests are added to built-ins/Temporal/Calendar but the gregory tests are added to intl402/Temporal/Calendar in test262.

@justingrant
Copy link
Collaborator

Reproed. Thanks for catching this! Should be straightforward to expand the demitasse tests. I've only minimally touched the 262 tests but I'll take a look.

I think I just found another bug while debugging this. Looks like weeks are being returned by non-ISO dateUntil, when AFAIK expected behavior is that either weeks or days is returned, but not both unless largestUnit is weeks and smallestUnit is days. Is this assumption correct? I remember we spent a long time on this algorithm last year but wanted to double-check I understand desired behavior correctly.

@justingrant
Copy link
Collaborator

One more question: does code outside Calendar guarantee that Calendar.p.dateUntil is always called with two > one? I remember we tried to simplify implementation for the calendar author in a few places, but I don't remember if this was one of the simplifications we added. I'm asking because the cause of the bug you reported above was the non-ISO implementation assuming that two > one. I've fixed the non-ISO implementation but was wondering if the actual fix is actually needed upstream.

@justingrant
Copy link
Collaborator

Adding calendars to datemath turned out to be pretty easy for the demitasse tests, and there was only one test failure (the repro you found) so I'll just include the the fix in #1761 as well as extending demitasse tests to use gregory too. Great idea to use gregory, BTW.

I'll also fix the bug noted above where weeks were returned when largestUnit was 'months or 'years'.

I didn't know how to add gregory to the 262 tests so I'll skip that part for now, if that's OK. I'll open a new issue to cover the equivalent of this issue, but for Test262.

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Aug 25, 2021
Fixes tc39#1768 which was caused by assuming that `two` was always later
than `one` when `largestUnit` was `'years'` or `'months'`.

Also included `calendar: 'gregory'` in datemath demitasse tests.
@justingrant
Copy link
Collaborator

justingrant commented Aug 25, 2021

Fixes to the repro above and demitasse tests are in #1761. Future gregory test coverage is now tracked by #1776. I answered my own questions from above, if you want to skip reading comments.

does code outside Calendar guarantee that Calendar.p.dateUntil is always called with two > one? I remember we tried to simplify implementation for the calendar author in a few places, but I don't remember if this was one of the simplifications we added.

In the polyfill the answer is "no". I assume the same is true in the spec but haven't confirmed.

expected behavior is that either weeks or days is returned, but not both unless largestUnit is weeks and smallestUnit is days. Is this assumption correct?

Yes. Weeks is only returned if 'weeks' is largestUnit and/or smallestUnit. Otherwise it's not.

@ptomato
Copy link
Collaborator Author

ptomato commented Aug 25, 2021

Thanks! (The "When converting the tests to test262..." part was meant more as a note to myself, sorry didn't mean to try to put that task onto you.)

I think we removed all of the two > one guarantees when we added negative durations.

ptomato pushed a commit that referenced this issue Aug 25, 2021
Fixes #1768 which was caused by assuming that `two` was always later
than `one` when `largestUnit` was `'years'` or `'months'`.

Also included `calendar: 'gregory'` in datemath demitasse tests.
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 a pull request may close this issue.

2 participants