From 264edb3fb5de63051e6a66a533e40dadee253ae0 Mon Sep 17 00:00:00 2001
From: Sam Brannen <sam@sambrannen.com>
Date: Thu, 19 Jan 2017 05:42:26 +0100
Subject: [PATCH] Improve initialization of Assume class

Prior to this commit, the org.springframework.tests.Assume class could
fail to load resulting in a NoClassDefFoundError if parsing of the
'testGroups' system property failed. This is because the parsing took
place while initializing a static field.

This commit addresses this issue by moving the 'testGroups' system
property lookup to a dedicated method that is lazily invoked upon
demand instead of eagerly when loading the Assume class itself.

In addition, when an error occurs, TestGroup.parse() now logs the
complete original value of the supplied test groups string instead of
potentially omitting the "all-" prefix. This results in more
informative error messages similar to the following.

  java.lang.IllegalStateException: Failed to parse 'testGroups' system
  property: Unable to find test group 'bogus' when parsing testGroups
  value: 'all-bogus'. Available groups include:
  [LONG_RUNNING,PERFORMANCE,JMXMP,CI]

Issue: SPR-15163
---
 .../org/springframework/tests/Assume.java     |  34 +++--
 .../springframework/tests/AssumeTests.java    | 124 ++++++++++++++++++
 .../org/springframework/tests/TestGroup.java  |  21 +--
 .../springframework/tests/TestGroupTests.java |  46 ++++---
 4 files changed, 193 insertions(+), 32 deletions(-)
 create mode 100644 spring-core/src/test/java/org/springframework/tests/AssumeTests.java

diff --git a/spring-core/src/test/java/org/springframework/tests/Assume.java b/spring-core/src/test/java/org/springframework/tests/Assume.java
index 8ab8958935..7b487d08b8 100644
--- a/spring-core/src/test/java/org/springframework/tests/Assume.java
+++ b/spring-core/src/test/java/org/springframework/tests/Assume.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,13 +28,14 @@ import static org.junit.Assume.*;
  * conditions hold {@code true}. If the assumption fails, it means the test should be
  * skipped.
  *
- * Tests can be categorized into {@link TestGroup}s. Active groups are enabled using
+ * <p>Tests can be categorized into {@link TestGroup}s. Active groups are enabled using
  * the 'testGroups' system property, usually activated from the gradle command line:
- * <pre>
+ *
+ * <pre class="code">
  * gradle test -PtestGroups="performance"
  * </pre>
  *
- * Groups can be specified as a comma separated list of values, or using the pseudo group
+ * <p>Groups can be specified as a comma separated list of values, or using the pseudo group
  * 'all'. See {@link TestGroup} for a list of valid groups.
  *
  * @author Rob Winch
@@ -46,7 +47,7 @@ import static org.junit.Assume.*;
  */
 public abstract class Assume {
 
-	private static final Set<TestGroup> GROUPS = TestGroup.parse(System.getProperty("testGroups"));
+	static final String TEST_GROUPS_SYSTEM_PROPERTY = "testGroups";
 
 
 	/**
@@ -55,8 +56,9 @@ public abstract class Assume {
 	 * @throws AssumptionViolatedException if the assumption fails
 	 */
 	public static void group(TestGroup group) {
-		if (!GROUPS.contains(group)) {
-			throw new AssumptionViolatedException("Requires unspecified group " + group + " from " + GROUPS);
+		Set<TestGroup> testGroups = loadTestGroups();
+		if (!testGroups.contains(group)) {
+			throw new AssumptionViolatedException("Requires unspecified group " + group + " from " + testGroups);
 		}
 	}
 
@@ -70,7 +72,8 @@ public abstract class Assume {
 	 * @since 4.2
 	 */
 	public static void group(TestGroup group, Executable executable) throws Exception {
-		if (GROUPS.contains(group)) {
+		Set<TestGroup> testGroups = loadTestGroups();
+		if (testGroups.contains(group)) {
 			executable.execute();
 		}
 	}
@@ -85,6 +88,21 @@ public abstract class Assume {
 		assumeFalse(log.isDebugEnabled());
 	}
 
+	/**
+	 * Load test groups dynamically instead of during static
+	 * initialization in order to avoid a {@link NoClassDefFoundError}
+	 * being thrown while attempting to load the {@code Assume} class.
+	 */
+	private static Set<TestGroup> loadTestGroups() {
+		try {
+			return TestGroup.parse(System.getProperty(TEST_GROUPS_SYSTEM_PROPERTY));
+		}
+		catch (Exception ex) {
+			throw new IllegalStateException("Failed to parse '" + TEST_GROUPS_SYSTEM_PROPERTY
+					+ "' system property: " + ex.getMessage(), ex);
+		}
+	}
+
 
 	/**
 	 * @since 4.2
diff --git a/spring-core/src/test/java/org/springframework/tests/AssumeTests.java b/spring-core/src/test/java/org/springframework/tests/AssumeTests.java
new file mode 100644
index 0000000000..11aab79227
--- /dev/null
+++ b/spring-core/src/test/java/org/springframework/tests/AssumeTests.java
@@ -0,0 +1,124 @@
+/*
+ * Copyright 2002-2017 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.tests;
+
+import java.util.Arrays;
+
+import org.junit.After;
+import org.junit.AssumptionViolatedException;
+import org.junit.Before;
+import org.junit.Test;
+
+import static java.util.stream.Collectors.*;
+import static org.hamcrest.CoreMatchers.*;
+import static org.junit.Assert.*;
+import static org.springframework.tests.Assume.*;
+import static org.springframework.tests.TestGroup.*;
+
+/**
+ * Tests for {@link Assume}.
+ *
+ * @author Sam Brannen
+ * @since 5.0
+ */
+public class AssumeTests {
+
+	private String originalTestGroups;
+
+
+	@Before
+	public void trackOriginalTestGroups() {
+		this.originalTestGroups = System.getProperty(TEST_GROUPS_SYSTEM_PROPERTY);
+	}
+
+	@After
+	public void restoreOriginalTestGroups() {
+		if (this.originalTestGroups != null) {
+			setTestGroups(this.originalTestGroups);
+		}
+		else {
+			setTestGroups("");
+		}
+	}
+
+	@Test
+	public void assumeGroupWithNoActiveTestGroups() {
+		setTestGroups("");
+		Assume.group(JMXMP);
+		fail("assumption should have failed");
+	}
+
+	@Test
+	public void assumeGroupWithNoMatchingActiveTestGroup() {
+		setTestGroups(PERFORMANCE, CI);
+		Assume.group(JMXMP);
+		fail("assumption should have failed");
+	}
+
+	@Test
+	public void assumeGroupWithMatchingActiveTestGroup() {
+		setTestGroups(JMXMP);
+		try {
+			Assume.group(JMXMP);
+		}
+		catch (AssumptionViolatedException ex) {
+			fail("assumption should NOT have failed");
+		}
+	}
+
+	@Test
+	public void assumeGroupWithBogusActiveTestGroup() {
+		assertBogusActiveTestGroupBehavior("bogus");
+	}
+
+	@Test
+	public void assumeGroupWithAllMinusBogusActiveTestGroup() {
+		assertBogusActiveTestGroupBehavior("all-bogus");
+	}
+
+	private void assertBogusActiveTestGroupBehavior(String testGroups) {
+		// Should result in something similar to the following:
+		//
+		// java.lang.IllegalStateException: Failed to parse 'testGroups' system property:
+		// Unable to find test group 'bogus' when parsing testGroups value: 'all-bogus'.
+		// Available groups include: [LONG_RUNNING,PERFORMANCE,JMXMP,CI]
+
+		setTestGroups(testGroups);
+		try {
+			Assume.group(JMXMP);
+			fail("assumption should have failed");
+		}
+		catch (IllegalStateException ex) {
+			assertThat(ex.getMessage(),
+				startsWith("Failed to parse '" + TEST_GROUPS_SYSTEM_PROPERTY + "' system property: "));
+
+			assertThat(ex.getCause(), instanceOf(IllegalArgumentException.class));
+			assertThat(ex.getCause().getMessage(),
+				equalTo("Unable to find test group 'bogus' when parsing testGroups value: '" + testGroups
+						+ "'. Available groups include: [LONG_RUNNING,PERFORMANCE,JMXMP,CI]"));
+		}
+	}
+
+	private void setTestGroups(TestGroup... testGroups) {
+		setTestGroups(Arrays.stream(testGroups).map(TestGroup::name).collect(joining(", ")));
+	}
+
+	private void setTestGroups(String testGroups) {
+		System.setProperty(TEST_GROUPS_SYSTEM_PROPERTY, testGroups);
+	}
+
+}
diff --git a/spring-core/src/test/java/org/springframework/tests/TestGroup.java b/spring-core/src/test/java/org/springframework/tests/TestGroup.java
index 6fe51b6949..eb0baa606d 100644
--- a/spring-core/src/test/java/org/springframework/tests/TestGroup.java
+++ b/spring-core/src/test/java/org/springframework/tests/TestGroup.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -31,6 +31,7 @@ import static java.lang.String.*;
  * @see Assume#group(TestGroup)
  * @author Phillip Webb
  * @author Chris Beams
+ * @author Sam Brannen
  */
 public enum TestGroup {
 
@@ -65,23 +66,27 @@ public enum TestGroup {
 	 * Parse the specified comma separated string of groups.
 	 * @param value the comma separated string of groups
 	 * @return a set of groups
+	 * @throws IllegalArgumentException if any specified group name is not a
+	 * valid {@link TestGroup}
 	 */
-	public static Set<TestGroup> parse(String value) {
-		if (value == null || "".equals(value)) {
+	public static Set<TestGroup> parse(String value) throws IllegalArgumentException {
+		if (!StringUtils.hasText(value)) {
 			return Collections.emptySet();
 		}
+		String originalValue = value;
+		value = value.trim();
 		if ("ALL".equalsIgnoreCase(value)) {
 			return EnumSet.allOf(TestGroup.class);
 		}
 		if (value.toUpperCase().startsWith("ALL-")) {
-			Set<TestGroup> groups = new HashSet<>(EnumSet.allOf(TestGroup.class));
-			groups.removeAll(parseGroups(value.substring(4)));
+			Set<TestGroup> groups = EnumSet.allOf(TestGroup.class);
+			groups.removeAll(parseGroups(originalValue, value.substring(4)));
 			return groups;
 		}
-		return parseGroups(value);
+		return parseGroups(originalValue, value);
 	}
 
-	private static Set<TestGroup> parseGroups(String value) {
+	private static Set<TestGroup> parseGroups(String originalValue, String value) throws IllegalArgumentException {
 		Set<TestGroup> groups = new HashSet<>();
 		for (String group : value.split(",")) {
 			try {
@@ -90,7 +95,7 @@ public enum TestGroup {
 			catch (IllegalArgumentException ex) {
 				throw new IllegalArgumentException(format(
 						"Unable to find test group '%s' when parsing testGroups value: '%s'. " +
-						"Available groups include: [%s]", group.trim(), value,
+						"Available groups include: [%s]", group.trim(), originalValue,
 						StringUtils.arrayToCommaDelimitedString(TestGroup.values())));
 			}
 		}
diff --git a/spring-core/src/test/java/org/springframework/tests/TestGroupTests.java b/spring-core/src/test/java/org/springframework/tests/TestGroupTests.java
index c5c1db9b76..fa31022617 100644
--- a/spring-core/src/test/java/org/springframework/tests/TestGroupTests.java
+++ b/spring-core/src/test/java/org/springframework/tests/TestGroupTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,7 +18,6 @@ package org.springframework.tests;
 
 import java.util.Collections;
 import java.util.EnumSet;
-import java.util.HashSet;
 import java.util.Set;
 
 import org.junit.Rule;
@@ -32,6 +31,7 @@ import static org.junit.Assert.*;
  * Tests for {@link TestGroup}.
  *
  * @author Phillip Webb
+ * @author Sam Brannen
  */
 public class TestGroupTests {
 
@@ -40,29 +40,34 @@ public class TestGroupTests {
 
 
 	@Test
-	public void parseNull() throws Exception {
-		assertThat(TestGroup.parse(null), equalTo(Collections.<TestGroup> emptySet()));
+	public void parseNull() {
+		assertThat(TestGroup.parse(null), equalTo(Collections.emptySet()));
 	}
 
 	@Test
-	public void parseEmptyString() throws Exception {
-		assertThat(TestGroup.parse(""), equalTo(Collections.<TestGroup> emptySet()));
+	public void parseEmptyString() {
+		assertThat(TestGroup.parse(""), equalTo(Collections.emptySet()));
 	}
 
 	@Test
-	public void parseWithSpaces() throws Exception {
-		assertThat(TestGroup.parse("PERFORMANCE,  PERFORMANCE"),
-				equalTo((Set<TestGroup>) EnumSet.of(TestGroup.PERFORMANCE)));
+	public void parseBlankString() {
+		assertThat(TestGroup.parse("     "), equalTo(Collections.emptySet()));
 	}
 
 	@Test
-	public void parseInMixedCase() throws Exception {
+	public void parseWithSpaces() {
+		assertThat(TestGroup.parse(" PERFORMANCE,  PERFORMANCE "),
+				equalTo(EnumSet.of(TestGroup.PERFORMANCE)));
+	}
+
+	@Test
+	public void parseInMixedCase() {
 		assertThat(TestGroup.parse("performance,  PERFormaNCE"),
-				equalTo((Set<TestGroup>) EnumSet.of(TestGroup.PERFORMANCE)));
+				equalTo(EnumSet.of(TestGroup.PERFORMANCE)));
 	}
 
 	@Test
-	public void parseMissing() throws Exception {
+	public void parseMissing() {
 		thrown.expect(IllegalArgumentException.class);
 		thrown.expectMessage("Unable to find test group 'missing' when parsing " +
 				"testGroups value: 'performance, missing'. Available groups include: " +
@@ -71,15 +76,24 @@ public class TestGroupTests {
 	}
 
 	@Test
-	public void parseAll() throws Exception {
-		assertThat(TestGroup.parse("all"), equalTo((Set<TestGroup>)EnumSet.allOf(TestGroup.class)));
+	public void parseAll() {
+		assertThat(TestGroup.parse("all"), equalTo(EnumSet.allOf(TestGroup.class)));
 	}
 
 	@Test
-	public void parseAllExcept() throws Exception {
-		Set<TestGroup> expected = new HashSet<>(EnumSet.allOf(TestGroup.class));
+	public void parseAllExceptPerformance() {
+		Set<TestGroup> expected = EnumSet.allOf(TestGroup.class);
 		expected.remove(TestGroup.PERFORMANCE);
 		assertThat(TestGroup.parse("all-performance"), equalTo(expected));
 	}
 
+	@Test
+	public void parseAllExceptMissing() {
+		thrown.expect(IllegalArgumentException.class);
+		thrown.expectMessage("Unable to find test group 'missing' when parsing " +
+				"testGroups value: 'all-missing'. Available groups include: " +
+				"[LONG_RUNNING,PERFORMANCE,JMXMP,CI]");
+		TestGroup.parse("all-missing");
+	}
+
 }