[MINOR] junit5 for eth/sync tests (hyperledger#5214)

* junit5 for eth/sync tests
* refactor to remove unnecessary stubbings
* add comment to explain lack of setup

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

---------

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java

@@ -68,7 +68,7 @@
 import java.util.function.Supplier;
 
 import org.apache.tuweni.bytes.Bytes;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
 

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/BlockBroadcasterTest.java

@@ -34,7 +34,7 @@
 import java.util.Collections;
 import java.util.stream.Stream;
 
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
 
 public class BlockBroadcasterTest {
 

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/BonsaiBlockPropagationManagerTest.java

@@ -18,19 +18,19 @@
 import org.hyperledger.besu.ethereum.core.BlockchainSetupUtil;
 import org.hyperledger.besu.ethereum.worldstate.DataStorageFormat;
 
-import org.junit.Before;
-import org.junit.BeforeClass;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
 
 public class BonsaiBlockPropagationManagerTest extends AbstractBlockPropagationManagerTest {
 
   private static Blockchain fullBlockchain;
 
-  @BeforeClass
+  @BeforeAll
   public static void setupSuite() {
     fullBlockchain = BlockchainSetupUtil.forTesting(DataStorageFormat.BONSAI).importAllBlocks();
   }
 
-  @Before
+  @BeforeEach
   public void setup() {
     setup(DataStorageFormat.BONSAI);
   }

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/ChainHeadTrackerTest.java

@@ -32,16 +32,15 @@
 import org.hyperledger.besu.evm.internal.EvmConfiguration;
 import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem;
 
-import java.util.Arrays;
-import java.util.Collection;
+import java.util.stream.Stream;
 
 import org.assertj.core.api.Assertions;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.ArgumentsProvider;
+import org.junit.jupiter.params.provider.ArgumentsSource;
 
-@RunWith(Parameterized.class)
 public class ChainHeadTrackerTest {
 
   private BlockchainSetupUtil blockchainSetupUtil;
@@ -56,19 +55,15 @@ public class ChainHeadTrackerTest {
 
   private final TrailingPeerLimiter trailingPeerLimiter = mock(TrailingPeerLimiter.class);
 
-  @Parameterized.Parameters
-  public static Collection<Object[]> data() {
-    return Arrays.asList(new Object[][] {{DataStorageFormat.BONSAI}, {DataStorageFormat.FOREST}});
+  static class ChainHeadTrackerTestArguments implements ArgumentsProvider {
+    @Override
+    public Stream<? extends Arguments> provideArguments(final ExtensionContext context) {
+      return Stream.of(
+          Arguments.of(DataStorageFormat.BONSAI), Arguments.of(DataStorageFormat.FOREST));
+    }
   }
 
-  private final DataStorageFormat storageFormat;
-
-  public ChainHeadTrackerTest(final DataStorageFormat storageFormat) {
-    this.storageFormat = storageFormat;
-  }
-
-  @Before
-  public void setup() {
+  public void setup(final DataStorageFormat storageFormat) {
     blockchainSetupUtil = BlockchainSetupUtil.forTesting(storageFormat);
     blockchain = blockchainSetupUtil.getBlockchain();
     ethProtocolManager = EthProtocolManagerTestUtil.create(blockchain);
@@ -87,8 +82,11 @@ public void setup() {
             new NoOpMetricsSystem());
   }
 
-  @Test
-  public void shouldRequestHeaderChainHeadWhenNewPeerConnects() {
+  @ParameterizedTest
+  @ArgumentsSource(ChainHeadTrackerTestArguments.class)
+  public void shouldRequestHeaderChainHeadWhenNewPeerConnects(
+      final DataStorageFormat storageFormat) {
+    setup(storageFormat);
     final Responder responder =
         RespondingEthPeer.blockchainResponder(
             blockchainSetupUtil.getBlockchain(),
@@ -104,8 +102,11 @@ public void shouldRequestHeaderChainHeadWhenNewPeerConnects() {
         .isEqualTo(blockchain.getChainHeadBlockNumber());
   }
 
-  @Test
-  public void shouldIgnoreHeadersIfChainHeadHasAlreadyBeenUpdatedWhileWaiting() {
+  @ParameterizedTest
+  @ArgumentsSource(ChainHeadTrackerTestArguments.class)
+  public void shouldIgnoreHeadersIfChainHeadHasAlreadyBeenUpdatedWhileWaiting(
+      final DataStorageFormat storageFormat) {
+    setup(storageFormat);
     final Responder responder =
         RespondingEthPeer.blockchainResponder(
             blockchainSetupUtil.getBlockchain(),
@@ -121,8 +122,10 @@ public void shouldIgnoreHeadersIfChainHeadHasAlreadyBeenUpdatedWhileWaiting() {
     Assertions.assertThat(chainHeadState().getEstimatedHeight()).isZero();
   }
 
-  @Test
-  public void shouldCheckTrialingPeerLimits() {
+  @ParameterizedTest
+  @ArgumentsSource(ChainHeadTrackerTestArguments.class)
+  public void shouldCheckTrialingPeerLimits(final DataStorageFormat storageFormat) {
+    setup(storageFormat);
     final Responder responder =
         RespondingEthPeer.blockchainResponder(
             blockchainSetupUtil.getBlockchain(),

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/DownloadHeadersStepTest.java

@@ -38,9 +38,9 @@
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
 
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 
 public class DownloadHeadersStepTest {
 
@@ -54,7 +54,7 @@ public class DownloadHeadersStepTest {
   private DownloadHeadersStep downloader;
   private SyncTargetRange checkpointRange;
 
-  @BeforeClass
+  @BeforeAll
   public static void setUpClass() {
     final BlockchainSetupUtil setupUtil = BlockchainSetupUtil.forTesting(DataStorageFormat.FOREST);
     setupUtil.importFirstBlocks(20);
@@ -63,7 +63,7 @@ public static void setUpClass() {
     blockchain = protocolContext.getBlockchain();
   }
 
-  @Before
+  @BeforeEach
   public void setUp() {
     ethProtocolManager = EthProtocolManagerTestUtil.create(blockchain);
     downloader =

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/ForestBlockPropagationManagerTest.java

@@ -18,19 +18,19 @@
 import org.hyperledger.besu.ethereum.core.BlockchainSetupUtil;
 import org.hyperledger.besu.ethereum.worldstate.DataStorageFormat;
 
-import org.junit.Before;
-import org.junit.BeforeClass;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
 
 public class ForestBlockPropagationManagerTest extends AbstractBlockPropagationManagerTest {
 
   private static Blockchain fullBlockchain;
 
-  @BeforeClass
+  @BeforeAll
   public static void setupSuite() {
     fullBlockchain = BlockchainSetupUtil.forTesting(DataStorageFormat.FOREST).importAllBlocks();
   }
 
-  @Before
+  @BeforeEach
   public void setup() {
     setup(DataStorageFormat.FOREST);
   }

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/PipelineChainDownloaderTest.java

@@ -42,14 +42,14 @@
 import java.util.concurrent.ExecutionException;
 import java.util.function.Supplier;
 
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
 import org.mockito.Mockito;
-import org.mockito.junit.MockitoJUnitRunner;
+import org.mockito.junit.jupiter.MockitoExtension;
 
-@RunWith(MockitoJUnitRunner.class)
+@ExtendWith(MockitoExtension.class)
 public class PipelineChainDownloaderTest {
 
   @Mock private SyncTargetManager syncTargetManager;
@@ -65,7 +65,7 @@ public class PipelineChainDownloaderTest {
   private SyncTarget syncTarget2;
   private PipelineChainDownloader chainDownloader;
 
-  @Before
+  @BeforeEach
   public void setUp() {
     syncTarget = new SyncTarget(peer1, commonAncestor);
     syncTarget2 = new SyncTarget(peer2, commonAncestor);
@@ -76,8 +76,6 @@ public void setUp() {
             downloadPipelineFactory,
             scheduler,
             new NoOpMetricsSystem());
-
-    immediatelyCompletePauseAfterError();
   }
 
   @Test
@@ -119,6 +117,7 @@ public void shouldUpdateSyncStateWhenTargetSelected() {
 
   @Test
   public void shouldRetryWhenSyncTargetSelectionFailsAndSyncTargetManagerShouldContinue() {
+    immediatelyCompletePauseAfterError();
     final CompletableFuture<SyncTarget> selectTargetFuture = new CompletableFuture<>();
     when(syncTargetManager.shouldContinueDownloading()).thenReturn(true);
     when(syncTargetManager.findSyncTarget())
@@ -152,7 +151,8 @@ public void shouldBeCompleteWhenSyncTargetSelectionFailsAndSyncTargetManagerShou
 
   @Test
   public void shouldBeCompleteWhenPipelineCompletesAndSyncTargetManagerShouldNotContinue() {
-    final CompletableFuture<Void> pipelineFuture = expectPipelineStarted(syncTarget);
+    final CompletableFuture<Void> pipelineFuture = new CompletableFuture<>();
+    when(syncTargetManager.findSyncTarget()).thenReturn(completedFuture(syncTarget));
     final CompletableFuture<Void> result = chainDownloader.start();
 
     verify(syncTargetManager).findSyncTarget();
@@ -259,6 +259,7 @@ public void shouldAbortPipelineIfCancelledAfterDownloadStarts() {
 
   @Test
   public void shouldRetryIfNotCancelledAndPipelineTaskThrowsException() {
+    immediatelyCompletePauseAfterError();
     when(syncTargetManager.shouldContinueDownloading()).thenReturn(true);
 
     final CompletableFuture<Void> pipelineFuture1 = new CompletableFuture<>();
@@ -268,7 +269,6 @@ public void shouldRetryIfNotCancelledAndPipelineTaskThrowsException() {
         .thenReturn(completedFuture(syncTarget))
         .thenReturn(completedFuture(syncTarget2));
 
-    expectPipelineCreation(syncTarget, downloadPipeline, pipelineFuture1);
     expectPipelineCreation(syncTarget2, downloadPipeline2, pipelineFuture2);
 
     final CompletableFuture<Void> result = chainDownloader.start();
@@ -279,10 +279,7 @@ public void shouldRetryIfNotCancelledAndPipelineTaskThrowsException() {
             "Async operation failed", new ExecutionException(new CancellationException()));
     pipelineFuture1.completeExceptionally(taskException);
 
-    assertThat(result).isNotDone();
-
     // Second time round, there are no task errors in the pipeline, and the download can complete.
-    when(syncTargetManager.shouldContinueDownloading()).thenReturn(false);
     pipelineFuture2.complete(null);
 
     assertThat(result).isDone();
@@ -312,7 +309,6 @@ public void shouldDisconnectSyncTargetOnInvalidBlockException_notFinishedDownloa
   public void testInvalidBlockHandling(
       final boolean isFinishedDownloading, final boolean isCancelled) {
     final CompletableFuture<SyncTarget> selectTargetFuture = new CompletableFuture<>();
-    when(syncTargetManager.shouldContinueDownloading()).thenReturn(isFinishedDownloading);
     when(syncTargetManager.findSyncTarget())
         .thenReturn(selectTargetFuture)
         .thenReturn(new CompletableFuture<>());

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/RangeHeadersFetcherTest.java

@@ -43,10 +43,10 @@
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.junit.MockitoJUnitRunner;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.junit.jupiter.MockitoExtension;
 
-@RunWith(MockitoJUnitRunner.class)
+@ExtendWith(MockitoExtension.class)
 public class RangeHeadersFetcherTest {
 
   private static final int SEGMENT_SIZE = 5;

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/RangeHeadersValidationStepTest.java

@@ -38,13 +38,12 @@
 import java.util.List;
 import java.util.stream.Stream;
 
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
+import org.mockito.junit.jupiter.MockitoExtension;
 
-@RunWith(MockitoJUnitRunner.class)
+@ExtendWith(MockitoExtension.class)
 public class RangeHeadersValidationStepTest {
   @Mock private ProtocolSchedule protocolSchedule;
   @Mock private ProtocolSpec protocolSpec;
@@ -64,7 +63,6 @@ public class RangeHeadersValidationStepTest {
           new SyncTargetRange(syncTarget, rangeStart, rangeEnd),
           asList(firstHeader, gen.header(12), rangeEnd));
 
-  @Before
   public void setUp() {
     when(protocolSchedule.getByBlockHeader(any(BlockHeader.class))).thenReturn(protocolSpec);
     when(protocolSpec.getBlockHeaderValidator()).thenReturn(headerValidator);
@@ -76,6 +74,7 @@ public void setUp() {
 
   @Test
   public void shouldValidateFirstHeaderAgainstRangeStartHeader() {
+    setUp();
     when(headerValidator.validateHeader(firstHeader, rangeStart, protocolContext, DETACHED_ONLY))
         .thenReturn(true);
     final Stream<BlockHeader> result = validationStep.apply(rangeHeaders);
@@ -90,6 +89,7 @@ public void shouldValidateFirstHeaderAgainstRangeStartHeader() {
 
   @Test
   public void shouldThrowExceptionWhenValidationFails() {
+    setUp();
     when(headerValidator.validateHeader(firstHeader, rangeStart, protocolContext, DETACHED_ONLY))
         .thenReturn(false);
     assertThatThrownBy(() -> validationStep.apply(rangeHeaders))
@@ -111,7 +111,10 @@ public void shouldThrowExceptionWhenValidationFails() {
   }
 
   @Test
-  public void acceptResponseWithNoHeaders() {
+  public void acceptResponseWithNoHeadersAndNoSetUp() {
+    // don't run the setUp
+    validationStep =
+        new RangeHeadersValidationStep(protocolSchedule, protocolContext, validationPolicy);
     var emptyRangeHeaders =
         new RangeHeaders(new SyncTargetRange(syncTarget, rangeStart, rangeEnd), List.of());
 

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/SyncTargetRangeSourceTest.java

@@ -43,7 +43,7 @@
 import java.util.concurrent.TimeoutException;
 import java.util.function.Supplier;
 
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
 
 public class SyncTargetRangeSourceTest {
 

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/TrailingPeerLimiterTest.java

@@ -37,8 +37,8 @@
 import java.util.Collections;
 import java.util.List;
 
-import org.junit.Before;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 
 public class TrailingPeerLimiterTest {
 
@@ -54,7 +54,7 @@ public class TrailingPeerLimiterTest {
               new TrailingPeerRequirements(
                   CHAIN_HEAD - TRAILING_PEER_BLOCKS_BEHIND_THRESHOLD, MAX_TRAILING_PEERS));
 
-  @Before
+  @BeforeEach
   public void setUp() {
     when(ethPeers.streamAvailablePeers()).then(invocation -> peers.stream());
   }