Refactoring in the unit test utility framework:

 * Moved unit test utilities to the onlab-junit package under utils/junit
   - ImmutableClassChecker
   - TestUtils and TestUtilsTest

 * Added/ported unit test utilities from the older code
   - UtilityClassChecker and UtilityClassCheckerTest
   - ImmutableClassCheckerTest

 * Updated/fixed some of the pom.xml files in the context of the
   onlab-junit package:
   - Added <scope>test</scope>
   - Replaced hard-coded "1.0.0-SNAPSHOT" with "${project.version}"

Change-Id: Ie5f51ba401ca1748340f38848ab6bfc251964adc

@@ -45,6 +45,12 @@
 
 
     <dependency>
     <dependency>
       <groupId>org.onlab.onos</groupId>
       <groupId>org.onlab.onos</groupId>
+      <artifactId>onlab-junit</artifactId>
+      <scope>test</scope>
+    </dependency>
+
+    <dependency>
+      <groupId>org.onlab.onos</groupId>
       <artifactId>onos-cli</artifactId>
       <artifactId>onos-cli</artifactId>
       <version>${project.version}</version>
       <version>${project.version}</version>
     </dependency>
     </dependency>

@@ -17,6 +17,8 @@ import java.util.Set;
 
 
 import org.junit.Before;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.Test;
+import org.onlab.junit.TestUtils;
+import org.onlab.junit.TestUtils.TestUtilsException;
 import org.onlab.onos.ApplicationId;
 import org.onlab.onos.ApplicationId;
 import org.onlab.onos.net.ConnectPoint;
 import org.onlab.onos.net.ConnectPoint;
 import org.onlab.onos.net.DefaultHost;
 import org.onlab.onos.net.DefaultHost;
@@ -42,8 +44,6 @@ import org.onlab.packet.IpAddress;
 import org.onlab.packet.IpPrefix;
 import org.onlab.packet.IpPrefix;
 import org.onlab.packet.MacAddress;
 import org.onlab.packet.MacAddress;
 import org.onlab.packet.VlanId;
 import org.onlab.packet.VlanId;
-import org.onlab.util.TestUtils;
-import org.onlab.util.TestUtils.TestUtilsException;
 
 
 import com.google.common.collect.Sets;
 import com.google.common.collect.Sets;
 
 

@@ -26,12 +26,12 @@ import org.jboss.netty.channel.socket.nio.NioClientSocketChannelFactory;
 import org.junit.After;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.Test;
+import org.onlab.junit.TestUtils;
+import org.onlab.junit.TestUtils.TestUtilsException;
 import org.onlab.onos.sdnip.RouteListener;
 import org.onlab.onos.sdnip.RouteListener;
 import org.onlab.onos.sdnip.RouteUpdate;
 import org.onlab.onos.sdnip.RouteUpdate;
 import org.onlab.packet.IpAddress;
 import org.onlab.packet.IpAddress;
 import org.onlab.packet.IpPrefix;
 import org.onlab.packet.IpPrefix;
-import org.onlab.util.TestUtils;
-import org.onlab.util.TestUtils.TestUtilsException;
 
 
 import com.google.common.net.InetAddresses;
 import com.google.common.net.InetAddresses;
 
 

@@ -6,6 +6,7 @@ import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertThat;
+import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
 
 
 /**
 /**
  * This class tests the immutability, equality, and non-equality of
  * This class tests the immutability, equality, and non-equality of
@@ -17,7 +18,7 @@ public class IntentIdTest {
      */
      */
     @Test
     @Test
     public void intentIdFollowsGuidelineForImmutableObject() {
     public void intentIdFollowsGuidelineForImmutableObject() {
-        ImmutableClassChecker.assertThatClassIsImmutable(IntentId.class);
+        assertThatClassIsImmutable(IntentId.class);
     }
     }
 
 
     /**
     /**

@@ -11,6 +11,7 @@ import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.not;
+import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
 import static org.onlab.onos.net.NetTestTools.hid;
 import static org.onlab.onos.net.NetTestTools.hid;
 
 
 /**
 /**
@@ -104,6 +105,6 @@ public class TestHostToHostIntent {
      */
      */
     @Test
     @Test
     public void checkImmutability() {
     public void checkImmutability() {
-        ImmutableClassChecker.assertThatClassIsImmutable(HostToHostIntent.class);
+        assertThatClassIsImmutable(HostToHostIntent.class);
     }
     }
 }
 }

@@ -4,6 +4,7 @@ import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.is;
+import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
 import static org.onlab.onos.net.NetTestTools.link;
 import static org.onlab.onos.net.NetTestTools.link;
 
 
 import java.util.HashSet;
 import java.util.HashSet;
@@ -154,6 +155,6 @@ public class TestLinkCollectionIntent {
      */
      */
     @Test
     @Test
     public void checkImmutability() {
     public void checkImmutability() {
-        ImmutableClassChecker.assertThatClassIsImmutable(LinkCollectionIntent.class);
+        assertThatClassIsImmutable(LinkCollectionIntent.class);
     }
     }
 }
 }

@@ -15,6 +15,7 @@ import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.is;
+import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
 import static org.onlab.onos.net.NetTestTools.connectPoint;
 import static org.onlab.onos.net.NetTestTools.connectPoint;
 
 
 /**
 /**
@@ -135,7 +136,6 @@ public class TestMultiPointToSinglePointIntent {
      */
      */
     @Test
     @Test
     public void checkImmutability() {
     public void checkImmutability() {
-        ImmutableClassChecker.
-                assertThatClassIsImmutable(MultiPointToSinglePointIntent.class);
+        assertThatClassIsImmutable(MultiPointToSinglePointIntent.class);
     }
     }
 }
 }

@@ -35,6 +35,7 @@
         <dependency>
         <dependency>
             <groupId>org.onlab.onos</groupId>
             <groupId>org.onlab.onos</groupId>
             <artifactId>onlab-junit</artifactId>
             <artifactId>onlab-junit</artifactId>
+            <scope>test</scope>
         </dependency>
         </dependency>
     </dependencies>
     </dependencies>
 
 

@@ -248,7 +248,7 @@
             <dependency>
             <dependency>
                 <groupId>org.onlab.onos</groupId>
                 <groupId>org.onlab.onos</groupId>
                 <artifactId>onlab-junit</artifactId>
                 <artifactId>onlab-junit</artifactId>
-                <version>1.0.0-SNAPSHOT</version>
+                <version>${project.version}</version>
                 <scope>test</scope>
                 <scope>test</scope>
             </dependency>
             </dependency>
 
 

@@ -27,6 +27,16 @@
             <artifactId>guava-testlib</artifactId>
             <artifactId>guava-testlib</artifactId>
             <scope>compile</scope>
             <scope>compile</scope>
         </dependency>
         </dependency>
+        <dependency>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest-core</artifactId>
+            <scope>compile</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest-library</artifactId>
+            <scope>compile</scope>
+        </dependency>
     </dependencies>
     </dependencies>
 
 
 </project>
 </project>

-package org.onlab.onos.net.intent;
-//TODO is this the right package?
+package org.onlab.junit;
 
 
 import org.hamcrest.Description;
 import org.hamcrest.Description;
 import org.hamcrest.StringDescription;
 import org.hamcrest.StringDescription;

-package org.onlab.util;
+package org.onlab.junit;
 
 
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Field;

+package org.onlab.junit;
+
+import org.hamcrest.Description;
+import org.hamcrest.StringDescription;
+import org.onlab.junit.TestUtils.TestUtilsException;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+
+
+/**
+ * Hamcrest style class for verifying that a class follows the
+ * accepted rules for utility classes.
+ *
+ * The rules that are enforced for utility classes:
+ *    - the class must be declared final
+ *    - the class must have only one constructor
+ *    - the constructor must be private and inaccessible to callers
+ *    - the class must have only static methods
+ */
+
+public class UtilityClassChecker {
+
+    private String failureReason = "";
+
+    /**
+     * Method to determine if a given class is a properly specified
+     * utility class.  In addition to checking that the class meets the criteria
+     * for utility classes, an object of the class type is allocated to force
+     * test code coverage onto the class constructor.
+     *
+     * @param clazz the class to check
+     * @return true if the given class is a properly specified utility class.
+     */
+    private boolean isProperlyDefinedUtilityClass(Class<?> clazz) {
+        // class must be declared final
+        if (!Modifier.isFinal(clazz.getModifiers())) {
+            failureReason = "a class that is not final";
+            return false;
+        }
+
+        // class must have only one constructor
+        final Constructor<?>[] constructors = clazz.getDeclaredConstructors();
+        if (constructors.length != 1) {
+            failureReason = "a class with more than one constructor";
+            return false;
+        }
+
+        //  constructor must not be accessible outside of the class
+        final Constructor<?> constructor = constructors[0];
+        if (constructor.isAccessible()) {
+            failureReason = "a class with an accessible default constructor";
+            return false;
+        }
+
+        // constructor must be private
+        if (!Modifier.isPrivate(constructor.getModifiers())) {
+            failureReason = "a class with a default constructor that is not private";
+            return false;
+        }
+
+        // class must have only static methods
+        for (final Method method : clazz.getMethods()) {
+            if (method.getDeclaringClass().equals(clazz)) {
+                if (!Modifier.isStatic(method.getModifiers())) {
+                    failureReason = "a class with one or more non-static methods";
+                    return false;
+                }
+            }
+
+        }
+
+        try {
+            final Object newObject = TestUtils.callConstructor(constructor);
+            if (newObject == null) {
+                failureReason = "could not instantiate a new object";
+                return false;
+            }
+        } catch (TestUtilsException e) {
+            failureReason = "could not instantiate a new object";
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * Describe why an error was reported.  Uses Hamcrest style Description
+     * interfaces.
+     *
+     * @param description the Description object to use for reporting the
+     *                    mismatch
+     */
+    public void describeMismatch(Description description) {
+        description.appendText(failureReason);
+    }
+
+    /**
+     * Describe the source object that caused an error, using a Hamcrest
+     * Matcher style interface.  In this case, it always returns
+     * that we are looking for a properly defined utility class.
+     *
+     * @param description the Description object to use to report the "to"
+     *                    object
+     */
+    public void describeTo(Description description) {
+        description.appendText("a properly defined utility class");
+    }
+
+    /**
+     * Assert that the given class adheres to the utility class rules.
+     *
+     * @param clazz the class to check
+     *
+     * @throws java.lang.AssertionError if the class is not a valid
+     *         utility class
+     */
+    public static void assertThatClassIsUtility(Class<?> clazz) {
+        final UtilityClassChecker checker = new UtilityClassChecker();
+        if (!checker.isProperlyDefinedUtilityClass(clazz)) {
+            final Description toDescription = new StringDescription();
+            final Description mismatchDescription = new StringDescription();
+
+            checker.describeTo(toDescription);
+            checker.describeMismatch(mismatchDescription);
+            final String reason =
+                "\n" +
+                "Expected: is \"" + toDescription.toString() + "\"\n" +
+                "    but : was \"" + mismatchDescription.toString() + "\"";
+
+            throw new AssertionError(reason);
+        }
+    }
+}

+package org.onlab.junit;
+
+import org.junit.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
+
+/**
+ * Set of unit tests to check the implementation of the immutable class
+ * checker.
+ */
+public class ImmutableClassCheckerTest {
+    /**
+     * Test class for non final class check.
+     */
+    // CHECKSTYLE IGNORE FinalClass FOR NEXT 1 LINES
+    static class NonFinal {
+        private NonFinal() { }
+    }
+
+    /**
+     * Check that a non final class correctly produces an error.
+     * @throws Exception if any of the reflection lookups fail.
+     */
+    @Test
+    public void testNonFinalClass() throws Exception {
+        boolean gotException = false;
+        try {
+            assertThatClassIsImmutable(NonFinal.class);
+        } catch (AssertionError assertion) {
+            assertThat(assertion.getMessage(),
+                    containsString("is not final"));
+            gotException = true;
+        }
+        assertThat(gotException, is(true));
+    }
+
+    /**
+     * Test class for non private member class check.
+     */
+    static final class FinalProtectedMember {
+        protected final int x = 0;
+    }
+
+    /**
+     * Check that a final class with a non-private member is properly detected.
+     *
+     * @throws Exception if any of the reflection lookups fail.
+     */
+    @Test
+    public void testFinalProtectedMember() throws Exception {
+        boolean gotException = false;
+        try {
+            assertThatClassIsImmutable(FinalProtectedMember.class);
+        } catch (AssertionError assertion) {
+            assertThat(assertion.getMessage(),
+                       containsString("a field named 'x' that is not private"));
+            gotException = true;
+        }
+        assertThat(gotException, is(true));
+    }
+
+    /**
+     * Test class for non private member class check.
+     */
+    static final class NotFinalPrivateMember {
+        private int x = 0;
+    }
+
+    /**
+     * Check that a final class with a non-final private
+     * member is properly detected.
+     *
+     * @throws Exception if any of the reflection lookups fail.
+     */
+    @Test
+    public void testNotFinalPrivateMember() throws Exception {
+        boolean gotException = false;
+        try {
+            assertThatClassIsImmutable(NotFinalPrivateMember.class);
+        } catch (AssertionError assertion) {
+            assertThat(assertion.getMessage(),
+                    containsString("a field named 'x' that is not final"));
+            gotException = true;
+        }
+        assertThat(gotException, is(true));
+    }
+
+    /**
+     * Test class for non private member class check.
+     */
+    static final class ClassWithSetter {
+        private final int x = 0;
+        public void setX(int newX) {
+        }
+    }
+
+    /**
+     * Check that a final class with a final private
+     * member that is modifyable by a setter is properly detected.
+     *
+     * @throws Exception if any of the reflection lookups fail.
+     */
+    @Test
+    public void testClassWithSetter() throws Exception {
+        boolean gotException = false;
+        try {
+            assertThatClassIsImmutable(ClassWithSetter.class);
+        } catch (AssertionError assertion) {
+            assertThat(assertion.getMessage(),
+                    containsString("a class with a setter named 'setX'"));
+            gotException = true;
+        }
+        assertThat(gotException, is(true));
+    }
+
+}
+

-package org.onlab.util;
+package org.onlab.junit;
 
 
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertEquals;
@@ -6,7 +6,7 @@ import static org.junit.Assert.assertNull;
 
 
 import org.junit.Before;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.Test;
-import org.onlab.util.TestUtils.TestUtilsException;
+import org.onlab.junit.TestUtils.TestUtilsException;
 
 
 /**
 /**
  * Test and usage examples for TestUtils.
  * Test and usage examples for TestUtils.

+package org.onlab.junit;
+
+import org.junit.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.onlab.junit.UtilityClassChecker.assertThatClassIsUtility;
+
+/**
+ * Set of unit tests to check the implementation of the utility class
+ * checker.
+ */
+public class UtilityClassCheckerTest {
+
+    // CHECKSTYLE:OFF test data intentionally not final
+    /**
+     * Test class for non final class check.
+     */
+    static class NonFinal {
+        private NonFinal() { }
+    }
+    // CHECKSTYLE:ON
+
+    /**
+     * Check that a non final class correctly produces an error.
+     * @throws Exception if any of the reflection lookups fail.
+     */
+    @Test
+    public void testNonFinalClass() throws Exception {
+        boolean gotException = false;
+        try {
+            assertThatClassIsUtility(NonFinal.class);
+        } catch (AssertionError assertion) {
+            assertThat(assertion.getMessage(),
+                       containsString("is not final"));
+            gotException = true;
+        }
+        assertThat(gotException, is(true));
+    }
+
+    /**
+     * Test class for final no constructor class check.
+     */
+    static final class FinalNoConstructor {
+    }
+
+    /**
+     * Check that a final class with no declared constructor correctly produces
+     * an error.  In this case, the compiler generates a default constructor
+     * for you, but the constructor is 'protected' and will fail the check.
+     *
+     * @throws Exception if any of the reflection lookups fail.
+     */
+    @Test
+    public void testFinalNoConstructorClass() throws Exception {
+        boolean gotException = false;
+        try {
+            assertThatClassIsUtility(FinalNoConstructor.class);
+        } catch (AssertionError assertion) {
+            assertThat(assertion.getMessage(),
+                    containsString("class with a default constructor that " +
+                                   "is not private"));
+            gotException = true;
+        }
+        assertThat(gotException, is(true));
+    }
+
+    /**
+     * Test class for class with more than one constructor check.
+     */
+    static final class TwoConstructors {
+        private TwoConstructors() { }
+        private TwoConstructors(int x) { }
+    }
+
+    /**
+     * Check that a non static class correctly produces an error.
+     * @throws Exception if any of the reflection lookups fail.
+     */
+    @Test
+    public void testOnlyOneConstructor() throws Exception {
+        boolean gotException = false;
+        try {
+            assertThatClassIsUtility(TwoConstructors.class);
+        } catch (AssertionError assertion) {
+            assertThat(assertion.getMessage(),
+                       containsString("more than one constructor"));
+            gotException = true;
+        }
+        assertThat(gotException, is(true));
+    }
+
+    /**
+     * Test class with a non private constructor.
+     */
+    static final class NonPrivateConstructor {
+        protected NonPrivateConstructor() { }
+    }
+
+    /**
+     * Check that a class with a non private constructor correctly
+     * produces an error.
+     * @throws Exception if any of the reflection lookups fail.
+     */
+    @Test
+    public void testNonPrivateConstructor() throws Exception {
+
+        boolean gotException = false;
+        try {
+            assertThatClassIsUtility(NonPrivateConstructor.class);
+        } catch (AssertionError assertion) {
+            assertThat(assertion.getMessage(),
+                       containsString("constructor that is not private"));
+            gotException = true;
+        }
+        assertThat(gotException, is(true));
+    }
+
+    /**
+     * Test class with a non static method.
+     */
+    static final class NonStaticMethod {
+        private NonStaticMethod() { }
+        public void aPublicMethod() { }
+    }
+
+    /**
+     * Check that a class with a non static method correctly produces an error.
+     * @throws Exception if any of the reflection lookups fail.
+     */
+    @Test
+    public void testNonStaticMethod() throws Exception {
+
+        boolean gotException = false;
+        try {
+            assertThatClassIsUtility(NonStaticMethod.class);
+        } catch (AssertionError assertion) {
+            assertThat(assertion.getMessage(),
+                       containsString("one or more non-static methods"));
+            gotException = true;
+        }
+        assertThat(gotException, is(true));
+    }
+}

@@ -24,6 +24,7 @@
         <dependency>
         <dependency>
             <groupId>org.onlab.onos</groupId>
             <groupId>org.onlab.onos</groupId>
             <artifactId>onlab-junit</artifactId>
             <artifactId>onlab-junit</artifactId>
+            <scope>test</scope>
         </dependency>
         </dependency>
         <dependency>
         <dependency>
             <groupId>io.netty</groupId>
             <groupId>io.netty</groupId>