Skip to content

Conversation

@ztarvos
Copy link

@ztarvos ztarvos commented Dec 19, 2018

Updated jdbc tests following static SQL types
introduction into tarantool 2.1.

Closes #90

Updated jdbc tests following static SQL types
introduction into tarantool 2.1.

Closes tarantool#90
@ztarvos ztarvos requested a review from Totktonada December 19, 2018 12:44
@coveralls
Copy link

Pull Request Test Coverage Report for Build 100

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 23 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.3%) to 62.47%

Files with Coverage Reduction New Missed Lines %
src/main/java/org/tarantool/TarantoolClientImpl.java 1 82.88%
src/main/java/org/tarantool/jdbc/SQLMsgPackLite.java 2 83.33%
src/main/java/org/tarantool/jdbc/SQLResultSet.java 5 17.98%
src/main/java/org/tarantool/MsgPackLite.java 5 78.18%
src/main/java/org/tarantool/jdbc/SQLPreparedStatement.java 10 33.66%
Totals Coverage Status
Change from base Build 99: -0.3%
Covered Lines: 1295
Relevant Lines: 2073

💛 - Coveralls

Copy link

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

I found names within TestHelper too common (it was a bit tricky for me to get the idea from them), but I don't think it worth to do one more review iteration because of that (I think we can be quite tolerable against testing code).

I think that we possibly miss to test cases like 2^64 for INT Tarantool/SQL type (see mapping on the wiki). But this is out of scope of this issue and we'll do it in the scope of #92.

Don't sure we should test storing dates within ints, but there are no other way now, so it does not much matter.

There were two other changes in Tarantool that breaks some tests, I'll push fixes to the master branch on top of this one.

Thank you, Sergei!

@Totktonada Totktonada merged commit b1ed485 into tarantool:master Dec 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants