From fb9bd637567096f6174bbc2e52a5e149a4eed882 Mon Sep 17 00:00:00 2001 From: Jim Hahn Date: Wed, 30 Sep 2020 12:52:25 -0400 Subject: 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 --- controlloop/common/controller-tdjam/pom.xml | 1 - .../policy/controlloop/tdjam/TdjamController.java | 28 ++++--- .../system/NonDroolsPolicyController.java | 9 +-- .../controlloop/tdjam/TdjamControllerTest.java | 3 +- .../system/NonDroolsPolicyControllerTest.java | 92 ++++++++++++++-------- 5 files changed, 78 insertions(+), 55 deletions(-) (limited to 'controlloop/common/controller-tdjam') 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 @@ policy-management ${version.policy.drools-pdp} provided - org.onap.policy.drools-applications.controlloop.common 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 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)); - } - /* ============================================================ */ /** -- cgit 1.2.3-korg