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

Refactoring for more readable IBFT IT #614

Merged
merged 16 commits into from
Jan 24, 2019
Merged

Refactoring for more readable IBFT IT #614

merged 16 commits into from
Jan 24, 2019

Conversation

CjHare
Copy link
Contributor

@CjHare CjHare commented Jan 21, 2019

A key goal of maintainable tests come from readability, any time the mental effort needed to understand the intent of the test, the better the chance of proper maintenance.

By removing the getter to the list of non proposing validators, I believe it creates a slightly nicer to read integration test.

this:

 roles
	        .getNonProposingPeers()
	        .forEach(
	            p -> {
	              p.injectPrepare(roundId, currentHeightBlock.getHash());
	              p.injectCommit(roundId, currentHeightBlock.getHash());
	            });

changes into:

	    roles.nonProposingPeersPrepare(roundId, currentHeightBlock.getHash());
	    roles.nonProposingPeerCommit(roundId, currentHeightBlock.getHash());

@CjHare CjHare requested review from rain-on and jframe January 21, 2019 06:18
@rain-on
Copy link
Contributor

rain-on commented Jan 21, 2019

The changes in the test are nice ... but not confident a thing called "Roles" should be generating msgs for the group ...

Maybe a rename needed? Or maybe a wrapping class?

GENERALLY - it'd be really nice if we could seed both the "assertMessagesSentToPeers" and a "Message Sender" with the peers, so we don't need to call them out explicitly ...

@CjHare
Copy link
Contributor Author

CjHare commented Jan 21, 2019

Ah yes, the fun of good naming!

Maybe a rename needed? Or maybe a wrapping class?

Probably a rename, as thnking about it, RoundSpecificNodeRoles is currently just the network of ValidatorPeers.
RoundSpecificValidatorPeers``, RoundSpecificNetworkandRoundSpecificPeers` sound like good candidates, any others?

"assertMessagesSentToPeers" and a "Message Sender" with the peers

This is a great idea, keeping both those functions in the same is grouping that aspect of responsibility together.

Copy link
Contributor

@rain-on rain-on left a comment

Choose a reason for hiding this comment

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

Definitely better than previous.

@CjHare CjHare merged commit 8d012a1 into PegaSysEng:master Jan 24, 2019
@CjHare CjHare deleted the ibft_it_framework_encapsulation branch January 24, 2019 23:25
rain-on pushed a commit to rain-on/pantheon that referenced this pull request Jan 29, 2019
* Refactoring for more readable IBFT IT

* Renaming Roles to peers

* Moving the assert behaviour into the RoundChangePeers

* Renmaing prefix of assert to verify, grammar

* Reducing usage of getAllPeers()

* Dropping the getter for the peer list

* Dropping peer from method names, as it's now in the class name

* Spotless
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