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

Test coverage missing for roundingMode: 'halfExpand' in since for negative numbers with 0.5 remainders #1111

Closed
justingrant opened this issue Nov 3, 2020 · 10 comments
Labels
Milestone

Comments

@justingrant
Copy link
Collaborator

justingrant commented Nov 3, 2020

Looks like rounding using the 'nearest' mode isn't doing away-from-zero rounding for negative 0.5 remainders. It seems to be rounding towards zero, i.e. same behavior as Math.round() which isn't correct per the proposal. From #827:

5.4.4 'nearest' - round to nearest integer, with 0.5 rounded away from zero.

Repro:

Temporal.Instant.fromEpochMilliseconds(0).subtract({milliseconds: 500}).round({smallestUnit: 'seconds'}).toString()
// => "1970-01-01T00:00:00Z" - OK (rounding towards beginning of time)

Temporal.Duration.from('-PT0.5S').round({smallestUnit: 'seconds'}).toString()
// => "-PT1S" - OK (rounds away from zero)

Temporal.DateTime.from('2020-01-01').subtract({milliseconds: 500}).since('2020-01-01', {smallestUnit: 'seconds'}).toString()
// actual: "PT0S"
// expected: "-PT1S"
@ptomato
Copy link
Collaborator

ptomato commented Nov 3, 2020

The existing behaviour seems correct to me. For non-Duration types we are treating "zero" as −∞ for the purposes of rounding (the principle of "zero is the Big Bang, not the Unix epoch", also from #827), effectively considering non-Duration types always positive. and that's why we treat floor and trunc as identical for non-Duration types. Given that, it seems consistent to round ties upwards in nearest mode.

@justingrant justingrant changed the title roundingMode: 'nearest' rounding incorrectly for negative numbers with 0.5 remainders roundingMode: 'nearest' in since is rounding incorrectly for negative numbers with 0.5 remainders Nov 3, 2020
@justingrant
Copy link
Collaborator Author

justingrant commented Nov 3, 2020

OK, makes sense re: Instant. I updated the OP to show a repro with DateTime.prototype.since that's still failing, though.

@ptomato
Copy link
Collaborator

ptomato commented Nov 3, 2020

Still seems correct to me, I believe we made "zero" == "the beginning of time" for all of the non-Duration types?

@justingrant
Copy link
Collaborator Author

justingrant commented Nov 3, 2020

Yep, but this is a Duration being rounded, not a DateTime. Duration rounding should round away from zero for negative durations.

@justingrant
Copy link
Collaborator Author

BTW, should I mark these failing tests with skip() in the PR for #993 if a fix for this issue doesn't land before my PR does? Or should I leave them as failing and they'll get fixed when the rounding fix comes in?

@ptomato
Copy link
Collaborator

ptomato commented Nov 4, 2020

skip would be better, with a comment pointing to this issue.

@justingrant
Copy link
Collaborator Author

It turned out that the failed tests in the #993 fix were my fault and were unrelated to this rounding problem, so I didn't need to skip any tests.

@justingrant
Copy link
Collaborator Author

Haha, looks like the problem doesn't repro anymore after #1023 and was merged. I'm not sure if that PR or @ptomato's earlier refactor of DateTime's add/subtract fixed it, but I'm OK to consider this fixed.

I'll leave this issue open in case someone wants to add a test case to handle this case because it was presumably a test hole before.

@ptomato ptomato added the non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! label Nov 6, 2020
@ptomato ptomato added this to the Stage 3 milestone Nov 6, 2020
@ptomato ptomato changed the title roundingMode: 'nearest' in since is rounding incorrectly for negative numbers with 0.5 remainders Test coverage missing for roundingMode: 'nearest' in since for negative numbers with 0.5 remainders Nov 30, 2020
@ptomato
Copy link
Collaborator

ptomato commented Jan 14, 2021

This is a polyfill or test262 thing and not a requirement for Stage 3.

@ptomato ptomato modified the milestones: Stage 3, Next Jan 14, 2021
@ptomato ptomato added test262 and removed non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! labels Jul 3, 2021
@Ms2ger Ms2ger changed the title Test coverage missing for roundingMode: 'nearest' in since for negative numbers with 0.5 remainders Test coverage missing for roundingMode: 'halfExpand' in since for negative numbers with 0.5 remainders Mar 10, 2022
@ptomato
Copy link
Collaborator

ptomato commented Jan 2, 2023

I'll move this to tc39/test262#3002.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants