Fix new sonars in drools-apps

Addressed the following sonars:
- too many assertions in test method
- rename test class
- use static method to modify static field
- use already defined constant
- code always returns the same value
- use assertNotSame
- use appropriate class name to access static method
- define a constant
- extract nested try block
- don't always return the same value
- use remove() instead of set(null) for thread-local-storage
- add @Override

Issue-ID: POLICY-2852
Change-Id: Icc62acd4ad57afa2d44ed4cdca504a3ac0810228
Signed-off-by: Jim Hahn <jrh3@att.com>

10 files changed, 101 insertions, 61 deletions

diff --git a/controlloop/common/controller-tdjam/pom.xml b/controlloop/common/controller-tdjam/pom.xml
index bee07cb2b..af882e3ed 100644
--- a/controlloop/common/controller-tdjam/pom.xml
+++ b/controlloop/common/controller-tdjam/pom.xml
@@ -168,7 +168,6 @@
             <artifactId>policy-management</artifactId>
             <version>${version.policy.drools-pdp}</version>
             <scope>provided</scope>
-            <!-- <optional>true</optional> -->
         </dependency>
         <dependency>
             <groupId>org.onap.policy.drools-applications.controlloop.common</groupId>
diff --git a/controlloop/common/controller-tdjam/src/main/java/org/onap/policy/controlloop/tdjam/TdjamController.java b/controlloop/common/controller-tdjam/src/main/java/org/onap/policy/controlloop/tdjam/TdjamController.java
index 0b17f196f..3b3655122 100644
--- a/controlloop/common/controller-tdjam/src/main/java/org/onap/policy/controlloop/tdjam/TdjamController.java
+++ b/controlloop/common/controller-tdjam/src/main/java/org/onap/policy/controlloop/tdjam/TdjamController.java
@@ -24,6 +24,7 @@ import static org.onap.policy.drools.properties.DroolsPropertyConstants.PROPERTY
 
 import java.io.ByteArrayOutputStream;
 import java.io.PrintStream;
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -72,6 +73,9 @@ public class TdjamController extends NonDroolsPolicyController {
     // the 'controller.type' property is set to this value
     private static final String TDJAM_CONTROLLER_BUILDER_TAG = "tdjam";
 
+    // topic on which to publish event notifications
+    private static final String POLICY_CL_MGT = "POLICY-CL-MGT";
+
     // additional data associated with session
     private final String groupId;
     private final String artifactId;
@@ -352,7 +356,9 @@ public class TdjamController extends NonDroolsPolicyController {
                        cp.getClosedLoopControlName());
         }
 
-        logger.debug(new String(bos.toByteArray()));
+        if (logger.isDebugEnabled()) {
+            logger.debug(new String(bos.toByteArray(), StandardCharsets.UTF_8));
+        }
     }
 
     /**
@@ -454,7 +460,7 @@ public class TdjamController extends NonDroolsPolicyController {
         // Generate notification
         //
         try {
-            PolicyEngineConstants.getManager().deliver("POLICY-CL-MGT", notification);
+            PolicyEngineConstants.getManager().deliver(POLICY_CL_MGT, notification);
 
         } catch (RuntimeException e) {
             logger.warn("{}: {}.{}: event={} exception generating notification",
@@ -609,18 +615,14 @@ public class TdjamController extends NonDroolsPolicyController {
                 // we create the ControlLoopEventManager. The ControlLoopEventManager
                 // will do extra syntax checking as well as check if the closed loop is disabled.
                 //
-                try {
-                    start();
-                } catch (Exception e) {
-                    eventManagers.remove(requestId, this);
-                    onsetToEventManager.remove(event, this);
-                    throw e;
-                }
+                start();
                 notification = makeNotification();
                 notification.setNotification(ControlLoopNotificationType.ACTIVE);
                 notification.setPolicyName(params.getPolicyName() + "." + ruleName);
             } catch (Exception e) {
                 logger.warn("{}: {}.{}", clName, params.getPolicyName(), ruleName, e);
+                eventManagers.remove(requestId, this);
+                onsetToEventManager.remove(event, this);
                 notification = new VirtualControlLoopNotification(event);
                 notification.setNotification(ControlLoopNotificationType.REJECTED);
                 notification.setMessage("Exception occurred: " + e.getMessage());
@@ -632,7 +634,7 @@ public class TdjamController extends NonDroolsPolicyController {
             // Generate notification
             //
             try {
-                PolicyEngineConstants.getManager().deliver("POLICY-CL-MGT", notification);
+                PolicyEngineConstants.getManager().deliver(POLICY_CL_MGT, notification);
 
             } catch (RuntimeException e) {
                 logger.warn("{}: {}.{}: event={} exception generating notification",
@@ -719,7 +721,7 @@ public class TdjamController extends NonDroolsPolicyController {
             //
             try {
                 notification.setPolicyName(getPolicyName() + "." + ruleName);
-                PolicyEngineConstants.getManager().deliver("POLICY-CL-MGT", notification);
+                PolicyEngineConstants.getManager().deliver(POLICY_CL_MGT, notification);
 
             } catch (RuntimeException e) {
                 logger.warn("{}: {}.{}: manager={} exception generating notification",
@@ -766,7 +768,7 @@ public class TdjamController extends NonDroolsPolicyController {
             //
             try {
                 notification.setPolicyName(getPolicyName() + "." + ruleName);
-                PolicyEngineConstants.getManager().deliver("POLICY-CL-MGT", notification);
+                PolicyEngineConstants.getManager().deliver(POLICY_CL_MGT, notification);
             } catch (RuntimeException e) {
                 logger.warn("{}: {}.{}: manager={} exception generating notification",
                             getClosedLoopControlName(), getPolicyName(), ruleName,
@@ -825,7 +827,7 @@ public class TdjamController extends NonDroolsPolicyController {
                                       List<TopicCoderFilterConfiguration> encoderConfigurations) throws LinkageError {
 
             if (TDJAM_CONTROLLER_BUILDER_TAG.equals(properties.getProperty(PROPERTY_CONTROLLER_TYPE))) {
-                return TdjamController.getBuildInProgress();
+                return NonDroolsPolicyController.getBuildInProgress();
             }
             return null;
         }
diff --git a/controlloop/common/controller-tdjam/src/main/java/org/onap/policy/extension/system/NonDroolsPolicyController.java b/controlloop/common/controller-tdjam/src/main/java/org/onap/policy/extension/system/NonDroolsPolicyController.java
index d876bee96..97eb6a04d 100644
--- a/controlloop/common/controller-tdjam/src/main/java/org/onap/policy/extension/system/NonDroolsPolicyController.java
+++ b/controlloop/common/controller-tdjam/src/main/java/org/onap/policy/extension/system/NonDroolsPolicyController.java
@@ -143,12 +143,13 @@ public class NonDroolsPolicyController extends AggregatedPolicyController implem
         return buildInProgress.get();
     }
 
+    @Override
     protected void initDrools(Properties properties) {
         try {
             // Register with drools factory
             buildInProgress.set(this);
             this.droolsController.set(getDroolsFactory().build(properties, sources, sinks));
-            buildInProgress.set(null);
+            buildInProgress.remove();
         } catch (Exception | LinkageError e) {
             logger.error("{}: cannot init-drools", this);
             throw new IllegalArgumentException(e);
@@ -198,9 +199,6 @@ public class NonDroolsPolicyController extends AggregatedPolicyController implem
         logger.info("START: {}", this);
 
         synchronized (this) {
-            if (this.alive) {
-                return true;
-            }
             this.alive = true;
         }
 
@@ -213,9 +211,6 @@ public class NonDroolsPolicyController extends AggregatedPolicyController implem
         logger.info("STOP: {}", this);
 
         synchronized (this) {
-            if (!this.alive) {
-                return true;
-            }
             this.alive = false;
         }
 
diff --git a/controlloop/common/controller-tdjam/src/test/java/org/onap/policy/controlloop/tdjam/TdjamControllerTest.java b/controlloop/common/controller-tdjam/src/test/java/org/onap/policy/controlloop/tdjam/TdjamControllerTest.java
index 1426865c6..990e473db 100644
--- a/controlloop/common/controller-tdjam/src/test/java/org/onap/policy/controlloop/tdjam/TdjamControllerTest.java
+++ b/controlloop/common/controller-tdjam/src/test/java/org/onap/policy/controlloop/tdjam/TdjamControllerTest.java
@@ -22,6 +22,7 @@ package org.onap.policy.controlloop.tdjam;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
@@ -211,7 +212,7 @@ public class TdjamControllerTest {
         clp.setClosedLoopControlName(closedLoopControlName);
 
         if (tc != null) {
-            assertTrue(tc.addControlLoopParams(clp) != clp);
+            assertNotSame(clp, tc.addControlLoopParams(clp));
         }
 
         return clp;
diff --git a/controlloop/common/controller-tdjam/src/test/java/org/onap/policy/extension/system/NonDroolsPolicyControllerTest.java b/controlloop/common/controller-tdjam/src/test/java/org/onap/policy/extension/system/NonDroolsPolicyControllerTest.java
index ee96cb893..57f98bd0a 100644
--- a/controlloop/common/controller-tdjam/src/test/java/org/onap/policy/extension/system/NonDroolsPolicyControllerTest.java
+++ b/controlloop/common/controller-tdjam/src/test/java/org/onap/policy/extension/system/NonDroolsPolicyControllerTest.java
@@ -36,6 +36,8 @@ import static org.onap.policy.drools.properties.DroolsPropertyConstants.PROPERTY
 import java.util.List;
 import java.util.Properties;
 import java.util.function.Function;
+import org.junit.After;
+import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.onap.policy.common.endpoints.event.comm.TopicSink;
@@ -52,15 +54,37 @@ public class NonDroolsPolicyControllerTest {
     //public static boolean loop = true;
     private static Properties prop;
 
+    private NonDroolsPolicyController controller;
+
     @BeforeClass
     public static void setupClass() throws Exception {
         prop = PropertyUtil.getProperties("src/test/resources/config/tdjam-controller.properties");
     }
 
+    /**
+     * Resets the stats and creates the controller.
+     */
+    @Before
+    public void setUp() {
+        DroolsControllerFeatureHandler.resetStats();
+
+        controller = buildController("tdjam");
+    }
+
+    /**
+     * Destroys the controller.
+     */
+    @After
+    public void tearDown() {
+        String name = controller.getName();
+        assertSame(controller, PolicyControllerConstants.getFactory().get(name));
+        PolicyControllerConstants.getFactory().destroy(controller);
+        assertThatIllegalArgumentException().isThrownBy(
+            () -> PolicyControllerConstants.getFactory().get(name));
+    }
+
     @Test
     public void testState() {
-        NonDroolsPolicyController controller = buildController("tdjam");
-
         assertEquals("nondrools", controller.getName());
         assertEquals("NonDroolsPolicyController", controller.getGroupId());
         assertEquals("nondrools", controller.getArtifactId());
@@ -81,48 +105,60 @@ public class NonDroolsPolicyControllerTest {
         assertFalse(controller.isLocked());
 
         // test a few stubbed-off methods
+        assertNull(controller.getContainer());
+        assertThatIllegalStateException().isThrownBy(() -> controller.updateToVersion(null, null, null, null, null));
+
+        controller.lock();
+        assertTrue(controller.isAlive());
+        assertTrue(controller.isLocked());
+
+        controller.stop();
+        assertFalse(controller.isAlive());
+        assertTrue(controller.isLocked());
+
+        controller.unlock();
+        assertFalse(controller.isAlive());
+        assertFalse(controller.isLocked());
+    }
+
+    @Test
+    public void testNames() {
         assertTrue(controller.getSessionNames().isEmpty());
         assertTrue(controller.getCanonicalSessionNames().isEmpty());
         assertTrue(controller.getBaseDomainNames().isEmpty());
+    }
+
+    @Test
+    public void testOffer() {
+        controller.start();
+
         assertFalse(controller.offer("topic", "event"));
         assertFalse(controller.offer("event"));
         assertEquals(0, controller.getRecentSourceEvents().length);
         assertEquals(0, controller.getRecentSinkEvents().length);
-        assertNull(controller.getContainer());
+    }
+
+    @Test
+    public void testFacts() {
         assertThatIllegalArgumentException().isThrownBy(
             () -> controller.fetchModelClass("NoSuchClass"));
-        assertThatIllegalStateException().isThrownBy(
-            () -> controller.updateToVersion(null, null, null, null, null));
         assertTrue(controller.factClassNames(null).isEmpty());
         assertEquals(0, controller.factCount(null));
         assertTrue(controller.facts(null, null, false).isEmpty());
         assertTrue(controller.facts("sessionName", String.class).isEmpty());
         assertTrue(controller.factQuery(null, null, null, false).isEmpty());
+    }
+
+    @Test
+    public void testDelete() {
         assertFalse(controller.delete("sessionName", "fact"));
         assertFalse(controller.delete("fact"));
         assertFalse(controller.delete("sessionName", String.class));
         assertFalse(controller.delete(String.class));
-
-        controller.lock();
-        assertTrue(controller.isAlive());
-        assertTrue(controller.isLocked());
-
-        controller.stop();
-        assertFalse(controller.isAlive());
-        assertTrue(controller.isLocked());
-
-        controller.unlock();
-        assertFalse(controller.isAlive());
-        assertFalse(controller.isLocked());
-
-        destroy(controller);
     }
 
     @Test
-    public void deliverTest() {
-        DroolsControllerFeatureHandler.resetStats();
-        final NonDroolsPolicyController controller = buildController("tdjam");
-
+    public void testDeliver() {
         final TopicSink topicSink = mock(TopicSink.class);
         when(topicSink.getTopic()).thenReturn("POLICY-CL-MGT");
         when(topicSink.send(any())).thenReturn(false);
@@ -201,8 +237,6 @@ public class NonDroolsPolicyControllerTest {
 
         assertFalse(signal.apply("nothing in particular"));
         assertEquals(1, DroolsControllerFeatureHandler.afterDeliverFalse);
-
-        destroy(controller);
     }
 
     private NonDroolsPolicyController buildController(String type) {
@@ -213,14 +247,6 @@ public class NonDroolsPolicyControllerTest {
         return (NonDroolsPolicyController) controller;
     }
 
-    private void destroy(PolicyController controller) {
-        String name = controller.getName();
-        assertSame(controller, PolicyControllerConstants.getFactory().get(name));
-        PolicyControllerConstants.getFactory().destroy(controller);
-        assertThatIllegalArgumentException().isThrownBy(
-            () -> PolicyControllerConstants.getFactory().get(name));
-    }
-
     /* ============================================================ */
 
     /**
diff --git a/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManager2Drools.java b/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManager2Drools.java
index 3af9defc7..9e8af9afb 100644
--- a/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManager2Drools.java
+++ b/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManager2Drools.java
@@ -33,6 +33,8 @@ import org.onap.policy.controlloop.drl.legacy.ControlLoopParams;
  * {@link #isActive()} returns {@code false}, indicating that all steps have completed.
  */
 public class ControlLoopEventManager2Drools extends ControlLoopEventManager2 {
+    private static final long serialVersionUID = 1L;
+
     private final transient WorkingMemory workMem;
     private transient FactHandle factHandle;
 
diff --git a/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopOperationManager2.java b/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopOperationManager2.java
index 31402f4cf..62d4fc8c0 100644
--- a/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopOperationManager2.java
+++ b/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopOperationManager2.java
@@ -300,7 +300,12 @@ public class ControlLoopOperationManager2 implements Serializable {
      * @param thrown exception that was generated
      * @return {@code null}
      */
-    private OperationOutcome handleException(Throwable thrown) {
+    private OperationOutcome handleException(Throwable thrown) { // NOSONAR
+        /*
+         * disabling sonar about returning the same value because we prefer the code to be
+         * structured this way
+         */
+
         if (thrown instanceof CancellationException || thrown.getCause() instanceof CancellationException) {
             return null;
         }
diff --git a/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/Step.java b/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/Step.java
index ae51c737f..1cbdb53b2 100644
--- a/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/Step.java
+++ b/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/Step.java
@@ -178,7 +178,12 @@ public class Step {
      * @param thrown exception that was generated
      * @return {@code null}
      */
-    private OperationOutcome handleException(Throwable thrown) {
+    private OperationOutcome handleException(Throwable thrown) { // NOSONAR
+        /*
+         * disabling sonar about returning the same value because we prefer the code to be
+         * structured this way
+         */
+
         if (thrown instanceof CancellationException || thrown.getCause() instanceof CancellationException) {
             return null;
         }
diff --git a/controlloop/common/rules-test/src/main/java/org/onap/policy/controlloop/common/rules/test/BaseTest.java b/controlloop/common/rules-test/src/main/java/org/onap/policy/controlloop/common/rules/test/BaseTest.java
index f5346def8..83825bca9 100644
--- a/controlloop/common/rules-test/src/main/java/org/onap/policy/controlloop/common/rules/test/BaseTest.java
+++ b/controlloop/common/rules-test/src/main/java/org/onap/policy/controlloop/common/rules/test/BaseTest.java
@@ -30,6 +30,7 @@ import java.util.function.Supplier;
 import java.util.stream.Collectors;
 import lombok.AccessLevel;
 import lombok.Getter;
+import lombok.Setter;
 import org.awaitility.Awaitility;
 import org.junit.Test;
 import org.onap.policy.appc.Request;
@@ -55,6 +56,9 @@ public abstract class BaseTest {
     private static final String APPC_RESTART_OP = "restart";
     private static final String APPC_MODIFY_CONFIG_OP = "ModifyConfig";
 
+    private static final String SDNR_MODIFY_CONFIG_OP = "ModifyConfig";
+    private static final String SNDR_MODIFY_CONFIG_ANR_OP = "ModifyConfigANR";
+
     /*
      * Canonical Topic Names.
      */
@@ -135,6 +139,7 @@ public abstract class BaseTest {
 
     // used to inject and wait for messages
     @Getter(AccessLevel.PROTECTED)
+    @Setter(AccessLevel.PROTECTED)
     protected static Topics topics;
 
     // used to wait for messages on SINK topics
@@ -168,7 +173,7 @@ public abstract class BaseTest {
      * Initializes {@link #topics} and {@link #controller}.
      */
     public void init() {
-        topics = topicMaker.get();
+        setTopics(topicMaker.get());
 
         Map<String, SimpleLock> locks = getLockMap();
         if (locks != null) {
@@ -348,7 +353,7 @@ public abstract class BaseTest {
      */
     @Test
     public void testVpciSunnyDayCompliant() {
-        sdnrSunnyDay(VPCI_TOSCA_COMPLIANT_POLICY, VPCI_ONSET, VPCI_SDNR_SUCCESS, "ModifyConfig");
+        sdnrSunnyDay(VPCI_TOSCA_COMPLIANT_POLICY, VPCI_ONSET, VPCI_SDNR_SUCCESS, SDNR_MODIFY_CONFIG_OP);
     }
 
     // VSONH
@@ -358,7 +363,7 @@ public abstract class BaseTest {
      */
     @Test
     public void testVsonhSunnyDayCompliant() {
-        sdnrSunnyDay(VSONH_TOSCA_COMPLIANT_POLICY, VSONH_ONSET, VSONH_SDNR_SUCCESS, "ModifyConfigANR");
+        sdnrSunnyDay(VSONH_TOSCA_COMPLIANT_POLICY, VSONH_ONSET, VSONH_SDNR_SUCCESS, SNDR_MODIFY_CONFIG_ANR_OP);
     }
 
     /**
diff --git a/controlloop/common/rules-test/src/test/java/org/onap/policy/controlloop/common/rules/test/NamedRunnerTest2.java b/controlloop/common/rules-test/src/test/java/org/onap/policy/controlloop/common/rules/test/NamedRunner2Test.java
index 1ed5b20bc..91e19ce79 100644
--- a/controlloop/common/rules-test/src/test/java/org/onap/policy/controlloop/common/rules/test/NamedRunnerTest2.java
+++ b/controlloop/common/rules-test/src/test/java/org/onap/policy/controlloop/common/rules/test/NamedRunner2Test.java
@@ -32,7 +32,7 @@ import org.junit.runner.RunWith;
  * executed.
  */
 @RunWith(NamedRunner.class)
-public class NamedRunnerTest2 {
+public class NamedRunner2Test {
 
     private static int testCount = 0;