Acceptance Test and DSL rename for IBFT2 #1493
Conversation
| @@ -42,10 +42,10 @@ | |||
| protected final Blockchain blockchain; | |||
| protected final Cluster cluster; | |||
| protected final CliqueTransactions cliqueTransactions; | |||
| protected final IbftTransactions ibftTransactions; | |||
| protected final IbftTwoTransactions ibftTwoTransactions; | |||
There was a problem hiding this comment.
Two seems like a cop-out - i.e. "I don't want to use "2" so this is the next best" - also generally worried that the word "Transaction" in the DSL is confusing wrt literal Ethereum Transactions ....
There was a problem hiding this comment.
Yes. Ibft2Transactions was the alternative ...however the 2 followed to a capital letter looks a bit funky.
There was a problem hiding this comment.
Term transaction can be changed, any suggestions?
There was a problem hiding this comment.
Actually prefer 2 instead of Two
...src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/ibft/ExpectProposals.java
Outdated
Show resolved
Hide resolved
...src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/condition/ibft/ExpectProposals.java
Outdated
Show resolved
Hide resolved
acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/jsonrpc/IbftTwo.java
Outdated
Show resolved
Hide resolved
| @@ -109,10 +109,10 @@ public WebSocketConfiguration createWebSocketEnabledConfig() { | |||
| } | |||
|
|
|||
| public JsonRpcConfiguration jsonRpcConfigWithAdmin() { | |||
There was a problem hiding this comment.
is this actually a useful abstraction?
There was a problem hiding this comment.
which part, the JsonRpcConfiguration or the PantheonNodeFactoryUtils?
...tance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/privacy/PrivacyNet.java
Outdated
Show resolved
Hide resolved
...ava/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/IbftTwoJsonRpcRequestFactory.java
Outdated
Show resolved
Hide resolved
| @@ -42,10 +42,10 @@ | |||
| protected final Blockchain blockchain; | |||
| protected final Cluster cluster; | |||
| protected final CliqueTransactions cliqueTransactions; | |||
| protected final IbftTransactions ibftTransactions; | |||
| protected final IbftTwoTransactions ibftTwoTransactions; | |||
There was a problem hiding this comment.
Actually prefer 2 instead of Two
...t/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/factory/PantheonNodeFactoryUtils.java
Show resolved
Hide resolved
|
Yeah, numbers in packages and classes I'm fine with ...but not in the variables names 😉 |
PR description
Tech debt, the IBFT suit of tests are actually not testing the Ibft algorithm but the Ibft2 algorithm (related, but not the same). Renaming the AT to avoid any potential confusion about what has / is covered by the AT.