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
diff --git a/apps/sdnip/pom.xml b/apps/sdnip/pom.xml
index b1465c1..5eb16f6 100644
--- a/apps/sdnip/pom.xml
+++ b/apps/sdnip/pom.xml
@@ -45,6 +45,12 @@
<dependency>
<groupId>org.onlab.onos</groupId>
+ <artifactId>onlab-junit</artifactId>
+ <scope>test</scope>
+ </dependency>
+
+ <dependency>
+ <groupId>org.onlab.onos</groupId>
<artifactId>onos-cli</artifactId>
<version>${project.version}</version>
</dependency>
diff --git a/apps/sdnip/src/test/java/org/onlab/onos/sdnip/RouterTest.java b/apps/sdnip/src/test/java/org/onlab/onos/sdnip/RouterTest.java
index 9e7e425..c61e1ec 100644
--- a/apps/sdnip/src/test/java/org/onlab/onos/sdnip/RouterTest.java
+++ b/apps/sdnip/src/test/java/org/onlab/onos/sdnip/RouterTest.java
@@ -17,6 +17,8 @@
import org.junit.Before;
import org.junit.Test;
+import org.onlab.junit.TestUtils;
+import org.onlab.junit.TestUtils.TestUtilsException;
import org.onlab.onos.ApplicationId;
import org.onlab.onos.net.ConnectPoint;
import org.onlab.onos.net.DefaultHost;
@@ -42,8 +44,6 @@
import org.onlab.packet.IpPrefix;
import org.onlab.packet.MacAddress;
import org.onlab.packet.VlanId;
-import org.onlab.util.TestUtils;
-import org.onlab.util.TestUtils.TestUtilsException;
import com.google.common.collect.Sets;
diff --git a/apps/sdnip/src/test/java/org/onlab/onos/sdnip/bgp/BgpSessionManagerTest.java b/apps/sdnip/src/test/java/org/onlab/onos/sdnip/bgp/BgpSessionManagerTest.java
index 99e3be1..f9dbb7b 100644
--- a/apps/sdnip/src/test/java/org/onlab/onos/sdnip/bgp/BgpSessionManagerTest.java
+++ b/apps/sdnip/src/test/java/org/onlab/onos/sdnip/bgp/BgpSessionManagerTest.java
@@ -26,12 +26,12 @@
import org.junit.After;
import org.junit.Before;
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.RouteUpdate;
import org.onlab.packet.IpAddress;
import org.onlab.packet.IpPrefix;
-import org.onlab.util.TestUtils;
-import org.onlab.util.TestUtils.TestUtilsException;
import com.google.common.net.InetAddresses;
diff --git a/core/api/src/test/java/org/onlab/onos/net/intent/IntentIdTest.java b/core/api/src/test/java/org/onlab/onos/net/intent/IntentIdTest.java
index 471e397..3e21187 100644
--- a/core/api/src/test/java/org/onlab/onos/net/intent/IntentIdTest.java
+++ b/core/api/src/test/java/org/onlab/onos/net/intent/IntentIdTest.java
@@ -6,6 +6,7 @@
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;
+import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
/**
* This class tests the immutability, equality, and non-equality of
@@ -17,7 +18,7 @@
*/
@Test
public void intentIdFollowsGuidelineForImmutableObject() {
- ImmutableClassChecker.assertThatClassIsImmutable(IntentId.class);
+ assertThatClassIsImmutable(IntentId.class);
}
/**
diff --git a/core/net/src/test/java/org/onlab/onos/net/intent/TestHostToHostIntent.java b/core/net/src/test/java/org/onlab/onos/net/intent/TestHostToHostIntent.java
index 4ebee73..2b56e88 100644
--- a/core/net/src/test/java/org/onlab/onos/net/intent/TestHostToHostIntent.java
+++ b/core/net/src/test/java/org/onlab/onos/net/intent/TestHostToHostIntent.java
@@ -11,6 +11,7 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
+import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
import static org.onlab.onos.net.NetTestTools.hid;
/**
@@ -104,6 +105,6 @@
*/
@Test
public void checkImmutability() {
- ImmutableClassChecker.assertThatClassIsImmutable(HostToHostIntent.class);
+ assertThatClassIsImmutable(HostToHostIntent.class);
}
}
diff --git a/core/net/src/test/java/org/onlab/onos/net/intent/TestLinkCollectionIntent.java b/core/net/src/test/java/org/onlab/onos/net/intent/TestLinkCollectionIntent.java
index 65aa71f..42b5b28 100644
--- a/core/net/src/test/java/org/onlab/onos/net/intent/TestLinkCollectionIntent.java
+++ b/core/net/src/test/java/org/onlab/onos/net/intent/TestLinkCollectionIntent.java
@@ -4,6 +4,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
+import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
import static org.onlab.onos.net.NetTestTools.link;
import java.util.HashSet;
@@ -154,6 +155,6 @@
*/
@Test
public void checkImmutability() {
- ImmutableClassChecker.assertThatClassIsImmutable(LinkCollectionIntent.class);
+ assertThatClassIsImmutable(LinkCollectionIntent.class);
}
}
diff --git a/core/net/src/test/java/org/onlab/onos/net/intent/TestMultiPointToSinglePointIntent.java b/core/net/src/test/java/org/onlab/onos/net/intent/TestMultiPointToSinglePointIntent.java
index 09dfcb2..0e4f706 100644
--- a/core/net/src/test/java/org/onlab/onos/net/intent/TestMultiPointToSinglePointIntent.java
+++ b/core/net/src/test/java/org/onlab/onos/net/intent/TestMultiPointToSinglePointIntent.java
@@ -15,6 +15,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
+import static org.onlab.junit.ImmutableClassChecker.assertThatClassIsImmutable;
import static org.onlab.onos.net.NetTestTools.connectPoint;
/**
@@ -135,7 +136,6 @@
*/
@Test
public void checkImmutability() {
- ImmutableClassChecker.
- assertThatClassIsImmutable(MultiPointToSinglePointIntent.class);
+ assertThatClassIsImmutable(MultiPointToSinglePointIntent.class);
}
}
diff --git a/core/pom.xml b/core/pom.xml
index c0f74cf..fc8216f 100644
--- a/core/pom.xml
+++ b/core/pom.xml
@@ -35,6 +35,7 @@
<dependency>
<groupId>org.onlab.onos</groupId>
<artifactId>onlab-junit</artifactId>
+ <scope>test</scope>
</dependency>
</dependencies>
diff --git a/pom.xml b/pom.xml
index ce02451..095e40e 100644
--- a/pom.xml
+++ b/pom.xml
@@ -248,7 +248,7 @@
<dependency>
<groupId>org.onlab.onos</groupId>
<artifactId>onlab-junit</artifactId>
- <version>1.0.0-SNAPSHOT</version>
+ <version>${project.version}</version>
<scope>test</scope>
</dependency>
diff --git a/utils/junit/pom.xml b/utils/junit/pom.xml
index d994a07..6ddadac 100644
--- a/utils/junit/pom.xml
+++ b/utils/junit/pom.xml
@@ -27,6 +27,16 @@
<artifactId>guava-testlib</artifactId>
<scope>compile</scope>
</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>
</project>
diff --git a/core/api/src/test/java/org/onlab/onos/net/intent/ImmutableClassChecker.java b/utils/junit/src/main/java/org/onlab/junit/ImmutableClassChecker.java
similarity index 97%
rename from core/api/src/test/java/org/onlab/onos/net/intent/ImmutableClassChecker.java
rename to utils/junit/src/main/java/org/onlab/junit/ImmutableClassChecker.java
index 0e63af9..30d895e 100644
--- a/core/api/src/test/java/org/onlab/onos/net/intent/ImmutableClassChecker.java
+++ b/utils/junit/src/main/java/org/onlab/junit/ImmutableClassChecker.java
@@ -1,5 +1,4 @@
-package org.onlab.onos.net.intent;
-//TODO is this the right package?
+package org.onlab.junit;
import org.hamcrest.Description;
import org.hamcrest.StringDescription;
diff --git a/utils/misc/src/main/java/org/onlab/util/TestUtils.java b/utils/junit/src/main/java/org/onlab/junit/TestUtils.java
similarity index 99%
rename from utils/misc/src/main/java/org/onlab/util/TestUtils.java
rename to utils/junit/src/main/java/org/onlab/junit/TestUtils.java
index 7e59564..64b7ae300 100644
--- a/utils/misc/src/main/java/org/onlab/util/TestUtils.java
+++ b/utils/junit/src/main/java/org/onlab/junit/TestUtils.java
@@ -1,4 +1,4 @@
-package org.onlab.util;
+package org.onlab.junit;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
diff --git a/utils/junit/src/main/java/org/onlab/junit/UtilityClassChecker.java b/utils/junit/src/main/java/org/onlab/junit/UtilityClassChecker.java
new file mode 100644
index 0000000..bd272d1
--- /dev/null
+++ b/utils/junit/src/main/java/org/onlab/junit/UtilityClassChecker.java
@@ -0,0 +1,134 @@
+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);
+ }
+ }
+}
diff --git a/utils/junit/src/test/java/org/onlab/junit/ImmutableClassCheckerTest.java b/utils/junit/src/test/java/org/onlab/junit/ImmutableClassCheckerTest.java
new file mode 100644
index 0000000..b4a6ff5
--- /dev/null
+++ b/utils/junit/src/test/java/org/onlab/junit/ImmutableClassCheckerTest.java
@@ -0,0 +1,120 @@
+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));
+ }
+
+}
+
diff --git a/utils/misc/src/test/java/org/onlab/util/TestUtilsTest.java b/utils/junit/src/test/java/org/onlab/junit/TestUtilsTest.java
similarity index 97%
rename from utils/misc/src/test/java/org/onlab/util/TestUtilsTest.java
rename to utils/junit/src/test/java/org/onlab/junit/TestUtilsTest.java
index 58e60c1..c57b351 100644
--- a/utils/misc/src/test/java/org/onlab/util/TestUtilsTest.java
+++ b/utils/junit/src/test/java/org/onlab/junit/TestUtilsTest.java
@@ -1,4 +1,4 @@
-package org.onlab.util;
+package org.onlab.junit;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
@@ -6,7 +6,7 @@
import org.junit.Before;
import org.junit.Test;
-import org.onlab.util.TestUtils.TestUtilsException;
+import org.onlab.junit.TestUtils.TestUtilsException;
/**
* Test and usage examples for TestUtils.
diff --git a/utils/junit/src/test/java/org/onlab/junit/UtilityClassCheckerTest.java b/utils/junit/src/test/java/org/onlab/junit/UtilityClassCheckerTest.java
new file mode 100644
index 0000000..7f56d81
--- /dev/null
+++ b/utils/junit/src/test/java/org/onlab/junit/UtilityClassCheckerTest.java
@@ -0,0 +1,145 @@
+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));
+ }
+}
diff --git a/utils/misc/pom.xml b/utils/misc/pom.xml
index d1d3a50..fcb0eb8 100644
--- a/utils/misc/pom.xml
+++ b/utils/misc/pom.xml
@@ -24,6 +24,7 @@
<dependency>
<groupId>org.onlab.onos</groupId>
<artifactId>onlab-junit</artifactId>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>io.netty</groupId>