Skip to content
This repository has been archived by the owner on Dec 3, 2023. It is now read-only.

fix: Timestamp.of(java.sql.Timestamp) pre-epoch on exact second #179

Merged
merged 4 commits into from Mar 12, 2020

Conversation

olavloite
Copy link
Contributor

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

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 12, 2020
@codecov
Copy link

codecov bot commented Mar 12, 2020

Codecov Report

Merging #179 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ Complexity Δ
...core/src/main/java/com/google/cloud/Timestamp.java 90.62% <100%> (ø) 26 <0> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae35d0e...b9e2161. Read the comment docs.

@@ -88,6 +88,14 @@ public void ofSqlTimestamp() {
assertThat(timestamp.toString()).isEqualTo(expectedTimestampString);
}

@Test
public void ofSqlTimestampOnExactSecond() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this method name more conventional? e.g. testOf_exactSecond or some such.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated all the test cases for Timestamp.of(java.sql.Timestamp) to use consistent naming and assertEquals instead of assertThat....

@olavloite olavloite requested a review from elharo March 12, 2020 12:37
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be technical, a non-zero remainder. Suggest rephrasing as "the timestamp is less than zero and is not divisible by 1000."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@olavloite olavloite requested a review from elharo March 12, 2020 16:59
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner-jdbc: JdbcTypeConverterTest failure
5 participants