From 3fd9dc0e5c584702d25982172bb5ee44b6b57aa3 Mon Sep 17 00:00:00 2001 From: Jim Hahn Date: Thu, 12 Apr 2018 14:23:25 -0400 Subject: Sonar fixes for policy-core locks Made a number of changes to the locking code in policy-core, to address sonar issues. This entaileed changing some of the Lock API methods to return OperResult instead of Boolean. Updated distributed locking with the new API return types. Simplified Thread creation using functional methods. Change-Id: If32bf7a435d2aedb969de1b77c7e7e27e110ecb0 Issue-ID: POLICY-728 Signed-off-by: Jim Hahn --- .../drools/core/lock/LockRequestFutureTest.java | 74 ++++++---------------- .../lock/PolicyResourceLockFeatureAPITest.java | 8 ++- .../core/lock/PolicyResourceLockManagerTest.java | 37 +++++------ .../drools/core/lock/SimpleLockManagerTest.java | 51 +++++++-------- .../onap/policy/drools/core/lock/TestUtils.java | 8 +-- 5 files changed, 69 insertions(+), 109 deletions(-) (limited to 'policy-core/src/test/java') diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java index 883778eb..2dd90f00 100644 --- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java +++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java @@ -80,13 +80,13 @@ public class LockRequestFutureTest { public void testLockRequestFutureStringStringBoolean_ArgEx() throws Exception { // null resource id - IllegalArgumentException ex = expectException(IllegalArgumentException.class, - xxx -> new LockRequestFuture(null, OWNER, true)); + IllegalArgumentException ex = + expectException(IllegalArgumentException.class, () -> new LockRequestFuture(null, OWNER, true)); assertEquals("null resourceId", ex.getMessage()); // null owner - ex = expectException(IllegalArgumentException.class, xxx -> new LockRequestFuture(RESOURCE, null, true)); + ex = expectException(IllegalArgumentException.class, () -> new LockRequestFuture(RESOURCE, null, true)); assertEquals("null owner", ex.getMessage()); } @@ -108,12 +108,12 @@ public class LockRequestFutureTest { // null resource id IllegalArgumentException ex = expectException(IllegalArgumentException.class, - xxx -> new LockRequestFuture(null, OWNER, callback)); + () -> new LockRequestFuture(null, OWNER, callback)); assertEquals("null resourceId", ex.getMessage()); // null owner - ex = expectException(IllegalArgumentException.class, xxx -> new LockRequestFuture(RESOURCE, null, callback)); + ex = expectException(IllegalArgumentException.class, () -> new LockRequestFuture(RESOURCE, null, callback)); assertEquals("null owner", ex.getMessage()); @@ -141,7 +141,7 @@ public class LockRequestFutureTest { assertTrue(fut.isDone()); // should not block now - expectException(CancellationException.class, xxx -> fut.get(0, TimeUnit.SECONDS)); + expectException(CancellationException.class, () -> fut.get(0, TimeUnit.SECONDS)); } @@ -153,7 +153,7 @@ public class LockRequestFutureTest { assertFalse(fut.cancel(true)); assertTrue(fut.isDone()); - expectException(CancellationException.class, xxx -> fut.get(0, TimeUnit.SECONDS)); + expectException(CancellationException.class, () -> fut.get(0, TimeUnit.SECONDS)); } @Test @@ -201,7 +201,7 @@ public class LockRequestFutureTest { assertFalse(fut.setLocked(false)); assertTrue(fut.isDone()); - expectException(CancellationException.class, xxx -> fut.get(0, TimeUnit.SECONDS)); + expectException(CancellationException.class, () -> fut.get(0, TimeUnit.SECONDS)); } @Test @@ -296,43 +296,25 @@ public class LockRequestFutureTest { @Test public void testGet_Cancelled() throws Exception { - new Thread() { - @Override - public void run() { - fut.cancel(false); - } - }.start(); - - expectException(CancellationException.class, xxx -> fut.get()); + new Thread(() -> fut.cancel(false)).start(); + expectException(CancellationException.class, () -> fut.get()); } @Test public void testGet_Acquired() throws Exception { - new Thread() { - @Override - public void run() { - fut.setLocked(true); - } - }.start(); - + new Thread(() -> fut.setLocked(true)).start(); assertTrue(fut.get()); } @Test public void testGet_Denied() throws Exception { - new Thread() { - @Override - public void run() { - fut.setLocked(false); - } - }.start(); - + new Thread(() -> fut.setLocked(false)).start(); assertFalse(fut.get()); } @Test public void testGetLongTimeUnit() throws Exception { - expectException(TimeoutException.class, xxx -> fut.get(0, TimeUnit.SECONDS)); + expectException(TimeoutException.class, () -> fut.get(0, TimeUnit.SECONDS)); fut.setLocked(true); assertTrue(fut.get(0, TimeUnit.SECONDS)); @@ -340,43 +322,25 @@ public class LockRequestFutureTest { @Test public void testGetLongTimeUnit_Timeout() throws Exception { - expectException(TimeoutException.class, xxx -> fut.get(0, TimeUnit.SECONDS)); - expectException(TimeoutException.class, xxx -> fut.get(2, TimeUnit.MILLISECONDS)); + expectException(TimeoutException.class, () -> fut.get(0, TimeUnit.SECONDS)); + expectException(TimeoutException.class, () -> fut.get(2, TimeUnit.MILLISECONDS)); } @Test public void testGetLongTimeUnit_Cancelled() throws Exception { - new Thread() { - @Override - public void run() { - fut.cancel(false); - } - }.start(); - - expectException(CancellationException.class, xxx -> fut.get(WAIT_SEC, TimeUnit.SECONDS)); + new Thread(() -> fut.cancel(false)).start(); + expectException(CancellationException.class, () -> fut.get(WAIT_SEC, TimeUnit.SECONDS)); } @Test public void testGetLongTimeUnit_Acquired() throws Exception { - new Thread() { - @Override - public void run() { - fut.setLocked(true); - } - }.start(); - + new Thread(() -> fut.setLocked(true)).start(); assertTrue(fut.get(WAIT_SEC, TimeUnit.SECONDS)); } @Test public void testGetLongTimeUnit_Denied() throws Exception { - new Thread() { - @Override - public void run() { - fut.setLocked(false); - } - }.start(); - + new Thread(() -> fut.setLocked(false)).start(); assertFalse(fut.get(WAIT_SEC, TimeUnit.SECONDS)); } diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPITest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPITest.java index 5a826bb7..57adc847 100644 --- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPITest.java +++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPITest.java @@ -20,10 +20,12 @@ package org.onap.policy.drools.core.lock; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import org.junit.Before; import org.junit.Test; +import org.onap.policy.drools.core.lock.PolicyResourceLockFeatureAPI.OperResult; public class PolicyResourceLockFeatureAPITest { @@ -55,7 +57,7 @@ public class PolicyResourceLockFeatureAPITest { @Test public void testBeforeUnlock() { - assertNull(api.beforeUnlock(RESOURCE_ID, OWNER)); + assertEquals(OperResult.OPER_UNHANDLED, api.beforeUnlock(RESOURCE_ID, OWNER)); } @Test @@ -66,11 +68,11 @@ public class PolicyResourceLockFeatureAPITest { @Test public void testBeforeIsLocked() { - assertNull(api.beforeIsLocked(RESOURCE_ID)); + assertEquals(OperResult.OPER_UNHANDLED, api.beforeIsLocked(RESOURCE_ID)); } @Test public void testBeforeIsLockedBy() { - assertNull(api.beforeIsLockedBy(RESOURCE_ID, OWNER)); + assertEquals(OperResult.OPER_UNHANDLED, api.beforeIsLockedBy(RESOURCE_ID, OWNER)); } } diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java index 2f8b5f83..fe883acf 100644 --- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java +++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java @@ -42,6 +42,7 @@ import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; import org.onap.policy.drools.core.lock.PolicyResourceLockFeatureAPI.Callback; +import org.onap.policy.drools.core.lock.PolicyResourceLockFeatureAPI.OperResult; import org.onap.policy.drools.core.lock.PolicyResourceLockManager.Factory; public class PolicyResourceLockManagerTest { @@ -111,9 +112,9 @@ public class PolicyResourceLockManagerTest { */ private void initImplementer(PolicyResourceLockFeatureAPI impl) { when(impl.beforeLock(anyString(), anyString(), any(Callback.class))).thenReturn(null); - when(impl.beforeUnlock(anyString(), anyString())).thenReturn(null); - when(impl.beforeIsLocked(anyString())).thenReturn(null); - when(impl.beforeIsLockedBy(anyString(), anyString())).thenReturn(null); + when(impl.beforeUnlock(anyString(), anyString())).thenReturn(OperResult.OPER_UNHANDLED); + when(impl.beforeIsLocked(anyString())).thenReturn(OperResult.OPER_UNHANDLED); + when(impl.beforeIsLockedBy(anyString(), anyString())).thenReturn(OperResult.OPER_UNHANDLED); } @Test @@ -147,10 +148,10 @@ public class PolicyResourceLockManagerTest { @Test public void testLock_ArgEx() { IllegalArgumentException ex = - expectException(IllegalArgumentException.class, xxx -> mgr.lock(null, OWNER1, callback1)); + expectException(IllegalArgumentException.class, () -> mgr.lock(null, OWNER1, callback1)); assertEquals(NULL_RESOURCE_ID, ex.getMessage()); - ex = expectException(IllegalArgumentException.class, xxx -> mgr.lock(RESOURCE_A, null, callback1)); + ex = expectException(IllegalArgumentException.class, () -> mgr.lock(RESOURCE_A, null, callback1)); assertEquals(NULL_OWNER, ex.getMessage()); // this should not throw an exception @@ -250,10 +251,10 @@ public class PolicyResourceLockManagerTest { @Test public void testUnlock_ArgEx() { - IllegalArgumentException ex = expectException(IllegalArgumentException.class, xxx -> mgr.unlock(null, OWNER1)); + IllegalArgumentException ex = expectException(IllegalArgumentException.class, () -> mgr.unlock(null, OWNER1)); assertEquals(NULL_RESOURCE_ID, ex.getMessage()); - ex = expectException(IllegalArgumentException.class, xxx -> mgr.unlock(RESOURCE_A, null)); + ex = expectException(IllegalArgumentException.class, () -> mgr.unlock(RESOURCE_A, null)); assertEquals(NULL_OWNER, ex.getMessage()); } @@ -263,7 +264,7 @@ public class PolicyResourceLockManagerTest { mgr.lock(RESOURCE_A, OWNER1, null); // have impl1 intercept - when(impl1.beforeUnlock(RESOURCE_A, OWNER1)).thenReturn(true); + when(impl1.beforeUnlock(RESOURCE_A, OWNER1)).thenReturn(OperResult.OPER_ACCEPTED); assertTrue(mgr.unlock(RESOURCE_A, OWNER1)); @@ -280,7 +281,7 @@ public class PolicyResourceLockManagerTest { mgr.lock(RESOURCE_A, OWNER1, null); // have impl1 intercept - when(impl1.beforeUnlock(RESOURCE_A, OWNER1)).thenReturn(false); + when(impl1.beforeUnlock(RESOURCE_A, OWNER1)).thenReturn(OperResult.OPER_DENIED); assertFalse(mgr.unlock(RESOURCE_A, OWNER1)); @@ -365,7 +366,7 @@ public class PolicyResourceLockManagerTest { @Test public void testIsLocked_ArgEx() { - IllegalArgumentException ex = expectException(IllegalArgumentException.class, xxx -> mgr.isLocked(null)); + IllegalArgumentException ex = expectException(IllegalArgumentException.class, () -> mgr.isLocked(null)); assertEquals(NULL_RESOURCE_ID, ex.getMessage()); } @@ -373,7 +374,7 @@ public class PolicyResourceLockManagerTest { public void testIsLocked_BeforeIntercepted_True() { // have impl1 intercept - when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(true); + when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(OperResult.OPER_ACCEPTED);; assertTrue(mgr.isLocked(RESOURCE_A)); @@ -388,7 +389,7 @@ public class PolicyResourceLockManagerTest { mgr.lock(RESOURCE_A, OWNER1, null); // have impl1 intercept - when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(false); + when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(OperResult.OPER_DENIED); assertFalse(mgr.isLocked(RESOURCE_A)); @@ -420,10 +421,10 @@ public class PolicyResourceLockManagerTest { @Test public void testIsLockedBy_ArgEx() { IllegalArgumentException ex = - expectException(IllegalArgumentException.class, xxx -> mgr.isLockedBy(null, OWNER1)); + expectException(IllegalArgumentException.class, () -> mgr.isLockedBy(null, OWNER1)); assertEquals(NULL_RESOURCE_ID, ex.getMessage()); - ex = expectException(IllegalArgumentException.class, xxx -> mgr.isLockedBy(RESOURCE_A, null)); + ex = expectException(IllegalArgumentException.class, () -> mgr.isLockedBy(RESOURCE_A, null)); assertEquals(NULL_OWNER, ex.getMessage()); } @@ -431,7 +432,7 @@ public class PolicyResourceLockManagerTest { public void testIsLockedBy_BeforeIntercepted_True() { // have impl1 intercept - when(impl1.beforeIsLockedBy(RESOURCE_A, OWNER1)).thenReturn(true); + when(impl1.beforeIsLockedBy(RESOURCE_A, OWNER1)).thenReturn(OperResult.OPER_ACCEPTED);; assertTrue(mgr.isLockedBy(RESOURCE_A, OWNER1)); @@ -446,7 +447,7 @@ public class PolicyResourceLockManagerTest { mgr.lock(RESOURCE_A, OWNER1, null); // have impl1 intercept - when(impl1.beforeIsLockedBy(RESOURCE_A, OWNER1)).thenReturn(false); + when(impl1.beforeIsLockedBy(RESOURCE_A, OWNER1)).thenReturn(OperResult.OPER_DENIED); assertFalse(mgr.isLockedBy(RESOURCE_A, OWNER1)); @@ -479,7 +480,7 @@ public class PolicyResourceLockManagerTest { @Test public void testDoIntercept_Impl1() { - when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(true); + when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(OperResult.OPER_ACCEPTED);; assertTrue(mgr.isLocked(RESOURCE_A)); @@ -489,7 +490,7 @@ public class PolicyResourceLockManagerTest { @Test public void testDoIntercept_Impl2() { - when(impl2.beforeIsLocked(RESOURCE_A)).thenReturn(true); + when(impl2.beforeIsLocked(RESOURCE_A)).thenReturn(OperResult.OPER_ACCEPTED);; assertTrue(mgr.isLocked(RESOURCE_A)); diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java index 6abc5bf9..833e1c7d 100644 --- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java +++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java @@ -98,10 +98,10 @@ public class SimpleLockManagerTest { @Test public void testLock_ArgEx() { IllegalArgumentException ex = - expectException(IllegalArgumentException.class, xxx -> mgr.lock(null, OWNER1, null)); + expectException(IllegalArgumentException.class, () -> mgr.lock(null, OWNER1, null)); assertEquals(NULL_RESOURCE_ID, ex.getMessage()); - ex = expectException(IllegalArgumentException.class, xxx -> mgr.lock(RESOURCE_A, null, null)); + ex = expectException(IllegalArgumentException.class, () -> mgr.lock(RESOURCE_A, null, null)); assertEquals(NULL_OWNER, ex.getMessage()); // this should not throw an exception @@ -118,10 +118,10 @@ public class SimpleLockManagerTest { @Test public void testUnlock_ArgEx() { - IllegalArgumentException ex = expectException(IllegalArgumentException.class, xxx -> mgr.unlock(null, OWNER1)); + IllegalArgumentException ex = expectException(IllegalArgumentException.class, () -> mgr.unlock(null, OWNER1)); assertEquals(NULL_RESOURCE_ID, ex.getMessage()); - ex = expectException(IllegalArgumentException.class, xxx -> mgr.unlock(RESOURCE_A, null)); + ex = expectException(IllegalArgumentException.class, () -> mgr.unlock(RESOURCE_A, null)); assertEquals(NULL_OWNER, ex.getMessage()); } @@ -156,7 +156,7 @@ public class SimpleLockManagerTest { @Test public void testIsLocked_ArgEx() { - IllegalArgumentException ex = expectException(IllegalArgumentException.class, xxx -> mgr.isLocked(null)); + IllegalArgumentException ex = expectException(IllegalArgumentException.class, () -> mgr.isLocked(null)); assertEquals(NULL_RESOURCE_ID, ex.getMessage()); } @@ -181,10 +181,10 @@ public class SimpleLockManagerTest { @Test public void testIsLockedBy_ArgEx() { IllegalArgumentException ex = - expectException(IllegalArgumentException.class, xxx -> mgr.isLockedBy(null, OWNER1)); + expectException(IllegalArgumentException.class, () -> mgr.isLockedBy(null, OWNER1)); assertEquals(NULL_RESOURCE_ID, ex.getMessage()); - ex = expectException(IllegalArgumentException.class, xxx -> mgr.isLockedBy(RESOURCE_A, null)); + ex = expectException(IllegalArgumentException.class, () -> mgr.isLockedBy(RESOURCE_A, null)); assertEquals(NULL_OWNER, ex.getMessage()); } @@ -221,34 +221,31 @@ public class SimpleLockManagerTest { for (int x = 0; x < nthreads; ++x) { String owner = "owner." + x; - Thread t = new Thread() { - @Override - public void run() { + Thread t = new Thread(() -> { - for (int y = 0; y < nlocks; ++y) { - String res = resources[y % resources.length]; + for (int y = 0; y < nlocks; ++y) { + String res = resources[y % resources.length]; - try { - // some locks will be acquired, some denied - mgr.lock(res, owner, null).get(); + try { + // some locks will be acquired, some denied + mgr.lock(res, owner, null).get(); - // do some "work" - stopper.await(1L, TimeUnit.MILLISECONDS); + // do some "work" + stopper.await(1L, TimeUnit.MILLISECONDS); - mgr.unlock(res, owner); + mgr.unlock(res, owner); - } catch (CancellationException | ExecutionException e) { - nfail.incrementAndGet(); + } catch (CancellationException | ExecutionException e) { + nfail.incrementAndGet(); - } catch (InterruptedException expected) { - Thread.currentThread().interrupt(); - break; - } + } catch (InterruptedException expected) { + Thread.currentThread().interrupt(); + break; } - - completed.countDown(); } - }; + + completed.countDown(); + }); t.setDaemon(true); threads.add(t); diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/TestUtils.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/TestUtils.java index 2e353936..f843f6ab 100644 --- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/TestUtils.java +++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/TestUtils.java @@ -31,7 +31,7 @@ public class TestUtils { */ public static T expectException(Class clazz, VoidFunction func) { try { - func.apply(null); + func.apply(); throw new AssertionError("missing exception"); } catch (Exception e) { @@ -50,10 +50,6 @@ public class TestUtils { @FunctionalInterface public static interface VoidFunction { - /** - * - * @param arg always {@code null} - */ - public void apply(Void arg) throws Exception; + public void apply() throws Exception; } } -- cgit 1.2.3-korg