Various unit tests, minor fixes, and dead code removal #1148
Conversation
|
Can we make the description of this pull-request more specific and give a general indication of the types of tests we are adding? |
| @@ -44,30 +40,19 @@ | |||
| { | |||
| private static final ObjectMapper jsonMapper = new DefaultObjectMapper(); | |||
|
|
|||
| public static <K, V> Map<K, V> zipMap(Iterable<K> keys, Iterable<V> values) | |||
There was a problem hiding this comment.
any reason we are removing this? This doesn't seem to be part of tests
There was a problem hiding this comment.
thanks, updated the description.
and, this code does not seem to get used anywhere. I can put it back if there is a reason to keep it around.
what do you think?
There was a problem hiding this comment.
I'm fine removing the code if it is not used anywhere.
|
I didn't closely look at every test, but I believe the following is true:
I am in favor of this PR if the above is true. |
| @@ -112,7 +112,7 @@ public IntBuffer getBaseList(int index) | |||
| final IntBuffer retVal = IntBuffer.wrap(array); | |||
|
|
|||
| if (index + 1 == baseListCount()) { | |||
| retVal.limit(maxIndex - (index * allocateSize)); | |||
| retVal.limit(maxIndex - (index * allocateSize) + 1); | |||
There was a problem hiding this comment.
It's a fix to a problem found with a unit test. Check out the branch, remove the +1 and run the tests...
|
There seem to be a few non unit test changes in this PR, can those be separated more clearly somehow? |
| { | ||
| testingIterator.remove(); | ||
| } | ||
| } |
There was a problem hiding this comment.
can we add newlines at the end of files?
|
I'm in agreement with @fjy , but I would really like to see the non-unit tests items in at least a separate commit so its more obvious they are changing. |
|
The non-unit test changes are all fixing bugs that exist in the current code found by the unit tests. If we remove them, the tests won't pass and then travis will be upset and then what do we do? |
|
@fjy all of what you said is true. |
xvrl
changed the title
xvrl
changed the title
|
I'm on board with all those fixes + changes, I updated the PR title to include the fact that we fixed things. |
|
I'm happy do the squashing and header update for you if that makes it easier. |
Also updated license header as part of the squash.
|
Merged as part of af807c6 |
this patch contains