Skip to content

Commit

Permalink
[TESTS] Clean up mockito never() and eq(null) usages
Browse files Browse the repository at this point in the history
never() and any() don't play well together for nullable types. Change
the broadcast mocks to user nullable() and fixup tests.
  • Loading branch information
julianknutsen committed Dec 3, 2019
1 parent 1bd450b commit 17f4b70
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ public void getMailboxDataWithSignedSeqNr_RemoveNoExist() throws NoSuchAlgorithm
SavedTestState beforeState = this.testState.saveTestState(protectedMailboxStorageEntry);
Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, TestState.getTestNodeAddress()));

this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, false, false, true);
this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, false, true, true);
}

// TESTCASE: Adding, then removing a mailbox message from the getMailboxDataWithSignedSeqNr API
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,11 @@ enum TestCase {
ON_MESSAGE,
}

void doAddAndVerify(PersistableNetworkPayload persistableNetworkPayload, boolean expectedReturnValue, boolean expectedStateChange) {
void doAddAndVerify(PersistableNetworkPayload persistableNetworkPayload,
boolean expectedReturnValue,
boolean expectedHashMapAndDataStoreUpdated,
boolean expectedListenersSignaled,
boolean expectedBroadcast) {
SavedTestState beforeState = this.testState.saveTestState(persistableNetworkPayload);

if (this.testCase == TestCase.PUBLIC_API) {
Expand All @@ -89,7 +93,7 @@ void doAddAndVerify(PersistableNetworkPayload persistableNetworkPayload, boolean
testState.mockedStorage.onMessage(new AddPersistableNetworkPayloadMessage(persistableNetworkPayload), mockedConnection);
}

this.testState.verifyPersistableAdd(beforeState, persistableNetworkPayload, expectedStateChange, expectedStateChange, expectedStateChange);
this.testState.verifyPersistableAdd(beforeState, persistableNetworkPayload, expectedHashMapAndDataStoreUpdated, expectedListenersSignaled, expectedBroadcast);
}

@Before
Expand Down Expand Up @@ -119,16 +123,15 @@ public static Collection<Object[]> data() {
@Test
public void addPersistableNetworkPayload() {
// First add should succeed regardless of parameters
doAddAndVerify(this.persistableNetworkPayload, true, true);
doAddAndVerify(this.persistableNetworkPayload, true, true, true, true);
}

@Test
public void addPersistableNetworkPayloadDuplicate() {
doAddAndVerify(this.persistableNetworkPayload, true, true);
doAddAndVerify(this.persistableNetworkPayload, true, true, true, true);

// Second call only succeeds if reBroadcast was set
boolean expectedReturnValue = this.reBroadcast;
doAddAndVerify(this.persistableNetworkPayload, expectedReturnValue, false);
// We return true and broadcast if reBroadcast is set
doAddAndVerify(this.persistableNetworkPayload, this.reBroadcast, false, false, this.reBroadcast);
}
}

Expand All @@ -145,7 +148,7 @@ PersistableNetworkPayloadStub createInstance() {
public void invalidHash() {
PersistableNetworkPayload persistableNetworkPayload = new PersistableNetworkPayloadStub(false);

doAddAndVerify(persistableNetworkPayload, false, false);
doAddAndVerify(persistableNetworkPayload, false, false, false, false);
}
}

Expand All @@ -168,7 +171,7 @@ public void outOfTolerance() {
// The onMessage path checks for tolerance
boolean expectedReturn = this.testCase != TestCase.ON_MESSAGE;

doAddAndVerify(persistableNetworkPayload, expectedReturn, expectedReturn);
doAddAndVerify(persistableNetworkPayload, expectedReturn, expectedReturn, expectedReturn, expectedReturn);
}
}
}
22 changes: 10 additions & 12 deletions p2p/src/test/java/bisq/network/p2p/storage/TestState.java
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,10 @@ void verifyPersistableAdd(SavedTestState beforeState,
verify(this.appendOnlyDataStoreListener, never()).onAdded(persistableNetworkPayload);

if (expectedBroadcast)
verify(this.mockBroadcaster).broadcast(any(AddPersistableNetworkPayloadMessage.class), any(NodeAddress.class),
eq(null));
verify(this.mockBroadcaster).broadcast(any(AddPersistableNetworkPayloadMessage.class),
nullable(NodeAddress.class), isNull());
else
verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class));
verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), nullable(NodeAddress.class), nullable(BroadcastHandler.Listener.class));
}

void verifyProtectedStorageAdd(SavedTestState beforeState,
Expand Down Expand Up @@ -219,14 +219,13 @@ void verifyProtectedStorageAdd(SavedTestState beforeState,

if (expectedBroadcast) {
final ArgumentCaptor<BroadcastMessage> captor = ArgumentCaptor.forClass(BroadcastMessage.class);
verify(this.mockBroadcaster).broadcast(captor.capture(), any(NodeAddress.class),
eq(null));
verify(this.mockBroadcaster).broadcast(captor.capture(), nullable(NodeAddress.class), isNull());

BroadcastMessage broadcastMessage = captor.getValue();
Assert.assertTrue(broadcastMessage instanceof AddDataMessage);
Assert.assertEquals(protectedStorageEntry, ((AddDataMessage) broadcastMessage).getProtectedStorageEntry());
} else {
verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class));
verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), nullable(NodeAddress.class), nullable(BroadcastHandler.Listener.class));
}

if (expectedSequenceNrMapWrite) {
Expand Down Expand Up @@ -276,7 +275,7 @@ void verifyProtectedStorageRemove(SavedTestState beforeState,
verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong());

if (!expectedBroadcast)
verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class));
verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), nullable(NodeAddress.class), nullable(BroadcastHandler.Listener.class));


protectedStorageEntries.forEach(protectedStorageEntry -> {
Expand All @@ -288,9 +287,9 @@ void verifyProtectedStorageRemove(SavedTestState beforeState,

if (expectedBroadcast) {
if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry)
verify(this.mockBroadcaster).broadcast(any(RemoveMailboxDataMessage.class), any(NodeAddress.class), eq(null));
verify(this.mockBroadcaster).broadcast(any(RemoveMailboxDataMessage.class), nullable(NodeAddress.class), isNull());
else
verify(this.mockBroadcaster).broadcast(any(RemoveDataMessage.class), any(NodeAddress.class), eq(null));
verify(this.mockBroadcaster).broadcast(any(RemoveDataMessage.class), nullable(NodeAddress.class), isNull());
}


Expand Down Expand Up @@ -320,8 +319,7 @@ void verifyRefreshTTL(SavedTestState beforeState,
Assert.assertTrue(entryAfterRefresh.getCreationTimeStamp() > beforeState.creationTimestampBeforeUpdate);

final ArgumentCaptor<BroadcastMessage> captor = ArgumentCaptor.forClass(BroadcastMessage.class);
verify(this.mockBroadcaster).broadcast(captor.capture(), any(NodeAddress.class),
eq(null));
verify(this.mockBroadcaster).broadcast(captor.capture(), nullable(NodeAddress.class), isNull());

BroadcastMessage broadcastMessage = captor.getValue();
Assert.assertTrue(broadcastMessage instanceof RefreshOfferMessage);
Expand All @@ -338,7 +336,7 @@ void verifyRefreshTTL(SavedTestState beforeState,
Assert.assertEquals(beforeState.creationTimestampBeforeUpdate, entryAfterRefresh.getCreationTimeStamp());
}

verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class));
verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), nullable(NodeAddress.class), nullable(BroadcastHandler.Listener.class));
verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong());
}
}
Expand Down

0 comments on commit 17f4b70

Please sign in to comment.