Add TestKit P2 methods [ECR-3050] #833

MakarovS · 2019-04-11T04:32:06Z

Overview

Add TestKit P2 methods, as well as nativeFreeTestKit method.

Questions:
Should we give a link to documentation in every method that mentions pool?
Is there a way to avoid making frameworkInjector static?

See: https://jira.bf.local/browse/ECR-3050

Definition of Done

There are no TODOs left in the code
Change is covered by automated tests
The coding guidelines are followed
Public API has Javadoc
Method preconditions are checked and documented in the Javadoc of the method
Changelog is updated if needed (in case of notable or breaking changes)
The continuous integration build passes

vitvakatu

Every native method can return null in case of internal error. I think such errors should be handled in Java, but we can do the other way if you wish.

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

vitvakatu · 2019-04-11T07:44:44Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+   * Returns the emulated TestKit node context.
+   */
+  public EmulatedNode getEmulatedNode() {
+    return nativeGetEmulatedNode(nativeHandle.get());


returns null on error

vitvakatu · 2019-04-11T07:45:10Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+   */
+  public Block createBlockWithTransaction(TransactionMessage transaction) {
+    byte[][] transactionMessageArray = { transaction.toBytes() };
+    byte[] block = nativeCreateBlockWithTransactions(nativeHandle.get(), transactionMessageArray);


.... and this one

bullet-tooth · 2019-04-11T13:54:13Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+    return new TestKit(nativeHandle, serviceAdapters);
+  }
+
+  private TestKit(long nativeHandle, List<UserServiceAdapter> serviceAdapters) {


constructor should be before methods

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

bullet-tooth · 2019-04-11T14:01:03Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+   * @see <a href="https://exonum.com/doc/version/0.10/advanced/consensus/specification/#pool-of-unconfirmed-transactions">Pool of Unconfirmed Transactions</a>
+   */
+  public Block createBlockWithTransactions(Iterable<TransactionMessage> transactions) {
+    List<TransactionMessage> messageList = Lists.newArrayList(transactions);


this intermediate list can be replaced by com.google.common.collect.Streams#stream(java.lang.Iterable<T>)

Streams are marked as @Beta, is it okay for us?

No, for a library no.

That's the reason we repackaged HashCode and friends.

https://jira.bf.local/browse/ECR-1530

bullet-tooth · 2019-04-11T14:02:42Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+   * @see <a href="https://exonum.com/doc/version/0.10/advanced/consensus/specification/#pool-of-unconfirmed-transactions">Pool of Unconfirmed Transactions</a>
+   */
+  public Block createBlockWithTransaction(TransactionMessage transaction) {
+    byte[][] transactionMessageArray = { transaction.toBytes() };


please reuse the code

bullet-tooth · 2019-04-11T14:06:01Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+    return withSnapshot((view) -> {
+      Blockchain blockchain = Blockchain.newInstance(view);
+      MapIndex<HashCode, TransactionMessage> txMessages = blockchain.getTxMessages();
+      List<TransactionMessage> messageList = ImmutableList.copyOf(txMessages.values());


also, can be replaced by Stream.stream(txMessages.values())

dmitry-timofeev · 2019-04-11T14:53:45Z

Returns null on error

@vitvakatu,

Do these errors have any context (description)? If so, it would be appropriate to include it in the error message of an exception instead of returning null.
Under which circumstances can these errors occur?
Are these errors the client’s fault? If not, who is to blame in case of these errors?

dmitry-timofeev · 2019-04-11T14:55:28Z

Simply putting checkNotNull(ret) all over the place does not seem like a good idea, neither does throw new UnknownError("Have no idea what happened, but here you go")

coveralls · 2019-04-11T17:48:42Z

Coverage increased (+0.4%) to 85.794% when pulling 41a2d38 on ECR-3050 into 5c0e2b3 on master.

MakarovS · 2019-04-15T23:45:15Z

About native methods returning null - @vitvakatu correct me if I'm wrong, but you said that it can only happen on panic in core, in which case we will get a RuntimeException rather than null?

vitvakatu · 2019-04-17T05:43:47Z

@MakarovS yep

bullet-tooth · 2019-04-17T07:28:06Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/EmulatedNode.java

   *
-   * @param validatorId validator id of the validator node, less or equal to 0 in case of an
-   *     auditor node
+   * @param validatorId validator id of the validator node, less than 0 in case of an auditor node


how about:
identifier of the validator node; or any negative value if the node is an auditor

Also, take a look at #841. -1 is used as the special value for auditor node

bullet-tooth · 2019-04-17T07:47:23Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+      List<TransactionMessage> messages = ImmutableList.copyOf(txMessages.values());
+      return messages.stream()
+          .filter(predicate)
+          .filter(tx -> !txResults.containsKey(tx.hash()))


JFYI: recently found sugar methods in Predicate class such as java.util.function.Predicate#not

Have you searched our codebase for possible simplifications?

This is @since 11

... not the Guava Predicates#not though

bullet-tooth · 2019-04-17T07:59:54Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

      checkCorrectServiceNumber(services.size());
-      return new TestKit(services, nodeType, validatorCount, timeProvider);
+      if (nodeType == EmulatedNodeType.VALIDATOR) {
+        validatorCount += 1;


do we have a test for that?

I don't understand this code — please clarify.

I still don't think it is OK as this comment does not explain why this complication is needed. Why don't we just set the total number of validators, not additional, and assume "1" by default?

Ok, I'll change that. I thought it was easier for users to deal with additional validators and distinguish these additional ones as not really emulated. We would have to deal with a little more complicated internal code, but maybe it would make things a bit more straightforward for users.

I see, but the current code does not say that anywhere but in the docs, therefore, gets confusing. Are you saying that people need additional validators quite rarely as it changes almost nothing in testkit behaviour? If this distinction is important, we can leave it as is, but make it clear in names of identifiers.

bullet-tooth · 2019-04-17T08:26:13Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

+      return toMap(proofMapIndexProxy);
+    });
+    assertThat(map).hasSize(1);
+    String initialValue = map.get(TestService.INITIAL_ENTRY_KEY);


org.assertj.core.api.AbstractMapAssert#containsEntry

Or

Map<HashCode, String> expected = ImmutableMap.of(.., ...); assertThat(map).isEqualTo(expected);

dmitry-timofeev · 2019-04-17T12:32:11Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

  static final short MAX_SERVICE_NUMBER = 256;
-  private final Injector frameworkInjector = Guice.createInjector(new TestKitFrameworkModule());
+  private static final Serializer<Block> BLOCK_SERIALIZER = BlockSerializer.INSTANCE;
+  private static final Injector frameworkInjector =


Why is it static (same for all Testkits)?

dmitry-timofeev · 2019-04-17T12:38:27Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

    return serviceModules.stream()
-        .map(this::createUserServiceAdapter)
+        .map(TestKit::createUserServiceAdapter)
+        .peek(s -> checkForDuplicateService(s, serviceIds))


I would suggest to check a list separately in an independent operation, because such use of peek is discouraged (see the Javadoc) — it produces a side-effect elsewhere and relies on it.

We can do it alternatively like that

List<UserServiceAdapter> services = serviceModules.stream() .map(TestKit::createUserServiceAdapter) .collect(toList()); return Maps.uniqueIndex(services, UserServiceAdapter::getId);

this operation will produce an error message including as many duplicates as are in the list: "Multiple entries with same key: 3=bar and 3=foo". It however, won't be in terms of services but in terms of map entries, so our own method might be better if we consider such scenario as realistic.

dmitry-timofeev · 2019-04-17T12:40:47Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java


-  private native long nativeCreateTestKit(UserServiceAdapter[] services, boolean auditor,
+  /**
+   * Creates a block with the given transaction. In-pool transactions will be ignored.


Will they be ignored? Why? It seems deficient: can't I commit a block that includes both in-pool and some incoming transactions?

Currently no, but we could combine some core TestKit methods and add such functionality. Should I add such a method (that commits both input and in-pool transactions) or change this into one?

What are the implications (in terms of testkit usage to test services) of either way?

I'd add both ways, it seems useful to be able to create a block that consists of both in-pool and incoming transactions. We could use core add_tx method for that - add incoming transactions to the pool and then commit block with all in-pool transactions, but I remember from ECR-2972 there is some problem with add_tx in 0.11 - @vitvakatu is that still so? Does that prevent us from using this method?

I am not sure that answers the question. Why would anyone writing a test ignore the in-pool transactions?

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

dmitry-timofeev · 2019-04-17T12:42:29Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+   * Creates a block with the given transaction. In-pool transactions will be ignored.
+   *
+   * @return created block
+   * @see <a href="https://exonum.com/doc/version/0.10/advanced/consensus/specification/#pool-of-unconfirmed-transactions">Pool of Unconfirmed Transactions</a>


Regarding references, it is OK not to include in each operation, but it is required to document why this pool is important in operations of this class (at class level).

dmitry-timofeev · 2019-04-17T13:00:19Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

+      return toMap(proofMapIndexProxy);
+    });
+    assertThat(map).hasSize(1);
+    String initialValue = map.get(TestService.INITIAL_ENTRY_KEY);


Or

Map<HashCode, String> expected = ImmutableMap.of(.., ...); assertThat(map).isEqualTo(expected);

dmitry-timofeev · 2019-04-17T13:01:23Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

-    private static final TransactionConverter THROWING_TX_CONVERTER = (tx) -> {
-      throw new IllegalStateException("No transactions in this service: " + tx);
-    };
+    Block block = testKit.createBlock();


It is unclear why this is invoked.

We want to create a block so that afterCommit is invoked and we can test it (by verifying that it indeed submits a transaction). Added a comment

dmitry-timofeev · 2019-04-17T13:01:35Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java


-    static short SERVICE_ID = 46;
-    static String SERVICE_NAME = "Test service";
+    testKit.createBlock();


It is unclear why this is invoked.

We want to create a block so that afterCommit submits a transaction into the pool and when we create the block with specified transactions, it ignores this in-pool transaction - then we check that it is indeed still in the pool (together with the second afterCommit transaction). Added a comment

dmitry-timofeev · 2019-04-17T13:02:09Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

-    }
+  @Test
+  void createBlockWithTransaction() {
+    TestKit testKit = TestKit.forService(TestServiceModule.class);


Why do all these tests leak the native object?

dmitry-timofeev · 2019-04-17T13:02:59Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

+
+  @Test
+  void createBlockWithTransactionsVarargs() {
+    TestKit testKit = TestKit.forService(TestServiceModule.class);


The code below seems to be the same as above except the function. I'd consider de-duplicating.

dmitry-timofeev · 2019-04-22T12:13:33Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

      checkCorrectServiceNumber(services.size());
-      return new TestKit(services, nodeType, validatorCount, timeProvider);
+      if (nodeType == EmulatedNodeType.VALIDATOR) {
+        validatorCount += 1;


I still don't think it is OK as this comment does not explain why this complication is needed. Why don't we just set the total number of validators, not additional, and assume "1" by default?

dmitry-timofeev · 2019-04-22T12:14:33Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

+    testKit.withSnapshot((view) -> {
+      // Check that initialization changed database state
+      TestSchema testSchema = service.createDataSchema(view);
+      ProofMapIndexProxy<HashCode, String> proofMapIndexProxy = testSchema.testMap();


testProofMap for this and testMap for j.u.Map?

dmitry-timofeev · 2019-04-22T12:15:28Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

-      bind(TransactionConverter.class).toInstance(THROWING_TX_CONVERTER);
+  @Test
+  void afterCommitSubmitsTransaction() {
+    Block nextBlock;


(Here and elsewhere) Why don't we declare these variables inside try-catch and initialize immediately where they are needed?

dmitry-timofeev · 2019-04-22T12:17:16Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

+
+  @Test
+  void createBlockWithTransactionWithWrongServiceId() {
+    Class<RuntimeException> exceptionType = RuntimeException.class;


(1) The thrown exception type is on the wrong abstraction level, RuntimeException communicates nothing. Since it is a testkit,
I would expect either IllegalArgumentException or https://ota4j-team.github.io/opentest4j/docs/current/api/org/opentest4j/AssertionFailedError.html
(2) As we established in the requirements, the error messages must be user-friendly and tell exactly what
the problem is (or which problems are possible). In this case I think there are two possible problems:

A test developer passed an incorrectly serialized transaction (they intended to pass a proper one).
The implementation of the service is fine.

A test dev passed a correctly serialized transaction, but the implementation of the service
(particularly, TransactionConverter) does not handle that transaction correctly.

In both cases, the actual thrown exception and the details about the transaction message are quite useful
to understand what is broken.

Speaking of the implementation, it might be more convenient to check that each tx message is correct and create
a proper exception in Java, before passing them to the native code.

There might be a third case which we do not explicitly support — intentionally passing an invalid transaction. I think we shall either recommend testing that separately from the testkit (just take your TxConverter); or add first-class support (checkCannotDeserialize?). I think the first option is fine for now.

dmitry-timofeev · 2019-04-22T12:38:22Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

 * (with no real network setup). Although it is possible to add several validator nodes to this
- * network, only one node will create the service instances and will execute their operations
- * (e.g., {@link Service#afterCommit(BlockCommittedEvent)} method logic).
+ * network, only one node will create the service instances, provide access to its


this pool thing seems out of place, I'd move it in a separate sentence.

"
... only one node will create ..., execute their operations (...), and provide access to its state [here I mean Snapshots of any state].

Only the emulated node [but what in case it is an auditor?] has a pool of unconfirmed transactions where a service can submit new transaction messages through Node#submitTransaction; or the test code through #commitBlock. All transactions from the pool are committed when a new block is created [is that a reasonable default?].

What difference does an auditor node make? I don't see how it matters here, as we don't deal with consensus.

or the test code through #commitBlock

Could you expand on this - what commitBlock do you mean?

I don't know, ask because it is unclear if an auditor needs to have a pool, because it does not participate in consensus and can process only already committed transaction.

Any commitBlock in testkit that accepts transactions.

…ore sending to native

dmitry-timofeev · 2019-04-24T10:11:29Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+      RawTransaction rawTransaction = toRawTransaction(transactionMessage);
+      service.convertToTransaction(rawTransaction);
+    } catch (Throwable conversionError) {
+      String message = String.format("Transaction message (%s) is invalid", transactionMessage);


I specifically requested this error message to be actionable, and described the possible cases. Please fix the error message to reflect them.

dmitry-timofeev · 2019-04-24T10:12:42Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+      // As only executed transactions are stored in TxResults, it wouldn't contain in-pool
+      // transactions
+      ProofMapIndexProxy<HashCode, TransactionResult> txResults = blockchain.getTxResults();
+      List<TransactionMessage> messages = ImmutableList.copyOf(txMessages.values());


Why this code still iterates over all messages?

dmitry-timofeev · 2019-04-25T07:40:09Z

exonum-java-binding/testkit/src/main/java/com/exonum/binding/testkit/TestKit.java

+  }
+
+  private <T> Set<T> toSet(KeySetIndexProxy<T> setIndex) {
+    return Sets.newHashSet(setIndex.iterator());


I would suggest using LinkedHashSet to preserve iteration order (= the order in which txs will be committed).

Suggested change

return Sets.newHashSet(setIndex.iterator());

return Sets.newLinkedHashSet(setIndex.iterator());

Or, better — just stream them directly, with no intermediate copy — I'll push that

dmitry-timofeev · 2019-04-25T07:41:16Z

exonum-java-binding/testkit/src/test/java/com/exonum/binding/testkit/TestKitTest.java

+      RawTransaction rawTransaction = TestKit.toRawTransaction(message);
+      String expectedMessage = String.format("Unknown service id (%s) in transaction (%s)",
+          wrongServiceId, rawTransaction);
+      assertTrue(thrownException.getMessage().contains(expectedMessage));


Here and below: assertTrue shall not be used where matchers are available because you won't get expected/actual in case of failure in the output (assertThat ... contains|matches|equals)

MakarovS added 4 commits April 10, 2019 23:13

ECR-3050 Add TestKit P2 functionality

6659564

Merge remote-tracking branch 'remotes/origin/master' into ECR-3050

476ccd0

ECR-3050 Minor change in Javadocs

47a386d

ECR-3050 Minor documentation fix

44e0c29

MakarovS requested review from bullet-tooth, dmitry-timofeev and vitvakatu April 11, 2019 04:32

vitvakatu reviewed Apr 11, 2019

View reviewed changes

bullet-tooth reviewed Apr 11, 2019

View reviewed changes

MakarovS added 2 commits April 11, 2019 13:22

ECR-3050 Review and test fixes

346e352

Merge remote-tracking branch 'remotes/origin/master' into ECR-3050

ce3ba85

MakarovS added 2 commits April 11, 2019 19:28

ECR-3050 Make native create TestKit method static and fix tests

aa097de

ECR-3050 Fix TestKit tests

13e796e

MakarovS added 4 commits April 15, 2019 19:45

Merge remote-tracking branch 'remotes/origin/master' into ECR-3050

9324650

ECR-3050 Minor fix

f92ffe0

ECR-3050 Remove Guava Streams usage

0c3aff5

Merge remote-tracking branch 'remotes/origin/master' into ECR-3050

0e7060b

bullet-tooth approved these changes Apr 17, 2019

View reviewed changes

dmitry-timofeev suggested changes Apr 17, 2019

View reviewed changes

MakarovS added 3 commits April 22, 2019 00:06

ECR-3050 Review fixes and minor native fix

0fcd636

Merge remote-tracking branch 'remotes/origin/master' into ECR-3050

56db99e

ECR-3050 Remove Predicate#not

20b8085

dmitry-timofeev reviewed Apr 22, 2019

View reviewed changes

ECR-3050 Review fixes

cf325b1

MakarovS added 7 commits April 22, 2019 21:05

ECR-3050 Minor Javadoc fix

bd97500

ECR-3050 Minor Javadoc fix

0d83e00

Merge remote-tracking branch 'remotes/origin/master' into ECR-3050

9d22e30

Merge remote-tracking branch 'remotes/origin/master' into ECR-3050

e8ce82f

ECR-3050 Use Blockchain#getTransactionPool and check transactions bef…

0483d0e

…ore sending to native

ECR-3050 Check in-pool transactions as well

9b5e050

ECR-3050 Imports reorder

f093261

dmitry-timofeev suggested changes Apr 24, 2019

View reviewed changes

MakarovS added 4 commits April 24, 2019 14:21

ECR-3050 Review fixes

07440ec

ECR-3050 Checkstyle fix

db1e297

ECR-3050 Minor fix

053dd00

Merge remote-tracking branch 'remotes/origin/master' into ECR-3050

f0f95de

dmitry-timofeev reviewed Apr 25, 2019

View reviewed changes

dmitry-timofeev added 2 commits April 25, 2019 12:36

Stream Hashes from KeySet directly

e8a74e1

Use assertThat instead of assertTrue

41a2d38

dmitry-timofeev requested a review from bullet-tooth April 25, 2019 09:39

dmitry-timofeev approved these changes Apr 25, 2019

View reviewed changes

Merge branch 'master' into ECR-3050

ef2a71b

dmitry-timofeev added the Next to merge 🛬 The next PR to merge (that is up-to-date with master) label Apr 25, 2019

dmitry-timofeev merged commit a0f37a0 into master Apr 25, 2019

MakarovS deleted the ECR-3050 branch April 27, 2019 17:44

	return Sets.newHashSet(setIndex.iterator());
	return Sets.newLinkedHashSet(setIndex.iterator());

Add TestKit P2 methods [ECR-3050] #833

Add TestKit P2 methods [ECR-3050] #833

Uh oh!

Conversation

MakarovS commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

vitvakatu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-timofeev commented Apr 11, 2019

Uh oh!

dmitry-timofeev commented Apr 11, 2019

Uh oh!

coveralls commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MakarovS commented Apr 15, 2019

Uh oh!

vitvakatu commented Apr 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MakarovS commented Apr 11, 2019 •

edited

Loading

coveralls commented Apr 11, 2019 •

edited

Loading

MakarovS Apr 22, 2019 •

edited

Loading

MakarovS Apr 17, 2019 •

edited

Loading