Skip to content

ADD: Testcase to show broken java.sql.Time support#558

Merged
peterbae merged 6 commits intomicrosoft:devfrom
FOCONIS:bug/testcase-for-time-support
Mar 12, 2018
Merged

ADD: Testcase to show broken java.sql.Time support#558
peterbae merged 6 commits intomicrosoft:devfrom
FOCONIS:bug/testcase-for-time-support

Conversation

@rPraml
Copy link
Copy Markdown
Contributor

@rPraml rPraml commented Nov 20, 2017 •
edited
Loading

This PR is for issue #559

@rPraml rPraml mentioned this pull request Nov 20, 2017
Closed
@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 21, 2017 •
edited
Loading

Codecov Report

Merging #558 into dev will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##                dev    #558      +/-   ##
===========================================
+ Coverage     48.04%   48.1%   +0.06%     
- Complexity     2573    2578       +5     
===========================================
  Files           113     113              
  Lines         26574   26574              
  Branches       4429    4429              
===========================================
+ Hits          12768   12784      +16     
+ Misses        11683   11667      -16     
  Partials       2123    2123
Flag Coverage Δ Complexity Δ
#JDBC42 47.97% <ø> (+0.06%) 2568 <ø> (+3) ⬆️
#JDBC43 47.96% <ø> (-0.01%) 2576 <ø> (+7)
Impacted Files Coverage Δ Complexity Δ
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java 61.92% <0%> (-0.84%) 63% <0%> (ø)
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 50.6% <0%> (-0.25%) 157% <0%> (ø)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 54.2% <0%> (-0.1%) 0% <0%> (ø)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.61% <0%> (-0.07%) 239% <0%> (-2%)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 44.94% <0%> (ø) 16% <0%> (ø) ⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 63.35% <0%> (+0.42%) 0% <0%> (ø) ⬇️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 39.18% <0%> (+0.64%) 43% <0%> (+1%) ⬆️
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 61.57% <0%> (+0.65%) 89% <0%> (+1%) ⬆️
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java 52.59% <0%> (+1.48%) 12% <0%> (+1%) ⬆️
...m/microsoft/sqlserver/jdbc/SQLServerException.java 77.23% <0%> (+1.62%) 30% <0%> (+1%) ⬆️
... and 1 more

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 4a99502...2d740b7. Read the comment docs.

@cheenamalhotra cheenamalhotra added this to the 6.3.6 milestone Nov 24, 2017
@cheenamalhotra cheenamalhotra removed this from the 6.3.6 milestone Dec 9, 2017
@peterbae peterbae self-assigned this Mar 9, 2018
peterbae
peterbae reviewed Mar 12, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@peterbae peterbae left a comment

Choose a reason for hiding this comment

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

Hi @rPraml, thanks for the contribution. I think we can merge this in after making these small changes to the PR.

Comment thread src/test/java/com/microsoft/sqlserver/jdbc/datatypes/DateAndTimeTypeTest.java Outdated
@RunWith(JUnitPlatform.class)
public class DateAndTimeTypeTest extends AbstractTest {

private static final Date DATE_TO_TEST = new java.sql.Date(2017, 20, 11);
Copy link
Copy Markdown
Contributor

@peterbae peterbae Mar 12, 2018

Choose a reason for hiding this comment

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

Could we use the non-deprecated constructors for Date, Time and Timestamp?

Copy link
Copy Markdown
Contributor Author

@rPraml rPraml Mar 12, 2018

Choose a reason for hiding this comment

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

Yes, of course. Just use any date/time/timestamp to run this test.
(I'm wondereing if 2017,20,11 are valid arguments at all)

Copy link
Copy Markdown
Contributor

@peterbae peterbae Mar 12, 2018

Choose a reason for hiding this comment

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

I think this style of argument is deprecated, and they want us to provide a long value that corresponds to this time instead. The long values for these Date, Time and Timestamp objects come to 61494793200000L, 74096000L, and 61494838496000L respectively, I believe.

Comment thread src/test/java/com/microsoft/sqlserver/jdbc/datatypes/DateAndTimeTypeTest.java Outdated

@BeforeEach
public void testSetup() throws TestAbortedException, Exception {
assumeTrue(9 <= new DBConnection(connectionString).getServerVersion(),
Copy link
Copy Markdown
Contributor

@peterbae peterbae Mar 12, 2018

Choose a reason for hiding this comment

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

I think we can close this DBConnection by using try statement.

Copy link
Copy Markdown
Contributor Author

@rPraml rPraml Mar 12, 2018

Choose a reason for hiding this comment

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

Ack. (I think I've copied this line from an other test)

@rPraml
Copy link
Copy Markdown
Contributor Author

rPraml commented Mar 12, 2018

Hello @peterbae can you update this PR as mentioned above or should I do that?

@peterbae
Copy link
Copy Markdown
Contributor

peterbae commented Mar 12, 2018

@rPraml I can update this PR for you, and I'll merge it as well.

@peterbae peterbae merged commit d970d77 into microsoft:dev Mar 12, 2018
@cheenamalhotra cheenamalhotra added this to the 6.5.1 milestone Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@peterbae peterbae peterbae approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@peterbae peterbae

Labels

None yet

Projects

None yet

Milestone

6.5.1

Development

Successfully merging this pull request may close these issues.

4 participants

@rPraml @codecov-io @peterbae @cheenamalhotra