Add missed MsgPackLite unit tests #236
Conversation
nicktorwald
force-pushed
the
nicktorwald/gh-127-msgpacklite-tests
branch
from
c89fc11
to
2d7cd48
Compare
There was a problem hiding this comment.
I looked over src/main part of the PR and propose to extract those changes into its own commits, because they are not about tests. I also propose to make those commits more grained (atomic) to push youself to describe all changes and don't miss ones. I explained below why I think that this extra effort is worthful.
I'll continue with review of the tests.
| bb.get(data); | ||
| } | ||
| } | ||
| } else if (item instanceof byte[]) { |
There was a problem hiding this comment.
Don't get this change. It seems that it changes the behaviour. Is it eliminates support of ByteBuffer? Is it becomes unnecessary at some point of development? Let's move this change into its own commit with a proper message. Now I (and anybody else who'll need to look through this commit) should repeat your effort on investigating that to understand a reason of the change.
There was a problem hiding this comment.
explained in 213ca31
I also tried to find any usages in previous versions but failed. Seems it's never been used.
There was a problem hiding this comment.
Thanks for the explanation. Okay so.
There was a problem hiding this comment.
Now I think about this again and it seems ByteBuffer may be passed as an argument to a tarantool operation and formally this change breaks backward compatibility.
I'll unresolve it to think again.
MsgPackLite is not a public API and is used internally by Tarantool client to (de-)serialize iproto packets. Code analysis and code coverage shows that ByteBuffer is not passed as an argument to be serialized (see https://coveralls.io/builds/27255963/source?filename=src/main/java/org/tarantool/MsgPackLite.java#L172). To keep code simplified this code branch was removed as unused.
nicktorwald
force-pushed
the
nicktorwald/gh-127-msgpacklite-tests
branch
3 times, most recently
from
a545735
to
2d29789
Compare
This commit includes a set of small patches that make MsgPackLite and SQLMsgPackLite classes more understandable and more performant. These patches are: - add a brief javadoc description of the classes; - add extra comments that enlighten MsgPack binary protocol; - replace DataOutputStream.write by DataOutputStream.writeByte that looks more readable. Another positive effect here is to use a not synchronized method that it may improve serialization performance. It is ok because MsgPackLite is stateless and can be shared between multiple threads as well; - avoid using shortened variables names (i.e. os instead of more meaningful output, outputStream and so on); - eliminate "unchecked cast" warning for generic containers such as List and Map using more safe unbounded wildcards; - increase an initial capacity of a map being de-serialized from the MsgPack map object (by 25% of the original size). It may help to avoid an unnecessary resize operation during the map filling up.
nicktorwald
force-pushed
the
nicktorwald/gh-127-msgpacklite-tests
branch
from
2d29789
to
40dfbe3
Compare
We are using a borrowed implementation of the MsgPack library w/o available tests. In addition, there are a few modifications that were done on our side. It requires the tests that will cover at least typical use cases related to (de-)serialization between designated java types and message pack types. This commit introduces a set of test cases that check correctness of packing/unpacking according to the MsgPack specification. Closes: #127
nicktorwald
force-pushed
the
nicktorwald/gh-127-msgpacklite-tests
branch
from
40dfbe3
to
2232444
Compare
Closes #127