Ibft Int. testing - Round Change #537
Conversation
Integration tests have been added to determine the IBFT components response to the reception of RoundChange messages. Through the course of creating these tests the following changes were made to the RoundChangeManager: * Stores RoundChangeMessages in the order of arrival * Discards subsequent RoundChange messages from the same node, targetting a specific round.
| return CommitMessageData.fromMessageData(actual).decode().equals(expected); | ||
| actualMessage = CommitMessageData.fromMessageData(actual).decode(); | ||
| break; | ||
| // final SignedData<CommitPayload> actualCommit = |
There was a problem hiding this comment.
Or these these comments are going to become deleted or uncommented?
There was a problem hiding this comment.
Removed - now do a recursive Field-by-field comparison.
| default: | ||
| return false; | ||
| assertThat(true).isFalse(); // fail |
There was a problem hiding this comment.
How about replacing assertThat(true).isFalse(); // fail with something like fail("Unexpected IBFTv2 message type")?
| } | ||
| assertThat(expectedPayload.equals(actualMessage.getPayload())).isTrue(); |
There was a problem hiding this comment.
A more comprehensive equals (that doesn't rely on equals being implemented correctly) for assertThat(expectedPayload.equals(actualMessage.getPayload())).isTrue(); would be
assertThat(actualMessage.getPayload()).isEqualToComparingFieldByFieldRecursively(expectedPayload);
It may or may not be easier / nicer to read, probably depends on personal preferences.
There was a problem hiding this comment.
Believe it or not - I was in the process of changing to this.
| @@ -48,7 +48,7 @@ | |||
|
|
|||
| // Store only 1 round change per round per validator | |||
| @VisibleForTesting | |||
| final Map<Address, SignedData<RoundChangePayload>> receivedMessages = Maps.newHashMap(); | |||
| final Map<Address, SignedData<RoundChangePayload>> receivedMessages = Maps.newLinkedHashMap(); | |||
There was a problem hiding this comment.
What's the reason for using the LinkedHashMap?
There was a problem hiding this comment.
To maintain insertion order - its not required - but it makes testing and debug a whole lot easier
| } | ||
| assertThat(expectedPayload.equals(actualMessage.getPayload())).isTrue(); |
There was a problem hiding this comment.
would be nicer to use the isEquals instead using equals with isTrue i.e. assertThat(expectedPayload).isEqualTo(actualMessage.getPayload());
There was a problem hiding this comment.
Move to fieldByFieldRecursively
There was a problem hiding this comment.
that's ok. still prefer equals though, field comparison is a bit ugly
There was a problem hiding this comment.
but is a really easy easy to see when tests fail and why - rather than using a debugger.
| package tech.pegasys.pantheon.consensus.ibft.tests; | ||
|
|
||
| import static java.util.Optional.empty; | ||
| import static org.mockito.Mockito.mock; |
There was a problem hiding this comment.
nit: noticed these imports aren't used
| } | ||
|
|
||
| @Test | ||
| public void messagesFromPreviousRoundAreDiscardedOnTransitionToFutureRound() {} |
There was a problem hiding this comment.
looks like this test hasn't been implemented
| } | ||
|
|
||
| @Test | ||
| public void roundChangeHasEmptyCertificateIfNoPrepareMessagesReceived() { |
There was a problem hiding this comment.
nice to have a similar test that if we have proposals but not quorum we get a prepare certificate
| } | ||
|
|
||
| @Test | ||
| public void roundChangeHasPopulatedCertificateIfQuorumPrepareMessagesAreReceived() { |
There was a problem hiding this comment.
nit: isn't this really we have quorum prepare messages and a proposal. without the proposal there wouldn't be a prepared cert if I remember correctly.
| targetRound, | ||
| Optional.of( | ||
| new PreparedCertificate( | ||
| roles |
There was a problem hiding this comment.
be create some small helper functions within the test for the prepare and proposal payload, think that would make this test a bit more readable
Integration tests have been added to determine the IBFT components
response to the reception of RoundChange messages.
Through the course of creating these tests the following changes were
made to the RoundChangeManager:
targetting a specific round.
PR description
Fixed Issue(s)