Excavator: Prefer AssertJ #5184
Conversation
svc-excavator-bot
force-pushed
the
roomba/assertj
branch
18 times, most recently
from
de33328 to
27229ef
Compare
svc-excavator-bot
force-pushed
the
roomba/assertj
branch
from
27229ef to
e4bd6a5
Compare
| @@ -79,7 +79,7 @@ public void refreshSingleCallsRefreshWithMatchingArgument() throws InterruptedEx | |||
| @Test | |||
| public void refreshSingleReturnsTokenIfCanBeRefreshed() throws InterruptedException { | |||
| when(mockClient.refresh(any())).then(REPLY_WITH_FIRST_TOKEN); | |||
| assertThat(LOCK_TOKEN).isEqualTo(client.refreshSingle(LOCK_TOKEN)); | |||
There was a problem hiding this comment.
wow! this is really cool
| fail(); | ||
| } catch (TransactionSerializableConflictException e) { | ||
| // this is expected to throw because it is a write skew | ||
| } |
There was a problem hiding this comment.
This is correct, though I'd add an as() clause for the explanation
There was a problem hiding this comment.
I believe it uses the message (if present) from fail(String). Losing the comment is unfortunate, we had tried to preserve them in some refactors, but clearly not all cases :-/
There was a problem hiding this comment.
It does, yep! I think you can read most of these as comments I'm writing to myself to fix.
I don't know if you want things of the form try { <things>; fail(); } catch (Exception e) { // INCLUDEME } since I'm sure there are going to be quite a few that just say expected or things like that
There was a problem hiding this comment.
Ya that's true, at the end of the day we always want humans to review this sort of change. If you see a pattern that occurs frequently and isn't difficult to detect, use your judgement on filing a ticket to track :-)
| } catch (Exception e) { | ||
| // success | ||
| } | ||
| assertThatThrownBy(() -> Namespace.create(string, pattern)).describedAs("Namespace '" + string + "' was not supposed to match pattern '" + pattern + "'").isInstanceOf(Exception.class); |
There was a problem hiding this comment.
clever! though I imagine checkstyle would fail
| assert index3.getRange(RangeRequest.builder().build()).count() == 1; | ||
| assertThat(index1.getRange(RangeRequest.builder().build()).count() == 1).isTrue(); | ||
| assertThat(index2.getRange(RangeRequest.builder().build()).count() == 1).isTrue(); | ||
| assertThat(index3.getRange(RangeRequest.builder().build()).count() == 1).isTrue(); |
There was a problem hiding this comment.
Yeah I guess let's have this thing iterate a bit more
| assertEquals(1, end.length); | ||
| assertEquals(-1, end[0]); | ||
| assertThat(end.length).isEqualTo(1); | ||
| assertThat(end[0]).isEqualTo(-1); |
There was a problem hiding this comment.
@carterkozak Probably don't need to fix this, but amazingly this isn't semantically equivalent! (The original works because Assert#assertEquals looks like it does widening conversions to longs, but this fails because the byte -1 and int -1 aren't considered equal in AssertJ, probably because they are different types after boxing).
There was a problem hiding this comment.
Yep, we definitely handled some of these, but I must have neglected to implement byte/int conversion. I think I used the rules for java implicit conversion, which doesn't quite match assertj behavior. Mind filing a ticket on https://github.com/palantir/assertj-automation ?
There was a problem hiding this comment.
Yep, here goes: palantir/assertj-automation#261 and a quick scan of the code suggests that yeah, it goes to the catchall Object rules for these
|
I wonder why excavator didn't format the results? |
| @@ -29,15 +29,15 @@ public void testPrefix() { | |||
| assertThat(end.length).isEqualTo(0); | |||
| end = RangeRequest.builder().prefixRange(new byte[] {-2}).build().getEndExclusive(); | |||
| assertThat(end.length).isEqualTo(1); | |||
| assertThat(end[0]).isEqualTo(-1); | |||
| assertThat(end[0]).isEqualTo((byte) -1); | |||
There was a problem hiding this comment.
This is pretty sad, but there is no nice way around it. This test is also super bad anyway so fine with leaving as is
atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/schema/indexing/IndexTest.java
Outdated
Show resolved
Hide resolved
atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/schema/indexing/IndexTest.java
Outdated
Show resolved
Hide resolved
…/indexing/IndexTest.java Co-authored-by: gmaretic <[email protected]>
…/indexing/IndexTest.java Co-authored-by: gmaretic <[email protected]>
|
👍 |
|
Released 0.285.0 |
excavator is a bot for automating changes across repositories.
Changes produced by the roomba/assertj check.
To enable or disable this check, please contact the maintainers of Excavator.