Ibft Integration test framework #502
Conversation
|
|
||
| public class NetworkLayout { | ||
|
|
||
| final NodeParams localNode; |
There was a problem hiding this comment.
Nit, should these also be scoped to private to keep consistency with the other classes in this PR? (and that accessors for these fields are being provided)
|
|
||
| public RoundSpecificNodeRoles getRoundSpecificRoles(final ConsensusRoundIdentifier roundId) { | ||
| // This will return NULL if the LOCAL node is the proposer for the specified round | ||
| final Address proposerAddress = finalState.getProposerForRound(roundId); |
There was a problem hiding this comment.
If the return value is optional, what are your thoughts about using Optional<Address> for the return type?
There was a problem hiding this comment.
Would really rather not - otherwise, everytime you go to use "getProposer()" you then have to perform a "get()".
ATM: The test script writer is expected to know how their network is laid out, and thus should appreciate that the "remote nodes" do not have a proposer amongst them....
There was a problem hiding this comment.
...and there's always the NPE.
There was a problem hiding this comment.
yeah, I kinda figured the NPE failing the test would result in someone checking out what's gone wrong ... ultimately is a badly formed test so should fail.
| } | ||
|
|
||
| public List<MessageData> getReceivedMessages() { | ||
| return receivedMessages; |
There was a problem hiding this comment.
You're breaking object encapsulation by returning a reference to the underlying list. Is this the intention, otherwise you could do something like Collections.unmodifiableList(receivedMessages) to return an immutable list (view of the data) and remove the possibility of callings messing with your object.
| @@ -38,14 +38,14 @@ | |||
| private final Collection<Signature> seals; | |||
| private final Optional<Vote> vote; | |||
| private final int round; | |||
| private final List<Address> validators; | |||
| private final Collection<Address> validators; | |||
There was a problem hiding this comment.
Are multiple entries per a validator permitted in this collection? (If not, then a Set would be better suited, also providing richer context for the reader)
There was a problem hiding this comment.
Are you OK if this gets raised as a Ticket, I THINK there's more to this than collection vs set vs list - but you're right, this should/could be a better representation.
...ft/src/integration-test/java/tech/pegasys/pantheon/consensus/ibft/support/NetworkLayout.java
Outdated
Show resolved
Hide resolved
.../integration-test/java/tech/pegasys/pantheon/consensus/ibft/support/StubIbftMulticaster.java
Outdated
Show resolved
Hide resolved
...ibft/src/integration-test/java/tech/pegasys/pantheon/consensus/ibft/support/TestContext.java
Outdated
Show resolved
Hide resolved
|
|
||
| public void injectMessage(final MessageData msgData) { | ||
| final DefaultMessage message = new DefaultMessage(null, msgData); | ||
| localNodeController.handleMessageEvent( |
There was a problem hiding this comment.
just a thought, but perhaps it would be better to put these on the ibft event queue instead through the controller directly. that make way it would be more complete from an integration point of view.
There was a problem hiding this comment.
The more we can fit in the better - but keeping it single threaded makes the test scripts very simple.
| public static TestContext createTestEnvironment( | ||
| final int validatorCount, final int indexOfFirstLocallyProposedBlock, final Clock clock) { | ||
|
|
||
| final NetworkLayout networkNodes = |
There was a problem hiding this comment.
this method is pretty nasty, haven't tried yet but is there any reasonable way to break it up a bit?
There was a problem hiding this comment.
Heh - this is after my first attempt to break it up. See what I can do.
|
|
||
| public class LocalNodeNotProposerTest { | ||
|
|
||
| // This ensures the localNode (i.e. the UUT) will not create the first block |
There was a problem hiding this comment.
probably just me, but is UUT?
There was a problem hiding this comment.
UUT - unit under test (but think this is now removed).
| assertPeersReceivedExactly(roles.getAllPeers(), expectedTxCommit); | ||
|
|
||
| // Ensure the local blockchain has NOT incremented yet. | ||
| assertThat(context.getBlockchain().getChainHeadBlockNumber()).isEqualTo(0); |
There was a problem hiding this comment.
an assert for the block height would be nice, this will probably be used a lot
There was a problem hiding this comment.
Context now exposes chainheight - good enough?
| public static boolean msgMatchesExpected( | ||
| final MessageData actual, final SignedData<? extends Payload> expected) { | ||
| final Payload expectedPayload = expected.getPayload(); | ||
| if (expectedPayload instanceof ProposalPayload) { |
There was a problem hiding this comment.
instead of using else if here we could use a switch here and use the message type for case. should make it bit easier to read.
something like
switch (expectedPayload.getMessageType()) {
case IbftV2.PROPOSAL: return ProposalMessage.fromMessage(actual).decode().equals(expected);
case IbftV2.PREPARE: return PrepareMessage.fromMessage(actual).decode().equals(expected);
...
| final MessageFactory messageFactory, | ||
| final IbftController localNodeController) { | ||
| this.nodeKeys = nodeParams.getNodeKeys(); | ||
| this.nodeAddress = nodeParams.getAddress(); |
There was a problem hiding this comment.
looks like nodeAddress is never used?
There was a problem hiding this comment.
Correct, but during testing it can be VERY helpful to be able to see this data.
| return address; | ||
| } | ||
|
|
||
| public KeyPair getNodeKeys() { |
There was a problem hiding this comment.
nit: getNodeKeyPair would be more accurate here. Sounds like the node has multiple key pairs otherwise.
| public static TestContext createTestEnvWithUtcClock( | ||
| final int validatorCount, final int indexOfFirstLocallyProposedBlock) { | ||
| return createTestEnvironment( | ||
| validatorCount, indexOfFirstLocallyProposedBlock, Clock.systemUTC()); |
There was a problem hiding this comment.
This is very much a knee-jerk reaction, but do we really have to support using a real clock in a test? It just seems to be asking for intermittency...
| .collect( | ||
| Collectors.toMap( | ||
| NodeParams::getAddress, | ||
| np -> |
There was a problem hiding this comment.
This would be a lot more readable if the np abbreviation wasn't used. We can't see the type info with lambdas so it's not particularly clear what np is short for.
To test the IBFT implementation at an integration level, an amount of "setup" is required. This commit creates a framework of "n" 'external' nodes which are able to inject messages to the IbftController, while also exposing the messages received from the local node. Thus integration tests are able to be written in terms of input events (including messages), expected (network) responses and changes in the blockchain state.
No description provided.