Intent manager unit test enhancements

- add test to be sure that all flows are removed when intents
  are withdrawn
- add test to be sure that all flows are removed when an intent
  installation fails. Currently disabled, intent cleanup is
  not implemented
- enable installation time out test

Change-Id: I8c7a75292a97404ef89856647c67ef2f70ffcf6f
diff --git a/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java b/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java
index dc8cd2b..239aa0a 100644
--- a/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java
+++ b/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java
@@ -330,18 +330,23 @@
             new IntentTestsMocks.MockTreatment();
 
     public static class MockFlowRule implements FlowRule {
+        static int nextId = 0;
 
         int priority;
         Type type;
+        long timestamp;
+        int id;
 
         public MockFlowRule(int priority) {
             this.priority = priority;
             this.type = Type.DEFAULT;
+            this.timestamp = System.currentTimeMillis();
+            this.id = nextId++;
         }
 
         @Override
         public FlowId id() {
-            return FlowId.valueOf(1);
+            return FlowId.valueOf(id);
         }
 
         @Override
@@ -398,7 +403,8 @@
                 return false;
             }
             final MockFlowRule other = (MockFlowRule) obj;
-            return Objects.equals(this.priority, other.priority);
+            return Objects.equals(this.timestamp, other.timestamp) &&
+                   this.id == other.id;
         }
 
         @Override
diff --git a/core/net/src/test/java/org/onosproject/net/intent/impl/IntentManagerTest.java b/core/net/src/test/java/org/onosproject/net/intent/impl/IntentManagerTest.java
index 87ca8c7..a794d51 100644
--- a/core/net/src/test/java/org/onosproject/net/intent/impl/IntentManagerTest.java
+++ b/core/net/src/test/java/org/onosproject/net/intent/impl/IntentManagerTest.java
@@ -23,6 +23,8 @@
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 
 import org.hamcrest.Description;
 import org.hamcrest.TypeSafeMatcher;
@@ -57,6 +59,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
@@ -164,6 +167,18 @@
         }
     }
 
+    private static class TestIntentCompilerMultipleFlows implements IntentCompiler<MockIntent> {
+        @Override
+        public List<Intent> compile(MockIntent intent, List<Intent> installable,
+                                    Set<LinkResourceAllocations> resources) {
+
+            return IntStream.rangeClosed(1, 5)
+                            .mapToObj(mock -> (new MockInstallableIntent()))
+                            .collect(Collectors.toList());
+        }
+    }
+
+
     private static class TestIntentCompilerError implements IntentCompiler<MockIntent> {
         @Override
         public List<Intent> compile(MockIntent intent, List<Intent> installable,
@@ -173,7 +188,7 @@
     }
 
     /**
-     * Hamcrest matcher to check that a conllection of Intents contains an
+     * Hamcrest matcher to check that a collection of Intents contains an
      * Intent with the specified Intent Id.
      */
     public static class EntryForIntentMatcher extends TypeSafeMatcher<Collection<Intent>> {
@@ -375,7 +390,6 @@
      * Tests handling a future that contains an unresolvable error as a result of
      * installing an intent.
      */
-    @Ignore("test needs to be rewritten, so that the intent is resubmitted")
     @Test
     public void errorIntentInstallNeverTrue() {
         final Long id = MockIntent.nextId();
@@ -489,4 +503,78 @@
         assertThat(intents, hasIntentWithId(intent2.id()));
         verifyState();
     }
+
+    /**
+     * Tests that removing all intents results in no flows remaining.
+     */
+    @Test
+    public void testFlowRemoval() {
+        List<Intent> intents;
+
+        flowRuleService.setFuture(true);
+
+        intents = Lists.newArrayList(service.getIntents());
+        assertThat(intents, hasSize(0));
+
+        final MockIntent intent1 = new MockIntent(MockIntent.nextId());
+        final MockIntent intent2 = new MockIntent(MockIntent.nextId());
+
+        listener.setLatch(1, Type.INSTALL_REQ);
+        listener.setLatch(1, Type.INSTALLED);
+
+        service.submit(intent1);
+        listener.await(Type.INSTALL_REQ);
+        listener.await(Type.INSTALLED);
+
+
+        listener.setLatch(1, Type.INSTALL_REQ);
+        listener.setLatch(1, Type.INSTALLED);
+
+        service.submit(intent2);
+        listener.await(Type.INSTALL_REQ);
+        listener.await(Type.INSTALLED);
+
+        assertThat(listener.getCounts(Type.INSTALLED), is(2));
+        assertThat(flowRuleService.getFlowRuleCount(), is(2));
+
+        listener.setLatch(1, Type.WITHDRAWN);
+        service.withdraw(intent1);
+        listener.await(Type.WITHDRAWN);
+
+        listener.setLatch(1, Type.WITHDRAWN);
+        service.withdraw(intent2);
+        listener.await(Type.WITHDRAWN);
+
+        assertThat(listener.getCounts(Type.WITHDRAWN), is(2));
+        assertThat(flowRuleService.getFlowRuleCount(), is(0));
+    }
+
+    /**
+     * Tests that an intent that fails installation results in no flows remaining.
+     */
+    @Test
+    @Ignore("Cleanup state is not yet implemented in the intent manager")
+    public void testFlowRemovalInstallError() {
+        final TestIntentCompilerMultipleFlows errorCompiler = new TestIntentCompilerMultipleFlows();
+        extensionService.registerCompiler(MockIntent.class, errorCompiler);
+        List<Intent> intents;
+
+        flowRuleService.setFuture(true);
+        flowRuleService.setErrorFlow(3);
+
+        intents = Lists.newArrayList(service.getIntents());
+        assertThat(intents, hasSize(0));
+
+        final MockIntent intent1 = new MockIntent(MockIntent.nextId());
+
+        listener.setLatch(1, Type.INSTALL_REQ);
+        listener.setLatch(1, Type.FAILED);
+
+        service.submit(intent1);
+        listener.await(Type.INSTALL_REQ);
+        listener.await(Type.FAILED);
+
+        assertThat(listener.getCounts(Type.FAILED), is(1));
+        assertThat(flowRuleService.getFlowRuleCount(), is(0));
+    }
 }
diff --git a/core/net/src/test/java/org/onosproject/net/intent/impl/MockFlowRuleService.java b/core/net/src/test/java/org/onosproject/net/intent/impl/MockFlowRuleService.java
index 27e523d..87ab21a 100644
--- a/core/net/src/test/java/org/onosproject/net/intent/impl/MockFlowRuleService.java
+++ b/core/net/src/test/java/org/onosproject/net/intent/impl/MockFlowRuleService.java
@@ -34,6 +34,11 @@
     final Set<FlowRule> flows = Sets.newHashSet();
     boolean success;
 
+    int errorFlow = -1;
+    public void setErrorFlow(int errorFlow) {
+        this.errorFlow = errorFlow;
+    }
+
     public void setFuture(boolean success) {
         this.success = success;
     }
@@ -41,16 +46,20 @@
     @Override
     public void apply(FlowRuleOperations ops) {
         ops.stages().forEach(stage -> stage.forEach(flow -> {
-            switch (flow.type()) {
-                case ADD:
-                case MODIFY: //TODO is this the right behavior for modify?
-                    flows.add(flow.rule());
-                    break;
-                case REMOVE:
-                    flows.remove(flow.rule());
-                    break;
-                default:
-                    break;
+            if (errorFlow == flow.rule().id().value()) {
+                success = false;
+            } else {
+                switch (flow.type()) {
+                    case ADD:
+                    case MODIFY: //TODO is this the right behavior for modify?
+                        flows.add(flow.rule());
+                        break;
+                    case REMOVE:
+                        flows.remove(flow.rule());
+                        break;
+                    default:
+                        break;
+                }
             }
         }));
         if (success) {
diff --git a/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleIntentStore.java b/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleIntentStore.java
index e94a7f7..8fe8a11 100644
--- a/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleIntentStore.java
+++ b/core/store/trivial/src/main/java/org/onosproject/store/trivial/impl/SimpleIntentStore.java
@@ -99,19 +99,19 @@
             IntentData pendingData = pending.get(newData.key());
 
             if (IntentData.isUpdateAcceptable(currentData, newData)) {
-                if (pendingData.state() == PURGE_REQ) {
-                    current.remove(newData.key(), newData);
-                } else {
-                    current.put(newData.key(), new IntentData(newData));
-                }
+                if (pendingData != null) {
+                    if (pendingData.state() == PURGE_REQ) {
+                        current.remove(newData.key(), newData);
+                    } else {
+                        current.put(newData.key(), new IntentData(newData));
+                    }
 
-                if (pendingData != null
+                    if (pendingData.version().compareTo(newData.version()) <= 0) {
                         // pendingData version is less than or equal to newData's
                         // Note: a new update for this key could be pending (it's version will be greater)
-                        && pendingData.version().compareTo(newData.version()) <= 0) {
-                    pending.remove(newData.key());
+                        pending.remove(newData.key());
+                    }
                 }
-
                 notifyDelegateIfNotNull(IntentEvent.getEvent(newData));
             }
         }