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 ++++++++++++++-------- .../ControlLoopEventManager2Drools.java | 2 + .../eventmanager/ControlLoopOperationManager2.java | 7 +- .../onap/policy/controlloop/eventmanager/Step.java | 7 +- .../controlloop/common/rules/test/BaseTest.java | 11 ++- .../common/rules/test/NamedRunner2Test.java | 58 ++++++++++++++ .../common/rules/test/NamedRunnerTest2.java | 58 -------------- 11 files changed, 158 insertions(+), 118 deletions(-) create mode 100644 controlloop/common/rules-test/src/test/java/org/onap/policy/controlloop/common/rules/test/NamedRunner2Test.java delete mode 100644 controlloop/common/rules-test/src/test/java/org/onap/policy/controlloop/common/rules/test/NamedRunnerTest2.java 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)); - } - /* ============================================================ */ /** 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 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/NamedRunner2Test.java b/controlloop/common/rules-test/src/test/java/org/onap/policy/controlloop/common/rules/test/NamedRunner2Test.java new file mode 100644 index 000000000..91e19ce79 --- /dev/null +++ b/controlloop/common/rules-test/src/test/java/org/onap/policy/controlloop/common/rules/test/NamedRunner2Test.java @@ -0,0 +1,58 @@ +/*- + * ============LICENSE_START======================================================= + * ONAP + * ================================================================================ + * Copyright (C) 2020 AT&T Intellectual Property. All rights reserved. + * ================================================================================ + * 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. + * ============LICENSE_END========================================================= + */ + +package org.onap.policy.controlloop.common.rules.test; + +import static org.junit.Assert.assertEquals; + +import org.junit.AfterClass; +import org.junit.Test; +import org.junit.runner.RunWith; + + +/** + * Tests NamedRunner when the TestNames annotation is missing - all tests should be + * executed. + */ +@RunWith(NamedRunner.class) +public class NamedRunner2Test { + + private static int testCount = 0; + + @AfterClass + public static void tearDownAfterClass() { + assertEquals(2, testCount); + } + + @Test + public void testAbc() { + checkTest(); + } + + @Test + public void testDef() { + checkTest(); + } + + + private static void checkTest() { + ++testCount; + } +} 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/NamedRunnerTest2.java deleted file mode 100644 index 1ed5b20bc..000000000 --- a/controlloop/common/rules-test/src/test/java/org/onap/policy/controlloop/common/rules/test/NamedRunnerTest2.java +++ /dev/null @@ -1,58 +0,0 @@ -/*- - * ============LICENSE_START======================================================= - * ONAP - * ================================================================================ - * Copyright (C) 2020 AT&T Intellectual Property. All rights reserved. - * ================================================================================ - * 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. - * ============LICENSE_END========================================================= - */ - -package org.onap.policy.controlloop.common.rules.test; - -import static org.junit.Assert.assertEquals; - -import org.junit.AfterClass; -import org.junit.Test; -import org.junit.runner.RunWith; - - -/** - * Tests NamedRunner when the TestNames annotation is missing - all tests should be - * executed. - */ -@RunWith(NamedRunner.class) -public class NamedRunnerTest2 { - - private static int testCount = 0; - - @AfterClass - public static void tearDownAfterClass() { - assertEquals(2, testCount); - } - - @Test - public void testAbc() { - checkTest(); - } - - @Test - public void testDef() { - checkTest(); - } - - - private static void checkTest() { - ++testCount; - } -} -- cgit 1.2.3-korg