Fix nearest minute rounding #3267
Conversation
| { roundingMethod: 'ceil' } | ||
| ) | ||
| assert.deepStrictEqual(result, new Date(2014, 6 /* Jul */, 10, 12, 12, 0)) | ||
| assert.deepStrictEqual(result, new Date(2014, 6 /* Jul */, 10, 12, 11, 0)) |
There was a problem hiding this comment.
The original expectation rounds the time up from 12:10:30:5, to 12:12:00:000 - a false assertion i guess
| { roundingMethod: 'floor' } | ||
| ) | ||
| assert.deepStrictEqual(result, new Date(2014, 6 /* Jul */, 10, 12, 11, 0)) | ||
| assert.deepStrictEqual(result, new Date(2014, 6 /* Jul */, 10, 12, 10, 0)) |
There was a problem hiding this comment.
Incorrect assertion if i pass in a rounding method 'floor' i expect to round down from 12:10:30 to 12:10:00 and not 12:11:00
src/roundToNearestMinutes/test.ts
Outdated
| const result = roundToNearestMinutes( | ||
| new Date(2014, 6 /* Jul */, 10, 12, 10, 30), | ||
| { nearestTo: 4 } | ||
| { nearestTo: 4, roundingMethod: 'round' } |
There was a problem hiding this comment.
the assertion in the test is based on 'normal' rounding behaviour, while trunc is date-fns' default.
There was a problem hiding this comment.
other option is to update the assertion
src/roundToNearestMinutes/test.ts
Outdated
| const result = roundToNearestMinutes( | ||
| new Date(2014, 6 /* Jul */, 10, 12, 13, 30) | ||
| new Date(2014, 6 /* Jul */, 10, 12, 13, 30), | ||
| { roundingMethod: 'round' } |
There was a problem hiding this comment.
the assertion in the test is based on 'normal' rounding behaviour, while trunc is date-fns' default.
There was a problem hiding this comment.
other option is to update the assertion
|
I understand that this change is considered breaking, and might belong in a major update only. |
Could it be added as a new roundingMethod for now so it does not change anything and those who want to use it could use it. Something like, "fixedFloor" |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
There seems to be no mention of it here, but roundToNearestMinute has a bug which will be fixed when pr 3467 is merged. My cursory reading of this issue indicates that the bug may be clouding understanding of the underlying issue. |
|
This is a change in behaviour but I wonder how many are using it since it doesn't work as expected when the round method is "floor". Would be nice to see this released. |
|
@kossnocorp please consult #3132 while working on this. It should be useful even though it was based on v2. It has fixes for bad unit tests, fractional milliseconds bug, docs improvements, etc. |
|
@fturmel, thank you for the heads up. I'm on it! I want to compare the fix implementations, as I'm not very familiar with the function. |
|
Thank you so much for your work. I'll make sure to mention you in the release notes. I used this PR as it covers more cases, adds more edge case tests and was also proposed a bit earlier: #3132 |
8 participants
Fixes: #3195 & #3268
The issue that I was facing was that when rewrote this test:
to this
it('rounds according to the passed mode - floor - when nearestTo is provided', () => { const result = roundToNearestMinutes( - new Date(2014, 6 /* Jul */, 10, 12, 10, 30, 5), + new Date(2014, 6 /* Jul */, 10, 12, 19, 30, 5), { nearestTo: 10, roundingMethod: 'floor' } ) assert.deepStrictEqual(result, new Date(2014, 6 /* Jul */, 10, 12, 10, 0)) })My expectation was that
12:19:30:500would be rounded down (floor) to the nearest interval on 10 minutes, i.e.12:10:00:000This updated test failed.
I have updated the tests and implementation accordingly.