Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Ibft Int Testing - Local Node is proposer #527

Merged
merged 10 commits into from
Jan 9, 2019

Conversation

rain-on
Copy link
Contributor

@rain-on rain-on commented Jan 8, 2019

PR description

Fixed Issue(s)

import org.junit.Test;

public class LocalNodeIsProposerTest {
// These tests assume the basic function of the Ibft Round State Machine has been proven
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this comment cannot be a class comment? (as it seems an appropriate level)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


@Before
public void setup() {
when(mockBlockCreationTime.millis()).thenReturn(blockTimeStamp * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blockTimeStamp * 1000 would be the 100 seconds ...what's the significance of that number?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So - as the local node is the proposer, it will create a block, with the timestamp as reported by the Clock (which is mocked in these tests).

In order to validate packets, the timestamp in the created (and transmitted) block must known - thus the expectedBlock has 'blockTimestamp' in it - and the clock which will provide a timestamp to the UUT is set to the same time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use Clock.fixed to created a clock for testing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so it's just about covering the blocTimeStamp to seconds ...because in that case you could useTimeUnit.MILLISECONDS.toSeconds(blocTimeStamp) and loose the magic number :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to clock.fixed, nicer than using Mocks if we don't need to.

assertPeersReceivedExactly(roles.getAllPeers(), expectedTxProposal);

// NOTE: In these test roles.getProposer() will return NULL.
roles.getNonProposingPeers().get(0).injectPrepare(roundId, expectedProposedBlock.getHash());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; for a readability gain, if you changed the signature of getNonProposingPeers()to accept an integer then getNonProposingPeers().get(0) could become getNonProposingPeer(0).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup - much nicer solution.


@Before
public void setup() {
when(mockBlockCreationTime.millis()).thenReturn(blockTimeStamp * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use Clock.fixed to created a clock for testing

expectedTxProposal =
localNodeMessageFactory.createSignedProposalPayload(roundId, expectedProposedBlock);

final IbftExtraData extraData =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps this could be moved into a common test util somewhere we are probably going to do this in many of the nodes. or perhaps just inside the createSignedCommitPayload using the proposedBlock as an arg to it?

Copy link
Contributor

@CjHare CjHare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rain-on rain-on merged commit c4b25b7 into PegaSysEng:master Jan 9, 2019
@rain-on rain-on deleted the int_test_happy branch January 16, 2019 21:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants