Repository: mockito/mockito
Issue: Mockito.after() method accepts negative timeperiods and subsequent verifications always pass
Labels: bug

e.g.

```
Runnable runnable = Mockito.mock(Runnable.class);
Mockito.verify(runnable, Mockito.never()).run(); // passes as expected
Mockito.verify(runnable, Mockito.after(1000).never()).run(); // passes as expected
Mockito.verify(runnable, Mockito.after(-1000).atLeastOnce()).run(); // passes incorrectly
```


Issue comments:
Well spotted, thx

I can try to fix that, but need to know what exactly is the issue here:
Should negative values in after method be forbidden and trigger exception to be thrown? or maybe negatives are allowed here (negative value means - immediately, pretty much it should behave like after(0) )?

I would vote for an IllegalArgumentException being thrown.

The same is happening for _timeout_ method:
e.g. 

```
       SomeClazz mock = Mockito.mock(SomeClazz.class);
       Mockito.verify(mock, timeout(-100)).someMethod(); //passes, which is incorrect
```

If method was invoked, then it passes, but this negative timeout is at least confusing:

```
        SomeClazz mock = Mockito.mock(SomeClazz.class);
        mock.someMethod();
        Mockito.verify(mock, timeout(-100)).someMethod(); //passes
```


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/org/mockito/exceptions/Reporter.java b/src/org/mockito/exceptions/Reporter.java
index e0b1105daf..6e0b3bd157 100644
--- a/src/org/mockito/exceptions/Reporter.java
+++ b/src/org/mockito/exceptions/Reporter.java
@@ -816,6 +816,15 @@ public void usingConstructorWithFancySerializable(SerializableMode mode) {
         throw new MockitoException("Mocks instantiated with constructor cannot be combined with " + mode + " serialization mode.");
     }
 
+    public void cannotCreateTimerWithNegativeDurationTime(long durationMillis) {
+        throw new FriendlyReminderException(join("",
+                "Don't panic! I'm just a friendly reminder!",
+                "It is impossible for time to go backward, therefore...",
+                "You cannot put negative value of duration: (" +  durationMillis +  ")",
+                "as argument of timer methods (after(), timeout())",
+                ""));
+    }
+
     private MockName safelyGetMockName(Object mock) {
         return new MockUtil().getMockName(mock);
     }
diff --git a/src/org/mockito/internal/util/Timer.java b/src/org/mockito/internal/util/Timer.java
index b71e4fdd8f..9f4829749f 100644
--- a/src/org/mockito/internal/util/Timer.java
+++ b/src/org/mockito/internal/util/Timer.java
@@ -1,11 +1,14 @@
 package org.mockito.internal.util;
 
+import org.mockito.exceptions.Reporter;
+
 public class Timer {
 
     private final long durationMillis;
     private long startTime = -1;
 
     public Timer(long durationMillis) {
+        validateInput(durationMillis);
         this.durationMillis = durationMillis;
     }
 
@@ -23,4 +26,14 @@ public boolean isCounting() {
     public void start() {
         startTime = System.currentTimeMillis();
     }
+
+    private void validateInput(long durationMillis) {
+        if (durationMillis < 0) {
+            new Reporter().cannotCreateTimerWithNegativeDurationTime(durationMillis);
+        }
+    }
+
+    public long duration() {
+        return durationMillis;
+    }
 }
diff --git a/src/org/mockito/internal/verification/VerificationOverTimeImpl.java b/src/org/mockito/internal/verification/VerificationOverTimeImpl.java
index 4ea2463adc..e719cb402b 100644
--- a/src/org/mockito/internal/verification/VerificationOverTimeImpl.java
+++ b/src/org/mockito/internal/verification/VerificationOverTimeImpl.java
@@ -17,7 +17,6 @@
 public class VerificationOverTimeImpl implements VerificationMode {
 
     private final long pollingPeriodMillis;
-    private final long durationMillis;
     private final VerificationMode delegate;
     private final boolean returnOnSuccess;
     private final Timer timer;
@@ -34,14 +33,13 @@ public class VerificationOverTimeImpl implements VerificationMode {
      *                        {@link org.mockito.verification.VerificationAfterDelay}).
      */
     public VerificationOverTimeImpl(long pollingPeriodMillis, long durationMillis, VerificationMode delegate, boolean returnOnSuccess) {
-        this(pollingPeriodMillis, durationMillis, delegate, returnOnSuccess, new Timer(durationMillis));
+        this(pollingPeriodMillis, delegate, returnOnSuccess, new Timer(durationMillis));
     }
 
     /**
      * Create this verification mode, to be used to verify invocation ongoing data later.
      *
      * @param pollingPeriodMillis The frequency to poll delegate.verify(), to check whether the delegate has been satisfied
-     * @param durationMillis The max time to wait (in millis) for the delegate verification mode to be satisfied
      * @param delegate The verification mode to delegate overall success or failure to
      * @param returnOnSuccess Whether to immediately return successfully once the delegate is satisfied (as in
      *                        {@link org.mockito.verification.VerificationWithTimeout}, or to only return once
@@ -49,9 +47,8 @@ public VerificationOverTimeImpl(long pollingPeriodMillis, long durationMillis, V
      *                        {@link org.mockito.verification.VerificationAfterDelay}).
      * @param timer Checker of whether the duration of the verification is still acceptable
      */
-    public VerificationOverTimeImpl(long pollingPeriodMillis, long durationMillis, VerificationMode delegate, boolean returnOnSuccess, Timer timer) {
+    public VerificationOverTimeImpl(long pollingPeriodMillis, VerificationMode delegate, boolean returnOnSuccess, Timer timer) {
         this.pollingPeriodMillis = pollingPeriodMillis;
-        this.durationMillis = durationMillis;
         this.delegate = delegate;
         this.returnOnSuccess = returnOnSuccess;
         this.timer = timer;
@@ -111,24 +108,15 @@ protected boolean canRecoverFromFailure(VerificationMode verificationMode) {
         return !(verificationMode instanceof AtMost || verificationMode instanceof NoMoreInteractions);
     }
 
+    public VerificationOverTimeImpl copyWithVerificationMode(VerificationMode verificationMode) {
+        return new VerificationOverTimeImpl(pollingPeriodMillis, timer.duration(), verificationMode, returnOnSuccess);
+    }
+
     private void sleep(long sleep) {
         try {
             Thread.sleep(sleep);
         } catch (InterruptedException ie) {
-            // oups. not much luck.
+            throw new RuntimeException("Thread sleep has been interrupted", ie);
         }
     }
-
-    public long getPollingPeriod() {
-        return pollingPeriodMillis;
-    }
-
-    public long getDuration() {
-        return durationMillis;
-    }
-
-    public VerificationMode getDelegate() {
-        return delegate;
-    }
-
 }
diff --git a/src/org/mockito/verification/After.java b/src/org/mockito/verification/After.java
index 07d312232a..8d203581b7 100644
--- a/src/org/mockito/verification/After.java
+++ b/src/org/mockito/verification/After.java
@@ -20,13 +20,17 @@ public After(long delayMillis, VerificationMode verificationMode) {
         this(10, delayMillis, verificationMode);
     }
     
-    public After(long pollingPeriod, long delayMillis, VerificationMode verificationMode) {
-        super(new VerificationOverTimeImpl(pollingPeriod, delayMillis, verificationMode, false));
+    After(long pollingPeriod, long delayMillis, VerificationMode verificationMode) {
+        this(new VerificationOverTimeImpl(pollingPeriod, delayMillis, verificationMode, false));
     }
-    
+
+    After(VerificationOverTimeImpl verificationOverTime) {
+        super(verificationOverTime);
+    }
+
     @Override
     protected VerificationMode copySelfWithNewVerificationMode(VerificationMode verificationMode) {
-        return new After(wrappedVerification.getPollingPeriod(), wrappedVerification.getDuration(), verificationMode);
+        return new After(wrappedVerification.copyWithVerificationMode(verificationMode));
     }
 
 }
diff --git a/src/org/mockito/verification/Timeout.java b/src/org/mockito/verification/Timeout.java
index d8e87ba448..9cfcdad69b 100644
--- a/src/org/mockito/verification/Timeout.java
+++ b/src/org/mockito/verification/Timeout.java
@@ -29,21 +29,25 @@ public Timeout(long millis, VerificationMode delegate) {
      * See the javadoc for {@link VerificationWithTimeout}
      */
     Timeout(long pollingPeriodMillis, long millis, VerificationMode delegate) {
-        super(new VerificationOverTimeImpl(pollingPeriodMillis, millis, delegate, true));
+        this(new VerificationOverTimeImpl(pollingPeriodMillis, millis, delegate, true));
     }
 
     /**
      * See the javadoc for {@link VerificationWithTimeout}
      */
-    Timeout(long pollingPeriodMillis, long millis, VerificationMode delegate, Timer timer) {
-        super(new VerificationOverTimeImpl(pollingPeriodMillis, millis, delegate, true, timer));
+    Timeout(long pollingPeriodMillis, VerificationMode delegate, Timer timer) {
+        this(new VerificationOverTimeImpl(pollingPeriodMillis, delegate, true, timer));
     }
-    
+
+    Timeout(VerificationOverTimeImpl verificationOverTime) {
+        super(verificationOverTime);
+    }
+
     @Override
     protected VerificationMode copySelfWithNewVerificationMode(VerificationMode newVerificationMode) {
-        return new Timeout(wrappedVerification.getPollingPeriod(), wrappedVerification.getDuration(), newVerificationMode);
+        return new Timeout(wrappedVerification.copyWithVerificationMode(newVerificationMode));
     }
-    
+
     public VerificationMode atMost(int maxNumberOfInvocations) {
         new Reporter().atMostAndNeverShouldNotBeUsedWithTimeout();
         return null;
@@ -53,5 +57,4 @@ public VerificationMode never() {
         new Reporter().atMostAndNeverShouldNotBeUsedWithTimeout();
         return null;
     }
-
 }
\ No newline at end of file


Test patch:
diff --git a/test/org/mockito/internal/util/TimerTest.java b/test/org/mockito/internal/util/TimerTest.java
index 5e5f0f7841..e1156688c1 100644
--- a/test/org/mockito/internal/util/TimerTest.java
+++ b/test/org/mockito/internal/util/TimerTest.java
@@ -1,6 +1,8 @@
 package org.mockito.internal.util;
 
+import org.junit.Assert;
 import org.junit.Test;
+import org.mockito.exceptions.misusing.FriendlyReminderException;
 import org.mockitoutil.TestBase;
 
 import static org.hamcrest.CoreMatchers.is;
@@ -15,22 +17,35 @@ public void should_return_true_if_task_is_in_acceptable_time_bounds() {
 
         //when
         timer.start();
-        boolean stillCounting = timer.isCounting();
 
         //then
-        assertThat(stillCounting, is(true));
+        assertThat(timer.isCounting(), is(true));
     }
 
     @Test
-    public void should_return_false_if_task_is_outside_the_acceptable_time_bounds() {
+    public void should_return_false_when_time_run_out() throws Exception {
         //given
-        Timer timer = new Timer(-1);
+        Timer timer = new Timer(0);
         timer.start();
 
         //when
-        boolean stillCounting = timer.isCounting();
+        oneMillisecondPasses();
 
         //then
         assertThat(timer.isCounting(), is(false));
     }
+
+    @Test
+    public void should_throw_friendly_reminder_exception_when_duration_is_negative() {
+        try {
+            new Timer(-1);
+            Assert.fail("It is forbidden to create timer with negative value of timer's duration.");
+        } catch (FriendlyReminderException e) {
+            Assert.assertTrue(true);
+        }
+    }
+
+    private void oneMillisecondPasses() throws InterruptedException {
+        Thread.sleep(1);
+    }
 }
diff --git a/test/org/mockito/verification/NegativeDurationTest.java b/test/org/mockito/verification/NegativeDurationTest.java
new file mode 100644
index 0000000000..fbb2480342
--- /dev/null
+++ b/test/org/mockito/verification/NegativeDurationTest.java
@@ -0,0 +1,29 @@
+package org.mockito.verification;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+import org.mockito.exceptions.misusing.FriendlyReminderException;
+
+public class NegativeDurationTest {
+
+    @Test
+    public void should_throw_exception_when_duration_is_negative_for_timeout_method() {
+        try {
+            Mockito.timeout(-1);
+            Assert.fail("It is forbidden to invoke Mockito.timeout() with negative value.");
+        } catch (FriendlyReminderException e) {
+            Assert.assertTrue(true);
+        }
+    }
+
+    @Test
+    public void should_throw_exception_when_duration_is_negative_for_after_method() {
+        try {
+            Mockito.after(-1);
+            Assert.fail("It is forbidden to invoke Mockito.after() with negative value.");
+        } catch (FriendlyReminderException e) {
+            Assert.assertTrue(true);
+        }
+    }
+}
diff --git a/test/org/mockito/verification/TimeoutTest.java b/test/org/mockito/verification/TimeoutTest.java
index ecd1b63ad8..1272ac0e99 100644
--- a/test/org/mockito/verification/TimeoutTest.java
+++ b/test/org/mockito/verification/TimeoutTest.java
@@ -9,9 +9,6 @@
 import org.mockito.Mock;
 import org.mockito.exceptions.base.MockitoAssertionError;
 import org.mockito.internal.util.Timer;
-import org.mockito.internal.verification.AtLeast;
-import org.mockito.internal.verification.Only;
-import org.mockito.internal.verification.Times;
 import org.mockito.internal.verification.VerificationDataImpl;
 import org.mockitoutil.TestBase;
 
@@ -27,7 +24,7 @@ public class TimeoutTest extends TestBase {
 
     @Test
     public void should_pass_when_verification_passes() {
-        Timeout t = new Timeout(1, 3, mode, timer);
+        Timeout t = new Timeout(1, mode, timer);
 
         when(timer.isCounting()).thenReturn(true);
         doNothing().when(mode).verify(data);
@@ -41,7 +38,7 @@ public void should_pass_when_verification_passes() {
 
     @Test
     public void should_fail_because_verification_fails() {
-        Timeout t = new Timeout(1, 2, mode, timer);
+        Timeout t = new Timeout(1, mode, timer);
 
         when(timer.isCounting()).thenReturn(true, true, true, false);
         doThrow(error).
@@ -59,7 +56,7 @@ public void should_fail_because_verification_fails() {
     
     @Test
     public void should_pass_even_if_first_verification_fails() {
-        Timeout t = new Timeout(1, 5, mode, timer);
+        Timeout t = new Timeout(1, mode, timer);
 
         when(timer.isCounting()).thenReturn(true, true, true, false);
         doThrow(error).
@@ -73,7 +70,7 @@ public void should_pass_even_if_first_verification_fails() {
 
     @Test
     public void should_try_to_verify_correct_number_of_times() {
-        Timeout t = new Timeout(10, 50, mode, timer);
+        Timeout t = new Timeout(10, mode, timer);
         
         doThrow(error).when(mode).verify(data);
         when(timer.isCounting()).thenReturn(true, true, true, true, true, false);
@@ -85,21 +82,5 @@ public void should_try_to_verify_correct_number_of_times() {
 
         verify(mode, times(5)).verify(data);
     }
-    
-    @Test
-    public void should_create_correctly_configured_timeout() {
-        Timeout t = new Timeout(25, 50, mode, timer);
-        
-        assertTimeoutCorrectlyConfigured(t.atLeastOnce(), Timeout.class, 50, 25, AtLeast.class);
-        assertTimeoutCorrectlyConfigured(t.atLeast(5), Timeout.class, 50, 25, AtLeast.class);
-        assertTimeoutCorrectlyConfigured(t.times(5), Timeout.class, 50, 25, Times.class);
-        assertTimeoutCorrectlyConfigured(t.only(), Timeout.class, 50, 25, Only.class);
-    }
-    
-    private void assertTimeoutCorrectlyConfigured(VerificationMode t, Class<?> expectedType, long expectedTimeout, long expectedPollingPeriod, Class<?> expectedDelegateType) {
-        assertEquals(expectedType, t.getClass());
-        assertEquals(expectedTimeout, ((Timeout) t).wrappedVerification.getDuration());
-        assertEquals(expectedPollingPeriod, ((Timeout) t).wrappedVerification.getPollingPeriod());
-        assertEquals(expectedDelegateType, ((Timeout) t).wrappedVerification.getDelegate().getClass());
-    }
+
 }
\ No newline at end of file