Shared mini test suite minor follow on (#5555)

* Minor follow on to #5536

- Noticed several SharedMiniClusterBase ITs were not calling `stopMiniCluster()`. All tests which start a mini cluster now stop the mini cluster as well.
- Added some javadoc changes to SharedMiniClusterBase and
  SimpleSharedMacTestSuite
- Small typo fixes
- Changed junit-platform-suite-engine dependency in test/pom.xml from being compile scope to being test scope

Author: kevinrr888

minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java

@@ -74,7 +74,7 @@ public class MiniAccumuloConfigImpl {
   private String rootPassword = null;
   private Map<String,String> hadoopConfOverrides = new HashMap<>();
   private Map<String,String> siteConfig = new HashMap<>();
-  private Map<String,String> configuredSiteConig = new HashMap<>();
+  private Map<String,String> configuredSiteConfig = new HashMap<>();
   private Map<String,String> clientProps = new HashMap<>();
   private Map<ServerType,Long> memoryConfig = new HashMap<>();
   private final EnumMap<ServerType,Class<?>> serverTypeClasses =
@@ -315,7 +315,7 @@ public MiniAccumuloConfigImpl setClientProps(Map<String,String> clientProps) {
 
   private MiniAccumuloConfigImpl _setSiteConfig(Map<String,String> siteConfig) {
     this.siteConfig = new HashMap<>(siteConfig);
-    this.configuredSiteConig = new HashMap<>(siteConfig);
+    this.configuredSiteConfig = new HashMap<>(siteConfig);
     return this;
   }
 
@@ -439,7 +439,7 @@ public Map<String,String> getClientProps() {
   }
 
   public Map<String,String> getConfiguredSiteConfig() {
-    return new HashMap<>(configuredSiteConig);
+    return new HashMap<>(configuredSiteConfig);
   }
 
   /**

test/pom.xml

@@ -202,10 +202,6 @@
       <groupId>org.junit.platform</groupId>
       <artifactId>junit-platform-suite-api</artifactId>
     </dependency>
-    <dependency>
-      <groupId>org.junit.platform</groupId>
-      <artifactId>junit-platform-suite-engine</artifactId>
-    </dependency>
     <dependency>
       <groupId>org.slf4j</groupId>
       <artifactId>slf4j-api</artifactId>
@@ -231,6 +227,11 @@
       <artifactId>log4j-slf4j2-impl</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.junit.platform</groupId>
+      <artifactId>junit-platform-suite-engine</artifactId>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
   <build>
     <pluginManagement>

test/src/main/java/org/apache/accumulo/harness/MiniClusterConfigurationCallback.java

@@ -25,6 +25,7 @@
  * Callback interface to inject configuration into the MiniAccumuloCluster or Hadoop core-site.xml
  * file used by the MiniAccumuloCluster
  */
+@FunctionalInterface
 public interface MiniClusterConfigurationCallback {
 
   class NoCallback implements MiniClusterConfigurationCallback {

test/src/main/java/org/apache/accumulo/harness/SharedMiniClusterBase.java

@@ -49,6 +49,7 @@
 import org.apache.accumulo.core.clientImpl.Namespace;
 import org.apache.accumulo.core.conf.ClientProperty;
 import org.apache.accumulo.miniclusterImpl.MiniAccumuloClusterImpl;
+import org.apache.accumulo.suites.SimpleSharedMacTestSuite;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.CommonConfigurationKeysPublic;
 import org.apache.hadoop.security.UserGroupInformation;
@@ -60,17 +61,20 @@
  * Integration-Test base class which starts one MAC for the entire Integration Test. This IT type is
  * faster and more geared for testing typical, expected behavior of a cluster. For more advanced
  * testing see {@link AccumuloClusterHarness}
- *
+ * <p>
  * There isn't a good way to build this off of the {@link AccumuloClusterHarness} (as would be the
  * logical place) because we need to start the MiniAccumuloCluster in a static BeforeAll-annotated
  * method. Because it is static and invoked before any other BeforeAll methods in the
  * implementation, the actual test classes can't expose any information to tell the base class that
  * it is to perform the one-MAC-per-class semantics.
- *
+ * <p>
  * Implementations of this class must be sure to invoke {@link #startMiniCluster()} or
  * {@link #startMiniClusterWithConfig(MiniClusterConfigurationCallback)} in a method annotated with
  * the {@link org.junit.jupiter.api.BeforeAll} JUnit annotation and {@link #stopMiniCluster()} in a
  * method annotated with the {@link org.junit.jupiter.api.AfterAll} JUnit annotation.
+ * <p>
+ * Implementations of this class should also consider if they can be added to
+ * {@link SimpleSharedMacTestSuite}. See the suites description to determine if they can be added.
  */
 @Tag(MINI_CLUSTER_ONLY)
 public abstract class SharedMiniClusterBase extends AccumuloITBase implements ClusterUsers {
@@ -84,7 +88,8 @@ public abstract class SharedMiniClusterBase extends AccumuloITBase implements Cl
   private static TestingKdc krb;
 
   /**
-   * Starts a MiniAccumuloCluster instance with the default configuration.
+   * Starts a MiniAccumuloCluster instance with the default configuration. This method is
+   * idempotent: necessitated by {@link SimpleSharedMacTestSuite}.
    */
   public static void startMiniCluster() throws Exception {
     startMiniClusterWithConfig(MiniClusterConfigurationCallback.NO_CALLBACK);
@@ -93,6 +98,7 @@ public static void startMiniCluster() throws Exception {
   /**
    * Starts a MiniAccumuloCluster instance with the default configuration but also provides the
    * caller the opportunity to update the configuration before the MiniAccumuloCluster is started.
+   * This method is idempotent: necessitated by {@link SimpleSharedMacTestSuite}.
    *
    * @param miniClusterCallback A callback to configure the minicluster before it is started.
    */
@@ -148,14 +154,11 @@ private static String getTestClassName() {
    */
   public static synchronized void stopMiniCluster() {
     if (STOP_DISABLED.get()) {
-      // If stop is disabled, then we are likely running a
-      // test class that is part of a larger suite. We don't
-      // want to shut down the cluster, but we should clean
-      // up any tables that were created, but not deleted,
-      // by the test class. This will prevent issues with
-      // subsequent tests that count objects or initiate
-      // compactions and wait for them, but some other table
-      // from a prior test is compacting.
+      // If stop is disabled, then we are likely running a test class that is part of a larger
+      // suite. We don't want to shut down the cluster, but we should clean up any tables or
+      // namespaces that were created, but not deleted, by the test class. This will prevent issues
+      // with subsequent tests that count objects or initiate compactions and wait for them, but
+      // some other table from a prior test is compacting.
       try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) {
         for (String tableName : client.tableOperations().list()) {
           if (!tableName.startsWith(Namespace.ACCUMULO.name() + ".")) {

test/src/main/java/org/apache/accumulo/suites/SimpleSharedMacTestSuite.java

@@ -20,14 +20,30 @@
 
 import static org.apache.accumulo.harness.AccumuloITBase.SIMPLE_MINI_CLUSTER_SUITE;
 
+import org.apache.accumulo.harness.MiniClusterConfigurationCallback;
 import org.apache.accumulo.harness.SharedMiniClusterBase;
+import org.junit.jupiter.api.Tag;
 import org.junit.platform.suite.api.AfterSuite;
 import org.junit.platform.suite.api.BeforeSuite;
 import org.junit.platform.suite.api.IncludeClassNamePatterns;
 import org.junit.platform.suite.api.IncludeTags;
 import org.junit.platform.suite.api.SelectPackages;
 import org.junit.platform.suite.api.Suite;
 
+/**
+ * This test suite is used to run applicable ITs against a single, shared cluster, starting and
+ * stopping the cluster only once for the duration of the suite. This avoids starting and stopping a
+ * cluster per IT, providing some speedup. An IT is applicable if:
+ * <p>
+ * 1) It is a subclass of {@link SharedMiniClusterBase}, meaning it starts and stops a single
+ * cluster for the entire IT.
+ * <p>
+ * 2) It does not start the cluster with any custom config (i.e., it does not use
+ * {@link SharedMiniClusterBase#startMiniClusterWithConfig(MiniClusterConfigurationCallback)})
+ * <p>
+ * An IT which meets this criteria should be tagged (using JUnit {@link Tag}) with
+ * {@link #SIMPLE_MINI_CLUSTER_SUITE} to be added to the suite.
+ */
 @Suite
 @SelectPackages("org.apache.accumulo.test") // look in this package and subpackages
 @IncludeTags(SIMPLE_MINI_CLUSTER_SUITE) // for tests with this tag

test/src/main/java/org/apache/accumulo/test/compaction/ExternalCompaction_1_IT.java

@@ -139,6 +139,7 @@ public static void afterTests() throws Exception {
     if (testLock != null) {
       testLock.unlock();
     }
+    stopMiniCluster();
   }
 
   public static class TestFilter extends Filter {

test/src/main/java/org/apache/accumulo/test/compaction/ExternalCompaction_3_IT.java

@@ -74,6 +74,7 @@
 import org.apache.hadoop.io.Text;
 import org.apache.thrift.TException;
 import org.apache.thrift.transport.TTransportException;
+import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
@@ -93,6 +94,11 @@ public static void beforeTests() throws Exception {
     startMiniClusterWithConfig(new ExternalCompaction3Config());
   }
 
+  @AfterAll
+  public static void afterTests() {
+    stopMiniCluster();
+  }
+
   @Test
   public void testMergeCancelsExternalCompaction() throws Exception {
 

test/src/main/java/org/apache/accumulo/test/compaction/FlakyExternalCompaction2IT.java

@@ -18,14 +18,12 @@
  */
 package org.apache.accumulo.test.compaction;
 
-import org.apache.accumulo.harness.SharedMiniClusterBase;
 import org.apache.accumulo.minicluster.ServerType;
 import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl;
 import org.apache.accumulo.test.ample.FlakyAmpleManager;
 import org.apache.accumulo.test.ample.FlakyAmpleServerContext;
 import org.apache.accumulo.test.ample.FlakyAmpleTserver;
 import org.apache.hadoop.conf.Configuration;
-import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 
 /**
@@ -52,9 +50,4 @@ public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration coreS
   public static void setup() throws Exception {
     startMiniClusterWithConfig(new FlakyExternalCompaction2Config());
   }
-
-  @AfterAll
-  public static void teardown() {
-    SharedMiniClusterBase.stopMiniCluster();
-  }
 }

test/src/main/java/org/apache/accumulo/test/functional/ManagerAssignmentIT.java

@@ -110,6 +110,7 @@ public static void beforeAll() throws Exception {
   @AfterAll
   public static void afterAll() {
     client.close();
+    SharedMiniClusterBase.stopMiniCluster();
   }
 
   @BeforeEach

test/src/main/java/org/apache/accumulo/test/functional/OnDemandTabletUnloadingIT.java

@@ -116,6 +116,7 @@ public static void beforeAll() throws Exception {
 
   @AfterAll
   public static void after() throws Exception {
+    SharedMiniClusterBase.stopMiniCluster();
     sink.close();
     metricConsumer.interrupt();
     metricConsumer.join();