Unit tests for BlockReceiver effects #3741

stanislavlyalin · 2022-06-16T17:02:58Z

Overview

Unit-tests for BlockReceiver. Continuation of PR #3738, but for effects tests.

Since the interaction with the BlockReceiver performs via Streams/Queues, the tests checks if the desired blocks get into the output stream.

Most tests check that a block is dropped for various reasons: invalid hash or block signature or shard name, etc.

The last test checks the full logic of the BlockReceiver.

Please make sure that this PR:

is at most 200 lines of code (excluding tests),
meets RChain development coding standards,
includes tests for all added features,
has a reviewer assigned,
has all commits signed.

Bors cheat-sheet:

bors r+ runs integration tests and merges the PR (if it's approved),
bors try runs integration tests for the PR,
bors delegate+ enables non-maintainer PR authors to run the above.

casper/src/test/scala/coop/rchain/casper/blocks/proposer/BlockReceiverSpec.scala

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverSpec.scala

tgrospic · 2022-07-08T09:40:47Z

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverSpec.scala

+          bs.contains(*) wasCalled once
+          br.ackReceived(*) wasCalled once
+          bds.getRepresentation wasCalled twice
+          outList.length shouldBe 1


The title of the test says that correct block is passed to output stream and here it's only checking the length.

Together with that, we are interested in checking calls to effectful components so better to emphasize that in the title of the test.

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverSpec.scala

project/CompilerSettings.scala

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverSpec.scala

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala

tgrospic · 2022-07-08T20:52:05Z

@tgrospic, I have done changes related to you review. In some cases the last test fails with error message
[info] - should pass to output blocks with resolved dependencies *** FAILED *** (159 milliseconds)
[info]   java.lang.NullPointerException:
[info]   at flatMap @ coop.rchain.casper.blocks.BlockReceiver$.$anonfun$apply$22(BlockReceiver.scala:255)
[info]   at flatMap @ coop.rchain.casper.blocks.BlockReceiver$.$anonfun$apply$20(BlockReceiver.scala:252)
[info]   at map @ coop.rchain.casper.batch2.BlockReceiverEffectsSpec.$anonfun$blockStoreMock$1(BlockReceiverEffectsSpec.scala:167)
[info]   at map @ coop.rchain.store.KeyValueTypedStoreOps$.contains$extension(KeyValueTypedStoreSyntax.scala:44)
[info]   at map @ coop.rchain.catscontrib.BooleanF$.$tilde$up(BooleanF.scala:24)
[info]   at map @ coop.rchain.casper.blocks.BlockReceiver$.$anonfun$apply$18(BlockReceiver.scala:250)
[info]   at flatTraverse @ fs2.internal.CompileScope.$anonfun$lease$1(CompileScope.scala:346)
[info]   at flatTraverse @ fs2.internal.CompileScope.$anonfun$lease$1(CompileScope.scala:346)
[info]   at traverse @ coop.rchain.casper.blocks.BlockReceiver$.$anonfun$apply$17(BlockReceiver.scala:249)
[info]   at flatMap @ coop.rchain.casper.blocks.BlockReceiver$.$anonfun$apply$17(BlockReceiver.scala:248)
[info]   ...
Can it be related to using shared state outside of F-context?

rchain/casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala

Lines 165 to 172 in 173d981

val state = Ref.unsafe[F, Map[BlockHash, BlockMessage]](Map())

val bsMock = mock[BlockStore[F]]

bsMock.contains(*) answers { keys: Seq[BlockHash] =>

state.get.map(s => Seq(s.contains(keys.head)))

}

bsMock.put(*) answers { kvPairs: Seq[(BlockHash, BlockMessage)] =>

state.update(s => kvPairs.foldLeft(s) { case (acc, item) => acc + item })

}

or to concurrent code in BlockReceiver?

There is no shared state between test cases. withEnv instantiate new state for every call.

tgrospic · 2022-07-08T20:57:34Z

project/CompilerSettings.scala

+    scalacOptions in (Test, console) := (scalacOptions in (Compile, console)).value,
+    // To prevent "dead code following this construct" error when using * mockito-scala library
+    // https://github.com/mockito/mockito-scala/#dead-code-warning
+    scalacOptions in Test -= "-Ywarn-dead-code"


I cannot compile the project in IntelliJ because of dead code error in tests.

I found this old bug which would explain the behavior. I noticed that scalaOptions set for Compile and Test configuration are loaded differently in IntelliJ.
https://youtrack.jetbrains.com/issue/SCL-11824

I'll try to load separately options for Compile and Test configuration without using -= which doesn't have effect in IntelliJ.

I couldn't find a workaround, it seems that IntelliJ cannot use separate configuration for Test scope. This is another related issue I found.
https://youtrack.jetbrains.com/issue/IDEA-232043

@stanislavlyalin Let's disable dead code detection completely, this seems the only solution for now. Please add this additional information in the comments also.

I returned initial version of my code and added some related info.

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala

stanislavlyalin · 2022-07-07T12:14:44Z

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverSpec.scala

+        implicit val (bsImp, brImp, bdsImp) = (bs, br, bds)
+        BlockReceiver(state, incomingBlockStream, validatedBlocksStream, shardId)
+      }
+      res <- f(incomingBlockQueue, validatedBlocksQueue, blockReceiver, bs, br, bds)


Passing mocks to f directly is not beautiful solution but I cannot figure out how to declare that f should take implicit parameters 🤔

This is completely fine. Implicit parameters cannot be specified function parameter.

stanislavlyalin · 2022-07-11T10:13:59Z

project/CompilerSettings.scala

+    scalacOptions in (Test, console) := (scalacOptions in (Compile, console)).value,
+    // To prevent "dead code following this construct" error when using * mockito-scala library
+    // https://github.com/mockito/mockito-scala/#dead-code-warning
+    scalacOptions in Test -= "-Ywarn-dead-code"


I returned initial version of my code and added some related info.

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala

stanislavlyalin · 2022-07-11T10:43:22Z

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala

+      }
+  }
+
+  it should "pass to output blocks with resolved dependencies" in withEnv[Task]("root") {


There is problem with the test as we can see in log.
When running locally, the test passes in most cases, but fails when run on github.
I have been debugging it for 2 days, but have not yet reached the results, since the bug does not appear regularly. To detect it, I wrote a loop in the shell to run BlockReceiver tests, but sbt is initialized every time it runs, which is very long.

// The arrows show the lines of code that the log refers to. ... for { // Save block to block store, resolve parents to request _ <- BlockStore[F].put(block) parents <- block.justifications // <- .traverse { hash => BlockStore[F].contains(hash).not.map((hash, _)) // <- } pendingRequests <- state.modify(_.endStored(block.blockHash, parents)) // <- // Notify BlockRetriever of finished validation of block _ <- BlockRetriever[F].ackReceived(block.blockHash) // <- // Send request for missing dependencies _ <- requestMissingDependencies(pendingRequests) // Check if block have all dependencies in the DAG dag <- BlockDagStorage[F].getRepresentation hasAllDeps = pendingRequests.isEmpty && block.justifications.forall(dag.contains) _ <- receiverOutputQueue.enqueue1(block.blockHash).whenA(hasAllDeps) } yield () ...

Judging by the fact that the problem does not appear regularly, it is related to concurrent code execution, and the NullPointerException error says that some object we are accessing has been destroyed.
Judging by the log, either the block or the mocks are destroyed. I can't say for sure yet.

Hm, this can be a big issue. Did you find similar cases already reported with Mockito library?

The problem could something similar as described here.
https://www.javafixing.com/2022/03/fixed-mockito-nullpointerexception-when.html

Hm, this can be a big issue. Did you find similar cases already reported with Mockito library?

No, I never thought it can be error in Mockito library.

I resolved the problem. The problem was that the admitHash method was not implemented, although it was called in some scenarios. The Mockito library did not indicate this, but only threw a NullPointerException. I was able to spot the problem by manually implementing mocks and noticing that a NotImplementedError was fired on the admitHash method that had an implementation of ???.

Awesome catch! It would be nice that Mockito can tell which method is called in that case.

@tgrospic, I was happy to find that this is not a bug in Mockito as the library is very handy. It allows you to write less manual code and contains a large set of validation methods that read like natural language 🙂

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala

tgrospic

Excellent work! 🎉

tgrospic · 2022-07-12T12:26:38Z

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala

+      } yield {
+        val bsPutCaptor = ArgCaptor[Seq[(BlockHash, BlockMessage)]]
+        bs.put(bsPutCaptor) wasCalled twice
+        bsPutCaptor.values should contain allOf (Seq((a1.blockHash, a1)), Seq((a2.blockHash, a2)))


Can this be shortened in two lines?

bs.put(Seq((a1.blockHash, a1))) wasCalled once bs.put(Seq((a2.blockHash, a2))) wasCalled once

Although, I'd like to leave at least one use of ArgCaptor as an example.

@tgrospic, yes, it can be. Nice idea!

nzpr · 2022-07-14T06:39:23Z

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala

+  private def addBlock[F[_]: Sync](bs: BlockStore[F]): F[BlockMessage] =
+    for {
+      block <- Sync[F].delay(makeBlock())
+      _     <- bs.put(Seq((block.blockHash, block)))
+    } yield block


just a comment - this can be done shorter

private def addBlock[F[_]: Sync](bs: BlockStore[F]): F[BlockMessage] = { val block = makeBlock() bs.put(Seq((block.blockHash, block))).as(block) }

@nzpr Is this function now referentially transparent?

Let's imagine that makeBlock() is heavy operation and possibly do some logging to make this example more extreme.

What would be executed when this assignment is created in the old and new version of addBlock function?

val makeBlockInStore = addBlock(blockStore)

nzpr

Awesome!

stanislavlyalin · 2022-07-14T06:51:46Z

bors merge

bors · 2022-07-14T07:24:08Z

Build succeeded:

bors build finished

stanislavlyalin self-assigned this Jun 16, 2022

stanislavlyalin added the build-tests-ci label Jun 16, 2022

stanislavlyalin added this to In progress in Core team via automation Jun 16, 2022

stanislavlyalin added this to the Block merge (Hard Fork 2) milestone Jun 16, 2022

stanislavlyalin linked an issue Jun 16, 2022 that may be closed by this pull request

Tests for BlockReceiver #3723

Closed

2 tasks

stanislavlyalin force-pushed the block-receiver-tests branch 2 times, most recently from 6dca6f3 to 0f3fe38 Compare June 22, 2022 14:04

stanislavlyalin commented Jun 22, 2022

View reviewed changes

casper/src/test/scala/coop/rchain/casper/blocks/proposer/BlockReceiverSpec.scala Outdated Show resolved Hide resolved

stanislavlyalin marked this pull request as ready for review June 22, 2022 14:24

stanislavlyalin requested review from tgrospic and nzpr June 22, 2022 14:24

stanislavlyalin moved this from In progress to Review in progress in Core team Jun 22, 2022

tgrospic reviewed Jun 23, 2022

View reviewed changes

casper/src/test/scala/coop/rchain/casper/blocks/proposer/BlockReceiverSpec.scala Outdated Show resolved Hide resolved

casper/src/test/scala/coop/rchain/casper/blocks/proposer/BlockReceiverSpec.scala Outdated Show resolved Hide resolved

stanislavlyalin force-pushed the block-receiver-tests branch 2 times, most recently from 60255d3 to aa0a042 Compare July 5, 2022 07:31

stanislavlyalin force-pushed the block-receiver-tests branch from d01b03c to be26576 Compare July 6, 2022 10:12

stanislavlyalin requested a review from tgrospic July 7, 2022 12:10

tgrospic changed the title ~~Unit-tests for BlockReceiver~~ Unit tests for BlockReceiver effects Jul 8, 2022

tgrospic reviewed Jul 8, 2022

View reviewed changes

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverSpec.scala Outdated Show resolved Hide resolved

stanislavlyalin added 10 commits July 8, 2022 15:27

Added simple tests for BlockReceiver

e21951f

Added tests for invalid block hash and signature

b3afe4c

Used Fs2StreamMatchers to check that streams are never emitted

6abd0e6

Added test for discarding known (stored) block

987f65e

Added a test for passing blocks with solved dependencies

070cad0

Updated according to latest dev

4d7d318

Fixed block signing

96a21c6

Introduced mock for BlockDagStorage

e8f86bd

Mock for BlockDagStorage generates with Scala Mockito library

c79b7c0

Introduced mock for BlockRetriever

1b4d0f0

tgrospic reviewed Jul 8, 2022

View reviewed changes

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala Outdated Show resolved Hide resolved

tgrospic reviewed Jul 8, 2022

View reviewed changes

stanislavlyalin added 7 commits July 9, 2022 07:17

withEnv uses generic type F

bc042d5

Commented out failing test

1bf0ef0

Removed unnecessary parenthesis

8898f53

Removed unnecessary Time dependency

c48eb66

Used lastOrError function to take element from output stream

5727ddd

makeBlock returns value outside of F-context

08ccd6d

Optimized imports, uncommented test "discard known block"

8d9fdcc

tgrospic reviewed Jul 11, 2022

View reviewed changes

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala Outdated Show resolved Hide resolved

stanislavlyalin added 2 commits July 11, 2022 13:05

Ywarn-dead-code warning disabled for the main code, not only for tests

871a7bd

Removed Sync as Concurrent is a derivation of Sync

e7a401f

stanislavlyalin commented Jul 11, 2022

View reviewed changes

tgrospic reviewed Jul 11, 2022

View reviewed changes

casper/src/test/scala/coop/rchain/casper/batch2/BlockReceiverEffectsSpec.scala Show resolved Hide resolved

Fixed broken test: added implementation of BlockRetriever.admitHash

3d2ba33

stanislavlyalin force-pushed the block-receiver-tests branch from a3507fa to 3d2ba33 Compare July 12, 2022 07:42

stanislavlyalin requested a review from tgrospic July 12, 2022 08:21

tgrospic approved these changes Jul 12, 2022

View reviewed changes

tgrospic assigned nzpr and unassigned stanislavlyalin Jul 12, 2022

tgrospic reviewed Jul 12, 2022

View reviewed changes

Simplified checks of called methods and their arguments

e052b3e

nzpr reviewed Jul 14, 2022

View reviewed changes

nzpr approved these changes Jul 14, 2022

View reviewed changes

Simplified addBlock implementation

d2cb8b3

bors bot merged commit b293b98 into rchain:dev Jul 14, 2022

Core team automation moved this from Review in progress to Done Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests for BlockReceiver effects #3741

Unit tests for BlockReceiver effects #3741

stanislavlyalin commented Jun 16, 2022 •

edited

tgrospic Jul 8, 2022

tgrospic commented Jul 8, 2022

tgrospic Jul 8, 2022

tgrospic Jul 11, 2022

stanislavlyalin Jul 11, 2022

stanislavlyalin Jul 7, 2022

tgrospic Jul 11, 2022

stanislavlyalin Jul 11, 2022

stanislavlyalin Jul 11, 2022

tgrospic Jul 11, 2022

tgrospic Jul 11, 2022

stanislavlyalin Jul 11, 2022

stanislavlyalin Jul 12, 2022

tgrospic Jul 12, 2022

stanislavlyalin Jul 12, 2022

tgrospic left a comment

tgrospic Jul 12, 2022

stanislavlyalin Jul 12, 2022

nzpr Jul 14, 2022

tgrospic Jul 14, 2022

nzpr left a comment

stanislavlyalin commented Jul 14, 2022

bors bot commented Jul 14, 2022

Unit tests for BlockReceiver effects #3741

Unit tests for BlockReceiver effects #3741

Conversation

stanislavlyalin commented Jun 16, 2022 • edited

Overview

Please make sure that this PR:

Bors cheat-sheet:

Choose a reason for hiding this comment

tgrospic commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgrospic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzpr left a comment

Choose a reason for hiding this comment

stanislavlyalin commented Jul 14, 2022

bors bot commented Jul 14, 2022

stanislavlyalin commented Jun 16, 2022 •

edited