Add Equals and Hashcode to ApiErrorBase / ApiErrorWithMetadata #26
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
============================================
+ Coverage 95.14% 95.27% +0.13%
- Complexity 722 739 +17
============================================
Files 70 71 +1
Lines 2018 2033 +15
Branches 324 331 +7
============================================
+ Hits 1920 1937 +17
+ Misses 60 59 -1
+ Partials 38 37 -1
Continue to review full report at Codecov.
|
rabeyta
force-pushed
the
apiErrorEquals
branch
from
f339edd
to
4854bf0
Compare
| @Test | ||
| @DataProvider(value = { | ||
| "false | false | false | false | false | true ", | ||
| "false | true | false | false | false | false ", |
There was a problem hiding this comment.
Missing the case where the first arg is true and everything else is false.
| doReturn(name).when(mockApiError1).getName(); | ||
| doReturn(name).when(mockApiError2).getName(); | ||
|
|
||
| String errorCode1 = "1"; |
There was a problem hiding this comment.
"1" vs "2" isn't definitive enough I think. I'd just make errorCode1 and errorCode2 some random UUIDs. Should give higher confidence that the test is verifying what we think it's verifying. You also might want to include some verify(mockApiError1).getErrorCode() to verify that getErrorCode() is being called on both ApiErrors.
There was a problem hiding this comment.
I've made those suggestions.
|
|
||
| /** | ||
| * A comparator that knows how to compare {@link ApiError} instances by {@link ApiError#getName()} first, and then by | ||
| * {@link ApiError#getMetadata()} to allow for multiple same-named errors that have different metadata. | ||
| * {@link ApiError#getErrorCode()} ()} to allow for multiple same-named errors that have different metadata. |
There was a problem hiding this comment.
Semi-incomplete javadoc update. I'd recommend:
A comparator that knows how to compare {@link ApiError} instances by {@link ApiError#getName()} first,
then by {@link ApiError#getErrorCode()}, and finally by everything else by comparing hashcodes using
{@link ApiErrorUtil#generateApiErrorHashCode(ApiError)}.
<p>Note that this means two {@link ApiError}s that are identical other than metadata will be considered
different by this comparator.
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Utility class to enable sharing code between {@link ApiError} implementations. This probably won't be used outside |
There was a problem hiding this comment.
I'd recommend a slightly different javadoc:
Utility class to enable sharing code between {@link ApiError} implementations. Most Backstopper end-users
won't need to use this class unless you're creating custom {@link ApiError} implementations (which is not
common).
| new ApiErrorBase(null, 42, "some error", 400); | ||
| } | ||
|
|
||
| @Test(expected = IllegalArgumentException.class) | ||
| public void constructor_should_throw_IllegalArgumentException_null_error_code() { | ||
| new ApiErrorBase(null, null, "some error", 400, null); |
There was a problem hiding this comment.
The IllegalArgumentException being thrown here is likely due to the null name, not the null error code you're trying to test. I'd recommend switching from @Test(expected = ...) to AssertJ's catchThrowable and verify the message so you know the exception was thrown for the desired reason.
There was a problem hiding this comment.
i've changed this test and the other in the class to use catchThrowable
| @Test | ||
| @DataProvider(value = { | ||
| "false | false | false | false | false | true | true ", | ||
| "false | true | false | false | false | false | false ", |
There was a problem hiding this comment.
Same missing data provider row.
|
|
||
| @Test | ||
| public void generate_api_error_hashcode_generates_expected_hashcodes() { | ||
| Map<String, Object> metadata = MapBuilder.<String, Object>builder().put("foo", UUID.randomUUID().toString()).build(); |
There was a problem hiding this comment.
I would duplicate the Objects.hash(...) here in the test and verify that the hashcode returned by the method matches the expected Objects.hash(...) value. It's a bit of a tautology, but works as a sanity check and can catch if the method gets updated in the future to make sure we take a step back and ask if it's a change we really want.
There was a problem hiding this comment.
good call. i've updated
| @Test | ||
| @DataProvider(value = { | ||
| "false | false | false | false | false | true ", | ||
| "false | true | false | false | false | false ", |
There was a problem hiding this comment.
Same missing data provider row.
rabeyta
force-pushed
the
apiErrorEquals
branch
2 times, most recently
from
1e1f22a
to
af5fcc0
Compare
…ata. Modify ApiErrorComparator to use new comparison and sort by ErrorCode after Name
rabeyta
force-pushed
the
apiErrorEquals
branch
from
af5fcc0
to
1517d94
Compare
|
Fantastic - thank you @rabeyta! |
Add equals and hashcode methods to ApiErrorBase and ApiErrorWithMetadata. Modify ApiErrorComparator to use new comparison and sort by ErrorCode after Name.