Add TestNG groups to pulsar-broker (apache#9712)

* Replace package filter mechanism with testng groups.
* Isolate flaky tests for pulsar-broker
* Cleanup unused code and exceptions
* Make certain methods private to prevent surefire from reporting them.

Co-authored-by: Ali Ahmed <[email protected]>

.github/workflows/ci-unit-broker-other.yaml

@@ -66,9 +66,9 @@ jobs:
         if: steps.docs.outputs.changed_only == 'no'
         run: mvn -B -ntp -q clean install -Pcore-modules -DskipTests
 
-      - name: run unit test 'BROKER_CLIENT_OTHER'
+      - name: run unit test 'BROKER_FLAKY'
         if: steps.docs.outputs.changed_only == 'no'
-        run: ./build/run_unit_group.sh BROKER_CLIENT_OTHER
+        run: ./build/run_unit_group.sh BROKER_FLAKY
 
       - name: package surefire artifacts
         if: failure()

build/retry_integration.sh

@@ -0,0 +1,48 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you 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.
+#
+
+function fail {
+  echo $1 >&2
+  exit 1
+}
+
+function retry {
+  local n=1
+  local max=3
+  local delay=10
+
+  # cleanup running containers
+  docker kill $(docker ps -q)
+  docker rm $(docker ps -a -q)
+
+  while true; do
+    "$@" && break || {
+      if [[ $n -lt $max ]]; then
+        ((n++))
+        echo "Command failed. Attempt $n/$max:"
+        sleep $delay;
+      else
+        fail "The command has failed after $n attempts."
+      fi
+    }
+  done
+}
+
+retry "$@"

build/run_integration_group.sh

@@ -37,7 +37,7 @@ mvn_run_integration_test() {
   RETRY=""
   # wrap with retry.sh script if next parameter is "--retry"
   if [[ "$1" == "--retry" ]]; then
-    RETRY="./build/retry.sh"
+    RETRY="./build/retry_integration.sh"
     shift
   fi
   # skip wrapping with retry.sh script if next parameter is "--no-retry"
@@ -46,6 +46,7 @@ mvn_run_integration_test() {
     shift
   fi
   set -x
+
   # run the integration tests
   $RETRY mvn -B -ntp -DredirectTestOutputToFile=false -f tests/pom.xml test "$@"
   )
@@ -63,6 +64,7 @@ test_group_backwards_compat() {
 test_group_cli() {
   # run pulsar cli integration tests
   mvn_run_integration_test "$@" -DintegrationTestSuiteFile=pulsar-cli.xml -DintegrationTests
+
   # run pulsar auth integration tests
   mvn_run_integration_test "$@" -DintegrationTestSuiteFile=pulsar-auth.xml -DintegrationTests
 }
@@ -103,17 +105,21 @@ test_group_tiered_jcloud() {
 test_group_pulsar_connectors_thread() {
   # run integration function
   mvn_run_integration_test --retry "$@" -DintegrationTestSuiteFile=pulsar-thread.xml -DintegrationTests -Dgroups=function
+
   # run integration source
   mvn_run_integration_test --retry "$@" -DintegrationTestSuiteFile=pulsar-thread.xml -DintegrationTests -Dgroups=source
+
   # run integration sink
   mvn_run_integration_test --retry "$@" -DintegrationTestSuiteFile=pulsar-thread.xml -DintegrationTests -Dgroups=sink
 }
 
 test_group_pulsar_connectors_process() {
   # run integration function
   mvn_run_integration_test --retry "$@" -DintegrationTestSuiteFile=pulsar-process.xml -DintegrationTests -Dgroups=function
+
   # run integration source
   mvn_run_integration_test --retry "$@" -DintegrationTestSuiteFile=pulsar-process.xml -DintegrationTests -Dgroups=source
+
   # run integraion sink
   mvn_run_integration_test --retry "$@" -DintegrationTestSuiteFile=pulsar-process.xml -DintegrationTests -Dgroups=sink
 }

build/run_unit_group.sh

@@ -25,72 +25,27 @@ set -o errexit
 
 MVN_TEST_COMMAND='build/retry.sh mvn -B -ntp test'
 
+echo -n "Test Group : $TEST_GROUP"
+
 # Test Groups  -- start --
 function broker_group_1() {
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="**/AdminApiOffloadTest.java" \
-                                      -DtestForkCount=1 \
-                                      -DtestReuseFork=true
-
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="org/apache/pulsar/broker/**/*.java" \
-                                      -Dexclude='org/apache/pulsar/broker/zookeeper/**/*.java,
-                                                 org/apache/pulsar/broker/loadbalance/**/*.java,
-                                                 org/apache/pulsar/broker/service/**/*.java,
-                                                 **/AdminApiOffloadTest.java'
-
+  $MVN_TEST_COMMAND -pl pulsar-broker -Dgroups='broker'
 }
 
 function broker_group_2() {
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="**/MessagePublishBufferThrottleTest.java" \
-                                      -DtestForkCount=1 \
-                                      -DtestReuseFork=true
-
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="**/ReplicatorTest.java" \
-                                      -DtestForkCount=1 \
-                                      -DtestReuseFork=true
-
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="**/TopicOwnerTest.java" \
-                                      -DtestForkCount=1 \
-                                      -DtestReuseFork=true
-
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="**/AntiAffinityNamespaceGroupTest.java" \
-                                      -DtestForkCount=1 \
-                                      -DtestReuseFork=true
-
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="**/*StreamingDispatcher*Test.java" \
-                                      -DtestForkCount=1 \
-                                      -DtestReuseFork=true
-
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="org/apache/pulsar/broker/zookeeper/**/*.java,
-                                                 org/apache/pulsar/broker/loadbalance/**/*.java,
-                                                 org/apache/pulsar/broker/service/**/*.java" \
-                                      -Dexclude='**/ReplicatorTest.java,
-                                                 **/MessagePublishBufferThrottleTest.java,
-                                                 **/TopicOwnerTest.java,
-                                                 **/*StreamingDispatcher*Test.java,
-                                                 **/AntiAffinityNamespaceGroupTest.java'
+  $MVN_TEST_COMMAND -pl pulsar-broker -Dgroups='schema,utils,functions-worker,broker-io,broker-discovery,broker-compaction,broker-naming'
 }
 
 function broker_client_api() {
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="**/DispatcherBlockConsumerTest.java" \
-                                      -DtestForkCount=1 \
-                                      -DtestReuseFork=true
-
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="**/SimpleProducerConsumerTest.java" \
-                                      -DtestForkCount=1 \
-                                      -DtestReuseFork=true
-
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="org/apache/pulsar/client/api/**/*.java" \
-                                      -Dexclude='**/DispatcherBlockConsumerTest.java,
-                                                 **/SimpleProducerConsumerTest.java'
+  $MVN_TEST_COMMAND -pl pulsar-broker -Dgroups='broker-api'
 }
 
 function broker_client_impl() {
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dinclude="org/apache/pulsar/client/impl/**/*.java"
+  $MVN_TEST_COMMAND -pl pulsar-broker -Dgroups='broker-impl'
 }
 
-function broker_client_other() {
-  $MVN_TEST_COMMAND -pl pulsar-broker -Dexclude='org/apache/pulsar/broker/**/*.java,
-                                                 org/apache/pulsar/client/**/*.java'
+function broker_flaky() {
+  $MVN_TEST_COMMAND -pl pulsar-broker -Dgroups='flaky' -DtestForkCount=1 -DtestReuseFork=false
 }
 
 function proxy() {
@@ -105,21 +60,17 @@ function other() {
                                                 **/PrimitiveSchemaTest.java,
                                                 BlobStoreManagedLedgerOffloaderTest.java'
 
-  $MVN_TEST_COMMAND -pl managed-ledger -Dinclude="**/ManagedLedgerTest.java" \
-                                       -DtestForkCount=1 \
-                                       -DtestReuseFork=true
+  $MVN_TEST_COMMAND -pl managed-ledger -Dinclude='**/ManagedLedgerTest.java' \
+                                       -DtestForkCount=1
 
-  $MVN_TEST_COMMAND -pl pulsar-sql/presto-pulsar-plugin -Dinclude="**/TestPulsarKeyValueSchemaHandler.java" \
-                                                        -DtestForkCount=1 \
-                                                        -DtestReuseFork=true
+  $MVN_TEST_COMMAND -pl pulsar-sql/presto-pulsar-plugin -Dinclude='**/TestPulsarKeyValueSchemaHandler.java' \
+                                                        -DtestForkCount=1
 
-  $MVN_TEST_COMMAND -pl pulsar-client -Dinclude="**/PrimitiveSchemaTest.java" \
-                                      -DtestForkCount=1 \
-                                      -DtestReuseFork=true
+  $MVN_TEST_COMMAND -pl pulsar-client -Dinclude='**/PrimitiveSchemaTest.java' \
+                                      -DtestForkCount=1
 
-  $MVN_TEST_COMMAND -pl tiered-storage/jcloud -Dinclude="**/BlobStoreManagedLedgerOffloaderTest.java" \
-                                              -DtestForkCount=1 \
-                                              -DtestReuseFork=true
+  $MVN_TEST_COMMAND -pl tiered-storage/jcloud -Dinclude='**/BlobStoreManagedLedgerOffloaderTest.java' \
+                                              -DtestForkCount=1
 }
 
 # Test Groups  -- end --
@@ -146,8 +97,8 @@ case $TEST_GROUP in
     broker_client_impl
     ;;
 
-  BROKER_CLIENT_OTHER)
-    broker_client_other
+  BROKER_FLAKY)
+    broker_flaky
     ;;
 
   PROXY)
@@ -163,4 +114,3 @@ case $TEST_GROUP in
     exit 1
     ;;
 esac
-

buildtools/src/main/java/org/apache/pulsar/tests/PulsarTestListener.java

@@ -35,7 +35,7 @@ public void onTestStart(ITestResult result) {
 
     @Override
     public void onTestSuccess(ITestResult result) {
-        System.out.format("------- SUCCESS -- %s.%s(%s)-------", result.getTestClass(),
+        System.out.format("------- SUCCESS -- %s.%s(%s)-------\n", result.getTestClass(),
                 result.getMethod().getMethodName(), Arrays.toString(result.getParameters()));
     }
 
@@ -52,7 +52,7 @@ public void onTestFailure(ITestResult result) {
 
     @Override
     public void onTestSkipped(ITestResult result) {
-        System.out.format("~~~~~~~~~ SKIPPED -- %s.%s(%s)-------", result.getTestClass(),
+        System.out.format("~~~~~~~~~ SKIPPED -- %s.%s(%s)-------\n", result.getTestClass(),
                 result.getMethod().getMethodName(), Arrays.toString(result.getParameters()));
     }
 

buildtools/src/main/java/org/apache/pulsar/tests/ThreadDumpUtil.java

@@ -40,7 +40,7 @@
 public class ThreadDumpUtil {
     static final String TEST_TIMED_OUT_PREFIX = "test timed out after";
 
-    private static String INDENT = "    ";
+    private static final String INDENT = "    ";
 
     public static String buildThreadDiagnosticString() {
         StringWriter sw = new StringWriter();

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

@@ -257,7 +257,7 @@ protected void validateAdminAccessForTenant(String tenant) {
     protected static void validateAdminAccessForTenant(PulsarService pulsar, String clientAppId,
                                                        String originalPrincipal, String tenant,
                                                        AuthenticationDataSource authenticationData)
-            throws RestException, Exception {
+            throws Exception {
         if (log.isDebugEnabled()) {
             log.debug("check admin access on tenant: {} - Authenticated: {} -- role: {}", tenant,
                     (isClientAuthenticated(clientAppId)), clientAppId);
@@ -778,12 +778,12 @@ private static ClusterData getOwnerFromPeerClusterList(PulsarService pulsar, Set
         return null;
     }
 
-    protected void checkConnect(TopicName topicName) throws RestException, Exception {
+    protected void checkConnect(TopicName topicName) throws Exception {
         checkAuthorization(pulsar(), topicName, clientAppId(), clientAuthData());
     }
 
     protected static void checkAuthorization(PulsarService pulsarService, TopicName topicName, String role,
-            AuthenticationDataSource authenticationData) throws RestException, Exception {
+            AuthenticationDataSource authenticationData) throws Exception {
         if (!pulsarService.getConfiguration().isAuthorizationEnabled()) {
             // No enforcing of authorization policies
             return;
@@ -1018,10 +1018,8 @@ protected CompletableFuture<Void> canUpdateCluster(String tenant, Set<String> ol
      *
      * @param broker
      *            Broker name
-     * @throws MalformedURLException
-     *             In case the redirect happens
      */
-    protected void validateBrokerName(String broker) throws MalformedURLException {
+    protected void validateBrokerName(String broker) {
         String brokerUrl = String.format("http://%s", broker);
         String brokerUrlTls = String.format("https://%s", broker);
         if (!brokerUrl.equals(pulsar().getSafeWebServiceAddress())
@@ -1089,8 +1087,8 @@ public static void deleteRecursive(BaseResources resources, final String pathRoo
 
     public static List<String> listSubTreeBFS(BaseResources resources, final String pathRoot)
             throws MetadataStoreException {
-        Deque<String> queue = new LinkedList<String>();
-        List<String> tree = new ArrayList<String>();
+        Deque<String> queue = new LinkedList<>();
+        List<String> tree = new ArrayList<>();
         queue.add(pathRoot);
         tree.add(pathRoot);
         while (true) {

pulsar-broker/src/test/java/org/apache/pulsar/PulsarBrokerStarterTest.java

@@ -35,10 +35,7 @@
 import org.apache.pulsar.broker.ServiceConfiguration;
 import org.testng.annotations.Test;
 
-/**
- * @version $Revision$<br>
- *          Created on Sep 6, 2012
- */
+@Test(groups = "broker")
 public class PulsarBrokerStarterTest {
 
     private File createValidBrokerConfigFile() throws FileNotFoundException {
@@ -92,7 +89,6 @@ private File createValidBrokerConfigFile() throws FileNotFoundException {
      * method returns a non-null {@link ServiceConfiguration} instance where all required settings are filled in and (2)
      * if the property variables inside the given property file are correctly referred to that returned object.
      */
-    @Test
     public void testLoadConfig() throws SecurityException, NoSuchMethodException, IOException, IllegalArgumentException,
             IllegalAccessException, InvocationTargetException {
 

pulsar-broker/src/test/java/org/apache/pulsar/broker/BookKeeperClientFactoryImplTest.java

@@ -44,6 +44,7 @@
 /**
  * Unit test {@link BookKeeperClientFactoryImpl}.
  */
+@Test(groups = "broker")
 public class BookKeeperClientFactoryImplTest {
 
     @Test

pulsar-broker/src/test/java/org/apache/pulsar/broker/SLAMonitoringTest.java

@@ -23,7 +23,6 @@
 import static org.testng.Assert.assertTrue;
 import static org.testng.Assert.fail;
 
-import java.net.MalformedURLException;
 import java.net.URL;
 import java.util.HashSet;
 import java.util.List;
@@ -40,7 +39,6 @@
 import org.apache.pulsar.broker.namespace.NamespaceService;
 import org.apache.pulsar.client.admin.PulsarAdmin;
 import org.apache.pulsar.client.admin.PulsarAdminException;
-import org.apache.pulsar.client.api.PulsarClientException;
 import org.apache.pulsar.common.policies.data.ClusterData;
 import org.apache.pulsar.common.policies.data.NamespaceOwnershipStatus;
 import org.apache.pulsar.common.policies.data.TenantInfo;
@@ -51,18 +49,24 @@
 import org.testng.annotations.Test;
 
 @Slf4j
+@Test(groups = "broker")
 public class SLAMonitoringTest {
     LocalBookkeeperEnsemble bkEnsemble;
 
-    ExecutorService executor = new ThreadPoolExecutor(5, 20, 30, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>());
+    ExecutorService executor =
+            new ThreadPoolExecutor(5,
+                    20,
+                    30,
+                    TimeUnit.SECONDS,
+                    new LinkedBlockingQueue<>());
 
     private static final int BROKER_COUNT = 5;
-    private int[] brokerWebServicePorts = new int[BROKER_COUNT];
-    private int[] brokerNativeBrokerPorts = new int[BROKER_COUNT];
-    private URL[] brokerUrls = new URL[BROKER_COUNT];
-    private PulsarService[] pulsarServices = new PulsarService[BROKER_COUNT];
-    private PulsarAdmin[] pulsarAdmins = new PulsarAdmin[BROKER_COUNT];
-    private ServiceConfiguration[] configurations = new ServiceConfiguration[BROKER_COUNT];
+    private final int[] brokerWebServicePorts = new int[BROKER_COUNT];
+    private final int[] brokerNativeBrokerPorts = new int[BROKER_COUNT];
+    private final URL[] brokerUrls = new URL[BROKER_COUNT];
+    private final PulsarService[] pulsarServices = new PulsarService[BROKER_COUNT];
+    private final PulsarAdmin[] pulsarAdmins = new PulsarAdmin[BROKER_COUNT];
+    private final ServiceConfiguration[] configurations = new ServiceConfiguration[BROKER_COUNT];
 
     @BeforeClass
     void setup() throws Exception {
@@ -104,7 +108,7 @@ void setup() throws Exception {
     }
 
     private void createTenant(PulsarAdmin pulsarAdmin)
-            throws PulsarClientException, MalformedURLException, PulsarAdminException {
+            throws PulsarAdminException {
         ClusterData clusterData = new ClusterData();
         clusterData.setServiceUrl(pulsarAdmin.getServiceUrl());
         pulsarAdmins[0].clusters().createCluster("my-cluster", clusterData);