fix: Timestamp.of(java.sql.Timestamp) pre-epoch on exact second - #179
Conversation
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #179 +/- ##
=========================================
Coverage 67.76% 67.76%
- Complexity 378 379 +1
=========================================
Files 36 36
Lines 1967 1967
Branches 259 259
=========================================
Hits 1333 1333
Misses 526 526
Partials 108 108
Continue to review full report at Codecov.
|
Sorry, something went wrong.
| } | ||
|
|
||
| @Test | ||
| public void ofSqlTimestampOnExactSecond() { |
There was a problem hiding this comment.
Can you make this method name more conventional? e.g. testOf_exactSecond or some such.
Sorry, something went wrong.
There was a problem hiding this comment.
Done. Although this means that the name of this test is not in line with the other test cases in this file, such as for example ofSqlTimestampPreEpoch.
Sorry, something went wrong.
There was a problem hiding this comment.
Section 8.5.2 of the Google style guide addresses this:
When adding new code to a file that is not in Google Style, reformatting the existing code first is recommended, subject to the advice in Section 8.5.1, Reformatting existing code.
If this reformatting is not done, new code is still required to be added in Google Style, even if this creates inconsistencies.
Sorry, something went wrong.
There was a problem hiding this comment.
I've updated all the test cases for Timestamp.of(java.sql.Timestamp) to use consistent naming and assertEquals instead of assertThat....
Sorry, something went wrong.
| // works. For example, -1001 / 1000 == -1. In this case of timestamps, we want this result to be | ||
| // -2. This causes any pre-epoch timestamp to be off by 1 second - fix this by adjusting the | ||
| // seconds value by 1 if the timestamp < 0. | ||
| // seconds value by 1 if the timestamp < 0 and the division by 1000 has a remainder. |
There was a problem hiding this comment.
to be technical, a non-zero remainder. Suggest rephrasing as "the timestamp is less than zero and is not divisible by 1000."
Sorry, something went wrong.
There was a problem hiding this comment.
Changed to fix this by subtracting 1 from the seconds value if the seconds value is less than zero and is not divisible by 1000.
Sorry, something went wrong.
codyoss
left a comment
There was a problem hiding this comment.
Good catch
Sorry, something went wrong.
The fix for negative timestamps in #160 did not take into account the possibility that the pre-epoch timestamp could be on an exact second value. In that case, the additional correction of the calculated second value should not be applied.
Fixes googleapis/java-spanner-jdbc#85