Ibft Json Acceptance tests #634
Conversation
...ests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/PantheonWeb3j.java
Outdated
Show resolved
Hide resolved
| "ibft_discardValidatorVote", singletonList(address), web3jService, DiscardResponse.class); | ||
| } | ||
|
|
||
| public Request<?, ProposalsResponse> ibftProposals() { |
There was a problem hiding this comment.
This doesn't really match up with the rpc method name. Original intent for the clique rpcs was that these method names corresponded to the underlying rpc method.
| cluster.waitUntil(removedCondition); | ||
| cluster.verify(ibft.validatorsEqual(validator1, validator2, validator3)); | ||
| } | ||
| } |
There was a problem hiding this comment.
A test to confirm that extra data has correct vote details would be nice
There was a problem hiding this comment.
Does that belong here? Or is that a lower level concept? I.e. do AcceptanceTests care how the vote gets around, or just that it does?
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static tech.pegasys.pantheon.tests.acceptance.dsl.WaitUtils.waitFor; | ||
| import static tech.pegasys.pantheon.tests.acceptance.dsl.transaction.clique.CliqueTransactions.LATEST; |
There was a problem hiding this comment.
we shouldn't be depending on this constant Clique even though it will work. This seems very generic LATEST could probably move somewhere more common.
| @Override | ||
| public List<Address> execute(final PantheonWeb3j node) { | ||
| try { | ||
| final SignersBlockResponse result = node.cliqueGetSignersAtHash(hash).send(); |
There was a problem hiding this comment.
This is using the clique signers getSignersAtHash
| import tech.pegasys.pantheon.tests.acceptance.dsl.node.Node; | ||
| import tech.pegasys.pantheon.tests.acceptance.dsl.transaction.ibft.IbftTransactions; | ||
|
|
||
| public class ExpectValidatorsAtBlockHash implements Condition { |
There was a problem hiding this comment.
Doesn't look like this is being used
| import tech.pegasys.pantheon.tests.acceptance.dsl.node.Node; | ||
| import tech.pegasys.pantheon.tests.acceptance.dsl.transaction.ibft.IbftTransactions; | ||
|
|
||
| public class ExpectValidatorsAtBlock implements Condition { |
There was a problem hiding this comment.
Doesn't look like this is being used
| private final EthTransactions eth; | ||
| private final IbftTransactions ibft; | ||
|
|
||
| public Ibft(final EthTransactions eth, final IbftTransactions ibft) { |
There was a problem hiding this comment.
Doesn't look like EthTransactions is used
| return new ExpectProposals(ibft, ImmutableMap.of()); | ||
| } | ||
|
|
||
| public ProposalsConfig proposalsEqual() { |
There was a problem hiding this comment.
Ibft is using pending votes instead of proposals for this concept, so I think it would be clearer in the ibft tests if we used the term pending votes
| SignersBlockResponse.class); | ||
| } | ||
|
|
||
| public Request<?, SignersBlockResponse> ibftGetSignersAtHash(final Hash hash) { |
There was a problem hiding this comment.
Doesn't look this is being used. But we I think we should have a test to verify this rpc is working.
| @Override | ||
| public Boolean execute(final JsonRequestFactories node) { | ||
| try { | ||
| final ProposeResponse result = node.ibft().ibftPropose(address, auth).send(); |
There was a problem hiding this comment.
If you're in the mood for sweeping renaming, with the addition of the split of the PanethonWeb3j into factories the method prefix becomes redundant and can be dropped ie. node.ibft().ibftPropose(address, auth).send(); -> node.ibft().propose(address, auth).send();
There was a problem hiding this comment.
I COMPLETELy agree with this, but would like to get this in first.
First drop of IBFT json Acceptances tests.
I blindly copied the Clique ones, and all associated Transactions etc. - so .... there might be quite a lot of room for finding commonalities.