search

TechnionYP5779 / team1

Git repo of team 1 of the yearly project technion CS
6 stars 3 forks source link

Unit tests for Utils.java #101

Closed matanpugach closed 5 years ago

matanpugach commented 5 years ago

96

matanpugach commented 5 years ago

Done. Please review! The coverage is maximized. can't do better.

ofiralexi commented 5 years ago

Your tests are good, but the use of assertions needs to match the required (look at assertTrue,assertFalse, and equals) :)

ShaiZeevi commented 5 years ago
  1. Please avoid such tests (there are a few, I'm only giving one as an example): image

Use the annotation "@Test(expected = MyException.class)" to ensure that an exception is thrown in a test.

  1. There is no reason to use assert instead of azzert here: image for that we have azzert.isNull and azzert.notNull

  2. Is there no way to ue the fluent API that&is here: image Since assertArrayEquals is not part of the fluent API I would prefer that you not use it.

  3. Please use azzert instead of Assert such as in this picture: image

  4. It's not a huge issue in my opinion but Arrays.emptyList() would be much better in my opinion in cases such as these (since it's declarative immutable and the variable is final anyway): image

Please fix these and submit for re-review, Thank you.

matanpugach commented 5 years ago

Couldn't find a suitable for Arrays.emptyList() would. Thanks so much for the awesome feedback! Updated accordingly 😄

ofiralexi commented 5 years ago

why there are a lot of unchecked null branches ? image

matanpugach commented 5 years ago

image @ofiralexi as stated, i couldn't reach those cases.

yoavz1997 commented 5 years ago

LGTM #79