Merge "Fix for: ConfigSuite calls @BeforeClass, @AfterClass, @ClassRule twice"
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 8064482..18a693f 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -202,7 +202,6 @@
import org.eclipse.jgit.util.FS;
import org.eclipse.jgit.util.SystemReader;
import org.junit.After;
-import org.junit.AfterClass;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Rule;
@@ -365,7 +364,7 @@
}
}
- @AfterClass
+ @ConfigSuite.AfterConfig
public static void stopCommonServer() throws Exception {
if (commonServer != null) {
try {
diff --git a/java/com/google/gerrit/testing/ConfigSuite.java b/java/com/google/gerrit/testing/ConfigSuite.java
index d7ae397..3101c48 100644
--- a/java/com/google/gerrit/testing/ConfigSuite.java
+++ b/java/com/google/gerrit/testing/ConfigSuite.java
@@ -25,7 +25,9 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import java.lang.annotation.Annotation;
+import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
@@ -35,11 +37,15 @@
import java.lang.reflect.Type;
import java.util.List;
import java.util.Map;
+import org.junit.internal.runners.statements.RunAfters;
+import org.junit.internal.runners.statements.RunBefores;
+import org.junit.rules.TestRule;
import org.junit.runner.Runner;
import org.junit.runners.BlockJUnit4ClassRunner;
import org.junit.runners.Suite;
import org.junit.runners.model.FrameworkMethod;
import org.junit.runners.model.InitializationError;
+import org.junit.runners.model.Statement;
/**
* Suite to run tests with different {@code gerrit.config} values.
@@ -47,6 +53,9 @@
* <p>For each {@link Config} method in the class and base classes, a new group of tests is created
* with the {@link Parameter} field set to the config.
*
+ * <p>Additional actions can be executed before or after each group of tests using
+ * {@literal @}BeforeConfig, {@literal @}AfterConfig or {@literal @}ConfigRule annotations.
+ *
* <pre>
* {@literal @}RunWith(ConfigSuite.class)
* public abstract class MyAbstractTest {
@@ -128,6 +137,38 @@
@Retention(RUNTIME)
public static @interface Name {}
+ /**
+ * Annotation for methods which should be run after executing group of tests with a new
+ * configuration.
+ *
+ * <p>Works similar to {@link org.junit.AfterClass}, but a method can be executed multiple times
+ * if a test class provides multiple configs.
+ */
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target(ElementType.METHOD)
+ public @interface AfterConfig {}
+
+ /**
+ * Annotation for methods which should be run before executing group of tests with a new
+ * configuration.
+ *
+ * <p>Works similar to {@link org.junit.BeforeClass}, but a method can be executed multiple times
+ * if a test class provides multiple configs.
+ */
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target(ElementType.METHOD)
+ public @interface BeforeConfig {}
+
+ /**
+ * Annotation for fields or methods which wraps all tests with the same config
+ *
+ * <p>Works similar to {@link org.junit.ClassRule}, but Statement evaluates multiple time - ones
+ * for each config provided by a test class.
+ */
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.FIELD, ElementType.METHOD})
+ public @interface ConfigRule {}
+
private static class ConfigRunner extends BlockJUnit4ClassRunner {
private final org.eclipse.jgit.lib.Config cfg;
private final Field parameterField;
@@ -168,6 +209,26 @@
String n = method.getName();
return name == null ? n : n + "[" + name + "]";
}
+
+ @Override
+ protected Statement withBeforeClasses(Statement statement) {
+ List<FrameworkMethod> befores = getTestClass().getAnnotatedMethods(BeforeConfig.class);
+ return befores.isEmpty() ? statement : new RunBefores(statement, befores, null);
+ }
+
+ @Override
+ protected Statement withAfterClasses(Statement statement) {
+ List<FrameworkMethod> afters = getTestClass().getAnnotatedMethods(AfterConfig.class);
+ return afters.isEmpty() ? statement : new RunAfters(statement, afters, null);
+ }
+
+ @Override
+ protected List<TestRule> classRules() {
+ List<TestRule> result =
+ getTestClass().getAnnotatedMethodValues(null, ConfigRule.class, TestRule.class);
+ result.addAll(getTestClass().getAnnotatedFieldValues(null, ConfigRule.class, TestRule.class));
+ return result;
+ }
}
private static List<Runner> runnersFor(Class<?> clazz) {
diff --git a/java/com/google/gerrit/testing/SystemPropertiesTestRule.java b/java/com/google/gerrit/testing/SystemPropertiesTestRule.java
new file mode 100644
index 0000000..c7a3542
--- /dev/null
+++ b/java/com/google/gerrit/testing/SystemPropertiesTestRule.java
@@ -0,0 +1,67 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// 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 com.google.gerrit.testing;
+
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.common.Nullable;
+import java.util.Map;
+import java.util.Optional;
+import org.junit.rules.ExternalResource;
+
+/** Setup system properties before tests and return previous value after tests are finished */
+public class SystemPropertiesTestRule extends ExternalResource {
+ ImmutableMap<String, Optional<String>> properties;
+ @Nullable ImmutableMap<String, Optional<String>> previousValues;
+
+ public SystemPropertiesTestRule(String key, String value) {
+ this(ImmutableMap.of(key, Optional.of(value)));
+ }
+
+ public SystemPropertiesTestRule(Map<String, Optional<String>> properties) {
+ this.properties = ImmutableMap.copyOf(properties);
+ }
+
+ @Override
+ protected void before() throws Throwable {
+ super.before();
+ checkState(
+ previousValues == null,
+ "after() wasn't called after the previous call to the before() method");
+ ImmutableMap.Builder<String, Optional<String>> previousValuesBuilder = ImmutableMap.builder();
+ for (String key : properties.keySet()) {
+ previousValuesBuilder.put(key, Optional.ofNullable(System.getProperty(key)));
+ }
+ previousValues = previousValuesBuilder.build();
+ properties.entrySet().stream().forEach(this::setSystemProperty);
+ }
+
+ @Override
+ protected void after() {
+ checkState(previousValues != null, "before() wasn't called");
+ previousValues.entrySet().stream().forEach(this::setSystemProperty);
+ previousValues = null;
+ super.after();
+ }
+
+ private void setSystemProperty(Map.Entry<String, Optional<String>> keyValue) {
+ if (keyValue.getValue().isPresent()) {
+ System.setProperty(keyValue.getKey(), keyValue.getValue().get());
+ } else {
+ System.clearProperty(keyValue.getKey());
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/index/DefaultIndexBindingIT.java b/javatests/com/google/gerrit/acceptance/testsuite/index/DefaultIndexBindingIT.java
index f5e4e09..8e0d4bb 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/index/DefaultIndexBindingIT.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/index/DefaultIndexBindingIT.java
@@ -20,29 +20,19 @@
import com.google.gerrit.index.IndexType;
import com.google.gerrit.index.testing.AbstractFakeIndex;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
+import com.google.gerrit.testing.SystemPropertiesTestRule;
import javax.inject.Inject;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import org.junit.ClassRule;
import org.junit.Test;
/** Test to check that the expected index backend was bound depending on sys/env properties. */
public class DefaultIndexBindingIT extends AbstractDaemonTest {
+ @ClassRule
+ public static SystemPropertiesTestRule systemProperties =
+ new SystemPropertiesTestRule(IndexType.SYS_PROP, "");
@Inject private ChangeIndexCollection changeIndex;
- private static String propertyBeforeTest;
-
- @BeforeClass
- public static void setup() {
- propertyBeforeTest = System.getProperty(IndexType.SYS_PROP);
- System.setProperty(IndexType.SYS_PROP, "");
- }
-
- @AfterClass
- public static void teardown() {
- System.setProperty(IndexType.SYS_PROP, propertyBeforeTest);
- }
-
@Test
public void fakeIsBoundByDefault() throws Exception {
assertThat(System.getProperty(IndexType.SYS_PROP)).isEmpty();
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/index/FakeIndexBindingIT.java b/javatests/com/google/gerrit/acceptance/testsuite/index/FakeIndexBindingIT.java
index 4122426..acb2e5a 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/index/FakeIndexBindingIT.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/index/FakeIndexBindingIT.java
@@ -20,29 +20,19 @@
import com.google.gerrit.index.IndexType;
import com.google.gerrit.index.testing.AbstractFakeIndex;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
+import com.google.gerrit.testing.SystemPropertiesTestRule;
import javax.inject.Inject;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import org.junit.ClassRule;
import org.junit.Test;
/** Test to check that the expected index backend was bound depending on sys/env properties. */
public class FakeIndexBindingIT extends AbstractDaemonTest {
+ @ClassRule
+ public static SystemPropertiesTestRule systemProperties =
+ new SystemPropertiesTestRule(IndexType.SYS_PROP, "fake");
@Inject private ChangeIndexCollection changeIndex;
- private static String propertyBeforeTest;
-
- @BeforeClass
- public static void setup() {
- propertyBeforeTest = System.getProperty(IndexType.SYS_PROP);
- System.setProperty(IndexType.SYS_PROP, "fake");
- }
-
- @AfterClass
- public static void teardown() {
- System.setProperty(IndexType.SYS_PROP, propertyBeforeTest);
- }
-
@Test
public void fakeIsBoundWhenConfigured() throws Exception {
assertThat(System.getProperty(IndexType.SYS_PROP)).isEqualTo("fake");
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/index/LuceneIndexBindingIT.java b/javatests/com/google/gerrit/acceptance/testsuite/index/LuceneIndexBindingIT.java
index 31e31fd..5dd6f01 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/index/LuceneIndexBindingIT.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/index/LuceneIndexBindingIT.java
@@ -20,29 +20,19 @@
import com.google.gerrit.index.IndexType;
import com.google.gerrit.lucene.LuceneChangeIndex;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
+import com.google.gerrit.testing.SystemPropertiesTestRule;
import javax.inject.Inject;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import org.junit.ClassRule;
import org.junit.Test;
/** Test to check that the expected index backend was bound depending on sys/env properties. */
public class LuceneIndexBindingIT extends AbstractDaemonTest {
+ @ClassRule
+ public static SystemPropertiesTestRule systemProperties =
+ new SystemPropertiesTestRule(IndexType.SYS_PROP, "lucene");
@Inject private ChangeIndexCollection changeIndex;
- private static String propertyBeforeTest;
-
- @BeforeClass
- public static void setup() {
- propertyBeforeTest = System.getProperty(IndexType.SYS_PROP);
- System.setProperty(IndexType.SYS_PROP, "lucene");
- }
-
- @AfterClass
- public static void teardown() {
- System.setProperty(IndexType.SYS_PROP, propertyBeforeTest);
- }
-
@Test
public void luceneIsBoundWhenConfigured() throws Exception {
assertThat(System.getProperty(IndexType.SYS_PROP)).isEqualTo("lucene");
diff --git a/javatests/com/google/gerrit/testing/BUILD b/javatests/com/google/gerrit/testing/BUILD
index 5774707..9443b0d 100644
--- a/javatests/com/google/gerrit/testing/BUILD
+++ b/javatests/com/google/gerrit/testing/BUILD
@@ -7,6 +7,7 @@
deps = [
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/testing:gerrit-test-util",
+ "//lib:jgit",
"//lib/truth",
],
)
diff --git a/javatests/com/google/gerrit/testing/ConfigSuiteTest.java b/javatests/com/google/gerrit/testing/ConfigSuiteTest.java
new file mode 100644
index 0000000..1ec30da
--- /dev/null
+++ b/javatests/com/google/gerrit/testing/ConfigSuiteTest.java
@@ -0,0 +1,266 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// 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 com.google.gerrit.testing;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.verify;
+
+import com.google.gerrit.testing.ConfigSuite.AfterConfig;
+import com.google.gerrit.testing.ConfigSuite.BeforeConfig;
+import com.google.gerrit.testing.ConfigSuite.ConfigRule;
+import org.eclipse.jgit.lib.Config;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TestRule;
+import org.junit.runner.Description;
+import org.junit.runner.Result;
+import org.junit.runner.RunWith;
+import org.junit.runner.notification.RunNotifier;
+import org.junit.runners.model.Statement;
+import org.mockito.InOrder;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class ConfigSuiteTest {
+ @Mock private ConfigBasedTestListener configBasedTestListener;
+ private RunNotifier notifier;
+
+ @Before
+ public void setUp() {
+ notifier = new RunNotifier();
+ ConfigBasedTest.setConfigBasedTestListener(configBasedTestListener);
+ }
+
+ @After
+ public void tearDown() {
+ ConfigBasedTest.setConfigBasedTestListener(null);
+ }
+
+ @Test
+ public void methodsExecuteInCorrectOrder() throws Exception {
+ InOrder inOrder = Mockito.inOrder(configBasedTestListener);
+ new ConfigSuite(ConfigBasedTest.class).run(notifier);
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).classRuleExecuted();
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).beforeClassExecuted();
+ // default config
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).configRuleExectued();
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).beforeConfigExecuted();
+ inOrder.verify(configBasedTestListener, Mockito.times(2)).testExecuted(any(), any(), any());
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).afterConfigExecuted();
+ // first config
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).configRuleExectued();
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).beforeConfigExecuted();
+ inOrder.verify(configBasedTestListener, Mockito.times(2)).testExecuted(any(), any(), any());
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).afterConfigExecuted();
+ // second config
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).configRuleExectued();
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).beforeConfigExecuted();
+ inOrder.verify(configBasedTestListener, Mockito.times(2)).testExecuted(any(), any(), any());
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).afterConfigExecuted();
+
+ inOrder.verify(configBasedTestListener, Mockito.times(1)).afterClassExecuted();
+ }
+
+ @Test
+ public void beforeClassExecutedOnce() throws Exception {
+ new ConfigSuite(ConfigBasedTest.class).run(notifier);
+ verify(configBasedTestListener, Mockito.times(1)).beforeClassExecuted();
+ }
+
+ @Test
+ public void afterClassExecutedOnce() throws Exception {
+ new ConfigSuite(ConfigBasedTest.class).run(notifier);
+ verify(configBasedTestListener, Mockito.times(1)).afterClassExecuted();
+ }
+
+ @Test
+ public void classRuleExecutedOnlyOnce() throws Exception {
+ new ConfigSuite(ConfigBasedTest.class).run(notifier);
+ verify(configBasedTestListener, Mockito.times(1)).classRuleExecuted();
+ }
+
+ @Test
+ public void beforeConfigExecutedForEachConfig() throws Exception {
+ new ConfigSuite(ConfigBasedTest.class).run(notifier);
+ // default, firstConfig, secondConfig
+ verify(configBasedTestListener, Mockito.times(3)).beforeConfigExecuted();
+ }
+
+ @Test
+ public void afterConfigExecutedForEachConfig() throws Exception {
+ new ConfigSuite(ConfigBasedTest.class).run(notifier);
+ // default, firstConfig, secondConfig
+ verify(configBasedTestListener, Mockito.times(3)).afterConfigExecuted();
+ }
+
+ @Test
+ public void configRuleExecutedForEachConfig() throws Exception {
+ new ConfigSuite(ConfigBasedTest.class).run(notifier);
+ // default, firstConfig, secondConfig
+ verify(configBasedTestListener, Mockito.times(3)).afterConfigExecuted();
+ }
+
+ @Test
+ public void testsExecuteWithCorrectConfigAndName() throws Exception {
+ new ConfigSuite(ConfigBasedTest.class).run(notifier);
+ verify(configBasedTestListener, Mockito.times(6)).testExecuted(any(), any(), any());
+
+ verify(configBasedTestListener, Mockito.times(1)).testExecuted("test1", "default", null);
+ verify(configBasedTestListener, Mockito.times(1)).testExecuted("test2", "default", null);
+ verify(configBasedTestListener, Mockito.times(1))
+ .testExecuted("test1", "firstValue", "firstConfig");
+ verify(configBasedTestListener, Mockito.times(1))
+ .testExecuted("test2", "firstValue", "firstConfig");
+ verify(configBasedTestListener, Mockito.times(1))
+ .testExecuted("test1", "secondValue", "secondConfig");
+ verify(configBasedTestListener, Mockito.times(1))
+ .testExecuted("test2", "secondValue", "secondConfig");
+ }
+
+ @Test
+ public void testResultWasSuccessful() throws Exception {
+ Result result = new Result();
+ notifier.addListener(result.createListener());
+ new ConfigSuite(ConfigBasedTest.class).run(notifier);
+ assertThat(result.wasSuccessful()).isTrue();
+ }
+
+ @Test
+ public void failedTestResultNotMissed() throws Exception {
+ Result result = new Result();
+ notifier.addListener(result.createListener());
+ new ConfigSuite(FailedConfigBasedTest.class).run(notifier);
+ // 3 fails with 3 different configs
+ assertThat(result.wasSuccessful()).isFalse();
+ assertThat(result.getFailureCount()).isEqualTo(3);
+ }
+
+ interface ConfigBasedTestListener {
+ void beforeClassExecuted();
+
+ void afterClassExecuted();
+
+ void classRuleExecuted();
+
+ void configRuleExectued();
+
+ void testExecuted(String testName, String testValue, String configName);
+
+ void beforeConfigExecuted();
+
+ void afterConfigExecuted();
+ }
+
+ public static class ConfigBasedTest {
+ private static ConfigBasedTestListener listener;
+
+ public static void setConfigBasedTestListener(ConfigBasedTestListener listener) {
+ ConfigBasedTest.listener = listener;
+ }
+
+ @BeforeClass
+ public static void beforeClass() {
+ listener.beforeClassExecuted();
+ }
+
+ @AfterClass
+ public static void afterClass() {
+ listener.afterClassExecuted();
+ }
+
+ @ClassRule
+ public static TestRule classRule =
+ new TestRule() {
+ @Override
+ public Statement apply(Statement statement, Description description) {
+ return new Statement() {
+ @Override
+ public void evaluate() throws Throwable {
+ listener.classRuleExecuted();
+ statement.evaluate();
+ }
+ };
+ }
+ };
+
+ @ConfigRule
+ public static TestRule configRule =
+ new TestRule() {
+ @Override
+ public Statement apply(Statement statement, Description description) {
+ return new Statement() {
+ @Override
+ public void evaluate() throws Throwable {
+ listener.configRuleExectued();
+ statement.evaluate();
+ }
+ };
+ }
+ };
+
+ @BeforeConfig
+ public static void beforeConfig() {
+ listener.beforeConfigExecuted();
+ }
+
+ @AfterConfig
+ public static void afterConfig() {
+ listener.afterConfigExecuted();
+ }
+
+ @ConfigSuite.Config
+ public static Config firstConfig() {
+ Config cfg = new Config();
+ cfg.setString("gerrit", null, "testValue", "firstValue");
+ return cfg;
+ }
+
+ @ConfigSuite.Config
+ public static Config secondConfig() {
+ Config cfg = new Config();
+ cfg.setString("gerrit", null, "testValue", "secondValue");
+ return cfg;
+ }
+
+ @ConfigSuite.Parameter public Config config;
+ @ConfigSuite.Name public String name;
+
+ @Test
+ public void test1() {
+ String testValue = config.getString("gerrit", null, "testValue");
+ listener.testExecuted("test1", testValue != null ? testValue : "default", name);
+ }
+
+ @Test
+ public void test2() {
+ String testValue = config.getString("gerrit", null, "testValue");
+ listener.testExecuted("test2", testValue != null ? testValue : "default", name);
+ }
+ }
+
+ public static class FailedConfigBasedTest extends ConfigBasedTest {
+ @Test
+ public void failedTest() {
+ assertThat(true).isFalse();
+ }
+ }
+}
