Simplified Drummer somewhat and fixed a bug in determining next beat.

Re-engineered Drummer unit test to cover more cases.
Fixed component test so that it can "see" the bug.

component-test/src/main/java/io/mifos/rhythm/AbstractRhythmTest.java

@@ -198,7 +198,7 @@ Beat createBeat(
 
     this.testSubject.createBeat(applicationIdentifier, beat);
 
-    Assert.assertTrue(this.eventRecorder.wait(EventConstants.POST_BEAT, new BeatEvent(applicationIdentifier, beat.getIdentifier())));
+    Assert.assertTrue(beat.getIdentifier(), this.eventRecorder.wait(EventConstants.POST_BEAT, new BeatEvent(applicationIdentifier, beat.getIdentifier())));
 
     Mockito.verify(beatPublisherServiceMock, Mockito.timeout(2_500).times(1)).requestPermissionForBeats(tenantIdentifier, applicationIdentifier);
 

component-test/src/main/java/io/mifos/rhythm/TestBeats.java

@@ -26,6 +26,7 @@
 import org.junit.Test;
 import org.mockito.Matchers;
 import org.mockito.Mockito;
+import org.mockito.internal.stubbing.answers.Returns;
 import org.springframework.beans.factory.annotation.Autowired;
 
 import javax.transaction.Transactional;
@@ -147,7 +148,7 @@ public void shouldBeatForMissingDays() throws InterruptedException {
               beatIdentifier,
               tenantDataStoreContext.getTenantName(),
               applicationIdentifier,
-              nextBeat.minusDays(daysAgo));
+              nextBeat.minusDays(i));
     }
   }
 
@@ -189,6 +190,7 @@ public void clockOffsetShouldEffectBeatTiming() throws InterruptedException {
 
     Mockito.verify(beatPublisherServiceMock, Mockito.timeout(10_000).times(1)).publishBeat(beatIdentifier, tenantIdentifier, applicationIdentifier, expectedBeatTimestamp);
 
+    //Set back to zero'ed clock offset so you don't break the rest of the tests.
     this.testSubject.setClockOffset(initialClockOffset);
     Assert.assertTrue(this.eventRecorder.wait(EventConstants.PUT_CLOCKOFFSET, initialClockOffset));
   }
@@ -205,6 +207,12 @@ LocalDateTime setBack(
         beatIdentifier).orElseThrow(IllegalStateException::new);
 
     Mockito.reset(beatPublisherServiceMock);
+    Mockito.doAnswer(new Returns(true)).when(beatPublisherServiceMock)
+        .publishBeat(
+            Matchers.eq(beatIdentifier),
+            Matchers.eq(tenantDataStoreContext.getTenantName()),
+            Matchers.eq(applicationIdentifier),
+            Matchers.any(LocalDateTime.class));
     final LocalDateTime nextBeat = beatEntity.getNextBeat();
 
     beatEntity.setNextBeat(nextBeat.minusDays(daysAgo));

service/src/main/java/io/mifos/rhythm/service/internal/service/Drummer.java

@@ -27,11 +27,9 @@
 import org.springframework.stereotype.Component;
 import org.springframework.transaction.annotation.Transactional;
 
-import javax.annotation.Nonnull;
 import java.time.Clock;
 import java.time.LocalDateTime;
 import java.time.temporal.ChronoUnit;
-import java.util.Optional;
 import java.util.function.Predicate;
 import java.util.stream.Stream;
 
@@ -78,17 +76,17 @@ public synchronized void checkForBeatsNeeded() {
         }
         else {
           logger.info("Checking if beat {} needs publishing.", beat);
-          final Optional<LocalDateTime> nextBeat = checkBeatForPublish(
+          final LocalDateTime nextBeat = checkBeatForPublish(
                   now,
                   beat.getBeatIdentifier(),
                   beat.getTenantIdentifier(),
                   beat.getApplicationIdentifier(),
                   beat.getAlignmentHour(),
                   beat.getNextBeat());
-          nextBeat.ifPresent(y -> {
-            beat.setNextBeat(y);
+          if (!nextBeat.equals(beat.getNextBeat())) {
+            beat.setNextBeat(nextBeat);
             beatRepository.save(beat);
-          });
+          }
           logger.info("Beat updated to {}.", beat);
         }
       });
@@ -100,7 +98,7 @@ public synchronized void checkForBeatsNeeded() {
     }
   }
 
-  private Optional<LocalDateTime> checkBeatForPublish(
+  private LocalDateTime checkBeatForPublish(
       final LocalDateTime now,
       final String beatIdentifier,
       final String tenantIdentifier,
@@ -113,33 +111,22 @@ private Optional<LocalDateTime> checkBeatForPublish(
   }
 
   //Helper is separated from original function so that it can be unit-tested separately from publishBeat.
-  static Optional<LocalDateTime> checkBeatForPublishHelper(
+  static LocalDateTime checkBeatForPublishHelper(
           final LocalDateTime now,
           final Integer alignmentHour,
           final LocalDateTime nextBeat,
           final ClockOffset clockOffset,
           final Predicate<LocalDateTime> publishSucceeded) {
-    final long numberOfBeatPublishesNeeded = getNumberOfBeatPublishesNeeded(now, nextBeat);
-    if (numberOfBeatPublishesNeeded == 0)
-      return Optional.empty();
-
-    final Optional<LocalDateTime> firstFailedBeat = Stream.iterate(nextBeat,
-            x -> incrementToAlignment(x, alignmentHour, clockOffset))
-            .limit(numberOfBeatPublishesNeeded)
-            .filter(x -> !publishSucceeded.test(x))
-            .findFirst();
-
-    if (firstFailedBeat.isPresent())
-      return firstFailedBeat;
-    else
-      return Optional.of(incrementToAlignment(now, alignmentHour, clockOffset));
-  }
-
-  static long getNumberOfBeatPublishesNeeded(final LocalDateTime now, final @Nonnull LocalDateTime nextBeat) {
-    if (nextBeat.isAfter(now))
-      return 0;
+    LocalDateTime beatToPublish = nextBeat;
+    for (;
+         !beatToPublish.isAfter(now);
+         beatToPublish = incrementToAlignment(beatToPublish, alignmentHour, clockOffset))
+    {
+      if (!publishSucceeded.test(beatToPublish))
+        break;
+    }
 
-    return Math.max(1, nextBeat.until(now, ChronoUnit.DAYS));
+    return beatToPublish;
   }
 
   static LocalDateTime incrementToAlignment(

service/src/test/java/io/mifos/rhythm/service/internal/service/DrummerTest.java

@@ -18,101 +18,188 @@
 import io.mifos.rhythm.api.v1.domain.ClockOffset;
 import org.junit.Assert;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 import org.mockito.Mockito;
 
 import java.time.Clock;
 import java.time.LocalDateTime;
 import java.time.temporal.ChronoUnit;
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashSet;
-import java.util.Optional;
 import java.util.Set;
 import java.util.function.Predicate;
 
 /**
  * @author Myrle Krantz
  */
+@RunWith(Parameterized.class)
 public class DrummerTest {
+  static class TestCase {
+    final String description;
+    LocalDateTime now = LocalDateTime.now(Clock.systemUTC());
+    LocalDateTime nextBeat = now.minusDays(1).truncatedTo(ChronoUnit.DAYS);
+    int alignmentHour = 0;
+    ClockOffset clockOffset = new ClockOffset();
+    LocalDateTime expectedIncrementedBeat = nextBeat.plusDays(1);
+    LocalDateTime expectedNextBeatAfterPublish = now.plusDays(1).truncatedTo(ChronoUnit.DAYS);
+    int expectedBeatPublishCount = 2;
 
-  @Test
-  public void incrementToAlignment() {
-    final LocalDateTime now = LocalDateTime.now(Clock.systemUTC());
-    final LocalDateTime tomorrow = Drummer.incrementToAlignment(now, 3, new ClockOffset());
+    TestCase(final String description) {
+      this.description = description;
+    }
+
+    TestCase now(final LocalDateTime now) {
+      this.now = now;
+      return this;
+    }
+
+    TestCase nextBeat(final LocalDateTime newVal) {
+      this.nextBeat = newVal;
+      return this;
+    }
+
+    TestCase alignmentHour(final int newVal) {
+      this.alignmentHour = newVal;
+      return this;
+    }
+
+    TestCase clockOffset(final ClockOffset newVal) {
+      this.clockOffset = newVal;
+      return this;
+    }
 
-    Assert.assertEquals(tomorrow.minusDays(1).truncatedTo(ChronoUnit.DAYS), now.truncatedTo(ChronoUnit.DAYS));
-    Assert.assertEquals(3, tomorrow.getHour());
+    TestCase expectedIncrementedBeat(final LocalDateTime newVal) {
+      this.expectedIncrementedBeat = newVal;
+      return this;
+    }
+
+    TestCase expectedNextBeatAfterPublish(final LocalDateTime newVal) {
+      this.expectedNextBeatAfterPublish = newVal;
+      return this;
+    }
+
+    TestCase expectedBeatPublishCount(final int newVal) {
+      this.expectedBeatPublishCount = newVal;
+      return this;
+    }
   }
 
-  @Test
-  public void getNumberOfBeatPublishesNeeded() {
-    final LocalDateTime now = LocalDateTime.now(Clock.systemUTC());
-    final long eventsNeeded3 = Drummer.getNumberOfBeatPublishesNeeded(now, now.minus(3, ChronoUnit.DAYS));
-    Assert.assertEquals(3, eventsNeeded3);
+  @Parameterized.Parameters
+  public static Collection testCases() {
+    final Collection<TestCase> ret = new ArrayList<>();
+    final TestCase basicCase = new TestCase("basicCase");
+    ret.add(basicCase);
+    ret.add(new TestCase("3daysBack")
+        .nextBeat(basicCase.now.minusDays(3))
+        .expectedIncrementedBeat(basicCase.now.minusDays(2).truncatedTo(ChronoUnit.DAYS))
+        .expectedBeatPublishCount(4)); //Four because "now" gets published too.
+    ret.add(new TestCase("inFuture")
+        .nextBeat(basicCase.now.plusDays(1))
+        .expectedIncrementedBeat(basicCase.now.plusDays(2).truncatedTo(ChronoUnit.DAYS))
+        .expectedNextBeatAfterPublish(basicCase.now.plusDays(1))
+        .expectedBeatPublishCount(0));
+    ret.add(new TestCase("nonZeroClockOffset")
+        .now(LocalDateTime.of(2017, 12, 18, 15, 5, 2))
+        .nextBeat(LocalDateTime.of(2017, 12, 17, 15, 0, 0))
+        .clockOffset(new ClockOffset(15, 5, 2))
+        .expectedIncrementedBeat(LocalDateTime.of(2017, 12, 18, 15, 5, 2))
+        .expectedNextBeatAfterPublish(LocalDateTime.of(2017, 12, 19, 15, 5, 2))
+        .expectedBeatPublishCount(2));
+    ret.add(new TestCase("nonZeroAlignmentHour")
+        .now(LocalDateTime.of(2017, 12, 18, 15, 5, 2))
+        .nextBeat(LocalDateTime.of(2017, 12, 17, 15, 0, 0))
+        .alignmentHour(4)
+        .expectedIncrementedBeat(LocalDateTime.of(2017, 12, 18, 4, 0, 0))
+        .expectedNextBeatAfterPublish(LocalDateTime.of(2017, 12, 19, 4, 0, 0))
+        .expectedBeatPublishCount(2));
+    ret.add(new TestCase("clockOffsetAndAlignmentHour")
+        .now(LocalDateTime.of(2017, 12, 18, 15, 5, 2))
+        .nextBeat(LocalDateTime.of(2017, 12, 17, 15, 0, 0))
+        .alignmentHour(5)
+        .clockOffset(new ClockOffset(15, 5, 2))
+        .expectedIncrementedBeat(LocalDateTime.of(2017, 12, 18, 20, 5, 2))
+        .expectedNextBeatAfterPublish(LocalDateTime.of(2017, 12, 18, 20, 5, 2))
+        .expectedBeatPublishCount(1));
+    return ret;
+  }
 
-    final long eventsNeededPast = Drummer.getNumberOfBeatPublishesNeeded(now, now.plus(1, ChronoUnit.DAYS));
-    Assert.assertEquals(0, eventsNeededPast);
+  private TestCase testCase;
 
-    final long eventsNeededNow = Drummer.getNumberOfBeatPublishesNeeded(now, now.minus(2, ChronoUnit.MINUTES));
-    Assert.assertEquals(1, eventsNeededNow);
+  public DrummerTest(final TestCase testCase) {
+    this.testCase = testCase;
   }
 
   @Test
-  public void checkBeatForPublishAllBeatsSucceed() {
-    final LocalDateTime now = LocalDateTime.now(Clock.systemUTC());
+  public void incrementToAlignment() {
+    final LocalDateTime incrementedBeat = Drummer.incrementToAlignment(
+        testCase.nextBeat,
+        testCase.alignmentHour,
+        testCase.clockOffset);
+
+    Assert.assertEquals(
+        "expectedIncrementedBeat",
+        testCase.expectedIncrementedBeat,
+        incrementedBeat);
+  }
+
+  @Test
+  public void checkBeatForPublishHelper()
+  {
     final Set<LocalDateTime> calledForTimes = new HashSet<>();
-    final Optional<LocalDateTime> ret = Drummer.checkBeatForPublishHelper(
-        now,
-        0,
-        now.minus(3, ChronoUnit.DAYS),
-        new ClockOffset(),
+    final LocalDateTime nextBeatAfterPublish = Drummer.checkBeatForPublishHelper(
+        testCase.now,
+        testCase.alignmentHour,
+        testCase.nextBeat,
+        testCase.clockOffset,
         x -> {
           calledForTimes.add(x);
           return true;
         });
-    Assert.assertEquals(Optional.of(Drummer.incrementToAlignment(now, 0, new ClockOffset())), ret);
-    Assert.assertEquals(3, calledForTimes.size());
+    Assert.assertEquals(
+        "expectedNextBeatAfterPublish",
+        testCase.expectedNextBeatAfterPublish,
+        nextBeatAfterPublish);
+    Assert.assertEquals(
+        "expectedBeatPublishCount",
+        testCase.expectedBeatPublishCount,
+        calledForTimes.size());
+
   }
 
   @Test
-  public void checkBeatForPublishFirstFails() {
-    final LocalDateTime now = LocalDateTime.now(Clock.systemUTC());
-    final LocalDateTime nextBeat = now.minus(3, ChronoUnit.DAYS);
-    @SuppressWarnings("unchecked") final Predicate<LocalDateTime> produceBeatsMock = Mockito.mock(Predicate.class);
-    Mockito.when(produceBeatsMock.test(nextBeat)).thenReturn(false);
-    final Optional<LocalDateTime> ret = Drummer.checkBeatForPublishHelper(
-        now,
-        0,
-        nextBeat,
-        new ClockOffset(),
+  public void checkBeatForPublishHelperFirstFails() {
+    @SuppressWarnings("unchecked")
+    final Predicate<LocalDateTime> produceBeatsMock = Mockito.mock(Predicate.class);
+    Mockito.when(produceBeatsMock.test(testCase.nextBeat)).thenReturn(false);
+    final LocalDateTime nextBeatAfterPublish = Drummer.checkBeatForPublishHelper(
+        testCase.now,
+        testCase.alignmentHour,
+        testCase.nextBeat,
+        testCase.clockOffset,
         produceBeatsMock);
-    Assert.assertEquals(Optional.of(nextBeat), ret);
+    Assert.assertEquals("nextBeat", testCase.nextBeat, nextBeatAfterPublish);
   }
 
   @Test
   public void checkBeatForPublishSecondFails() {
-    final LocalDateTime now = LocalDateTime.now(Clock.systemUTC());
-    final LocalDateTime nextBeat = now.minus(3, ChronoUnit.DAYS);
-    final LocalDateTime secondBeat = Drummer.incrementToAlignment(nextBeat, 0, new ClockOffset());
+    if (testCase.expectedBeatPublishCount < 2)
+      return;
+
+    final LocalDateTime secondBeat = Drummer.incrementToAlignment(
+        testCase.nextBeat,
+        testCase.alignmentHour,
+        testCase.clockOffset);
     @SuppressWarnings("unchecked") final Predicate<LocalDateTime> produceBeatsMock = Mockito.mock(Predicate.class);
-    Mockito.when(produceBeatsMock.test(nextBeat)).thenReturn(true);
+    Mockito.when(produceBeatsMock.test(testCase.nextBeat)).thenReturn(true);
     Mockito.when(produceBeatsMock.test(secondBeat)).thenReturn(false);
-    final Optional<LocalDateTime> ret = Drummer.checkBeatForPublishHelper(
-        now,
-        0,
-        nextBeat,
-        new ClockOffset(),
+    final LocalDateTime nextBeatAfterPublish = Drummer.checkBeatForPublishHelper(
+        testCase.now,
+        testCase.alignmentHour,
+        testCase.nextBeat,
+        testCase.clockOffset,
         produceBeatsMock);
-    Assert.assertEquals(Optional.of(secondBeat), ret);
-  }
-
-  @Test
-  public void checkBeatForPublishNoneNeeded() {
-    final LocalDateTime now = LocalDateTime.now(Clock.systemUTC());
-    final Optional<LocalDateTime> ret = Drummer.checkBeatForPublishHelper(
-        now,
-        0,
-        now.plus(1, ChronoUnit.DAYS),
-        new ClockOffset(),
-        x -> { Assert.fail("Publish shouldn't be called"); return true; });
-    Assert.assertEquals(Optional.empty(), ret);
+    Assert.assertEquals(secondBeat, nextBeatAfterPublish);
   }
 }