> On May 11, 2015, 11:52 p.m., Jason Altekruse wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulation.java, > > line 69 > > <https://reviews.apache.org/r/34063/diff/3/?file=955845#file955845line69> > > > > A minor comment, but as assertEquals will already print out something > > like: expecting <0> but recieved <other val>, This message doesn't add > > anything to that, I would make it more descriptive or just remove it. > > Hanifi Gunes wrote: > "value" indicates result coming out of accessor.get(#). Error message is > actually helpful in giving more context about a possible mismatch whether it > is offset or value mismatch.
I did realize this when I made my original comment, but I was trying to think from the perspective of someone running the full suite of unit tests and they see one of these failing. I think either way its pretty much a wash, because unless you wrote the test it is unlikely that a failure message will lead you to make a code change, I almost always will use the line number to look at the test to see what its doing and debug from there. They will see a message like: AssertionError: Values must match: expected <0> but was: <5> arguably an improvement over: AssertionError: expected <0> but was: <5> but I just didn't think it added more information when thinking from the perspective of a consumer of the message - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34063/#review83318 ----------------------------------------------------------- On May 12, 2015, 12:18 a.m., Hanifi Gunes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34063/ > ----------------------------------------------------------- > > (Updated May 12, 2015, 12:18 a.m.) > > > Review request for drill, abdelhakim deneche, Jason Altekruse, Mehant Baid, > and Venki Korukanti. > > > Repository: drill-git > > > Description > ------- > > DRILL-2776: add extensive tests for empty population coverage > > > Diffs > ----- > > > exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulation.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/TestEmptyPopulator.java > 23cc316aac7e7d64fea2a2549a42d0a9c4d401bb > exec/java-exec/src/test/resources/vector/complex/map-empty-between.json > PRE-CREATION > exec/java-exec/src/test/resources/vector/complex/map-empty-first.json > PRE-CREATION > exec/java-exec/src/test/resources/vector/complex/map-empty-last.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/multi-repeated-list-empty-between.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/multi-repeated-list-empty-first.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/multi-repeated-list-empty-last.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/multi-repeated-list-multi-empty.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/repeated-list-empty-between.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/repeated-list-empty-first.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/repeated-list-empty-last.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/repeated-map-empty-between.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/repeated-map-empty-first.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/repeated-map-empty-last.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/repeated-scalar-empty-between.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/repeated-scalar-empty-first.json > PRE-CREATION > > exec/java-exec/src/test/resources/vector/complex/repeated-scalar-empty-last.json > PRE-CREATION > > Diff: https://reviews.apache.org/r/34063/diff/ > > > Testing > ------- > > TestEmptyPopulation > > > Thanks, > > Hanifi Gunes > >
