Skip to content

[Kaizen] Unflake RegularSynchSpec #1045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 9, 2021
Merged

[Kaizen] Unflake RegularSynchSpec #1045

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/test/scala/io/iohk/ethereum/blockchain/sync/regular/RegularSyncSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ class RegularSyncSpec
def sync[T <: Fixture](test: => T): Future[Assertion] =
Future {
test
// this makes sure that actors are all done after the test (believe me, afterEach does not work with mocks)
TestKit.shutdownActorSystem(testSystem)
Copy link
Contributor

@jvdp jvdp Jul 9, 2021

Choose a reason for hiding this comment

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

This 'fixture' pretty weird... are there any other tests with this?

Copy link
Contributor Author

@dzajkowski dzajkowski Jul 9, 2021

Choose a reason for hiding this comment

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

it's custom to this test, haven't seen it any other place.

succeed
}

Expand Down Expand Up @@ -475,7 +477,7 @@ class RegularSyncSpec
peersClient.setAutoPilot(new PeersClientAutoPilot)
override lazy val branchResolution: BranchResolution = stub[BranchResolution]
(blockchainReader.getBestBlockNumber _).when().returns(0)
(branchResolution.resolveBranch _).when(*).returns(NewBetterBranch(Nil)).repeat(10)
(branchResolution.resolveBranch _).when(*).returns(NewBetterBranch(Nil)).atLeastOnce()
Copy link
Contributor Author

@dzajkowski dzajkowski Jul 9, 2021

Choose a reason for hiding this comment

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

not convinced if this is required but the repeat(10) seems arbitrary.

Copy link
Contributor

@leo-bogastry leo-bogastry Jul 9, 2021

Choose a reason for hiding this comment

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

Indeed, atLeastOnce or anyNumberOf Times seems more appropriate. I just always wonder why does a method needs to be called so many times. Is it a recursive one?

Copy link
Contributor Author

@dzajkowski dzajkowski Jul 9, 2021

Choose a reason for hiding this comment

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

the logic in RegularSync will run a retry on failure. the failure is the missing node (vide a bit lower) which should result in some logic and a retry. the 10 used to be 1 a long time ago.

(blockImport
.importBlock(_: Block)(_: Scheduler))
.when(*, *)
Expand Down