[ACS-962] - Attempt to fix concurrency failing in test. (#1019)

ACS-962 Extract waitForMethodToFinish for TestHelper to avoid code duplication.

repository/src/test/java/org/alfresco/opencmis/OpenCmisLocalTest.java

@@ -25,6 +25,9 @@
  */
 package org.alfresco.opencmis;
 
+import static java.time.Duration.of;
+import static java.time.temporal.ChronoUnit.MILLIS;
+
 import java.io.ByteArrayInputStream;
 import java.io.File;
 import java.io.IOException;
@@ -61,7 +64,7 @@ import org.alfresco.util.ApplicationContextHelper;
 import org.alfresco.util.FileFilterMode.Client;
 import org.alfresco.util.GUID;
 import org.alfresco.util.TempFileProvider;
-import org.alfresco.util.testing.category.FrequentlyFailingTests;
+import org.alfresco.util.TestHelper;
 import org.alfresco.util.testing.category.LuceneTests;
 import org.aopalliance.intercept.MethodInterceptor;
 import org.aopalliance.intercept.MethodInvocation;
@@ -77,6 +80,7 @@ import org.apache.chemistry.opencmis.commons.data.ContentStream;
 import org.apache.chemistry.opencmis.commons.enums.BaseTypeId;
 import org.apache.chemistry.opencmis.commons.enums.BindingType;
 import org.apache.chemistry.opencmis.commons.enums.VersioningState;
+import org.apache.chemistry.opencmis.commons.exceptions.CmisRuntimeException;
 import org.apache.chemistry.opencmis.commons.exceptions.CmisStorageException;
 import org.apache.chemistry.opencmis.commons.impl.dataobjects.ContentStreamImpl;
 import org.apache.chemistry.opencmis.commons.impl.server.AbstractServiceFactory;
@@ -87,6 +91,7 @@ import org.apache.chemistry.opencmis.server.shared.TempStoreOutputStreamFactory;
 import org.junit.experimental.categories.Category;
 import org.springframework.aop.framework.ProxyFactory;
 import org.springframework.context.ApplicationContext;
+import org.springframework.dao.ConcurrencyFailureException;
 
 /**
  * Tests basic local CMIS interaction
@@ -467,7 +472,6 @@ public class OpenCmisLocalTest extends TestCase
      * This test would have fit better within CheckOutCheckInServiceImplTest but
      * was added here to make use of existing methods
      */
-    @Category(FrequentlyFailingTests.class) // ACS-962
     public void testCancelCheckoutWhileInCheckedOutState()
     {
         ServiceRegistry serviceRegistry = (ServiceRegistry) ctx.getBean(ServiceRegistry.SERVICE_REGISTRY);
@@ -484,7 +488,7 @@ public class OpenCmisLocalTest extends TestCase
 
         // Set file properties
         String docname = "myDoc-" + GUID.generate() + ".txt";
-        Map<String, String> props = new HashMap<String, String>();
+        Map<String, String> props = new HashMap<>();
         {
             props.put(PropertyIds.OBJECT_TYPE_ID, BaseTypeId.CMIS_DOCUMENT.value());
             props.put(PropertyIds.NAME, docname);
@@ -501,7 +505,9 @@ public class OpenCmisLocalTest extends TestCase
         NodeRef doc1WorkingCopy = cociService.getWorkingCopy(doc1NodeRef);
 
         /* Cancel Checkout */
-        cociService.cancelCheckout(doc1WorkingCopy);
+        TestHelper.waitForMethodToFinish(of(100, MILLIS), () ->
+                cociService.cancelCheckout(doc1WorkingCopy),
+                CmisRuntimeException.class, ConcurrencyFailureException.class);
 
         /* Check if both the working copy and the document were deleted */
         NodeService nodeService = serviceRegistry.getNodeService();

repository/src/test/java/org/alfresco/repo/lock/JobLockServiceTest.java

@@ -29,7 +29,6 @@ import static java.time.Duration.of;
 import static java.time.temporal.ChronoUnit.MILLIS;
 import static java.time.temporal.ChronoUnit.SECONDS;
 
-import java.time.Duration;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import junit.framework.AssertionFailedError;
@@ -44,6 +43,7 @@ import org.alfresco.service.namespace.QName;
 import org.alfresco.service.transaction.TransactionService;
 import org.alfresco.test_category.OwnJVMTestsCategory;
 import org.alfresco.util.ApplicationContextHelper;
+import org.alfresco.util.TestHelper;
 import org.alfresco.util.testing.category.DBTests;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -549,22 +549,22 @@ public class JobLockServiceTest extends TestCase
             
             if (callback == null) throw new IllegalStateException();
 
-            waitForAssertion(of(100, MILLIS), () -> {
+            TestHelper.waitForMethodToFinish(of(100, MILLIS), () -> {
                 assertEquals(false,callback.released);
                 assertEquals(0,callback.getIsActiveCount());
-            });
+            }, AssertionFailedError.class);
 
-            waitForAssertion(of(1, SECONDS), () -> {
+            TestHelper.waitForMethodToFinish(of(1, SECONDS), () -> {
                 assertEquals(false, callback.released);
                 assertEquals(1, callback.getIsActiveCount());
-            });
+            }, AssertionFailedError.class);
 
             callback.isActive = false;
 
-            waitForAssertion(of(2, SECONDS), () -> {
+            TestHelper.waitForMethodToFinish(of(2, SECONDS), () -> {
                 assertEquals(true, callback.released);
                 assertEquals(2, callback.getIsActiveCount());
-            });
+            }, AssertionFailedError.class);
         }
         catch (IllegalArgumentException e)
         {            
@@ -621,43 +621,6 @@ public class JobLockServiceTest extends TestCase
             Logger.getLogger("org.alfresco.repo.lock").setLevel(saveLogLevel);
         }
     }
-
-    private static void waitForAssertion(Duration timeout, Runnable assertion)
-    {
-        logger.debug("Waiting for assertion to succeed.");
-        final long lastStep = 10;
-        final long delayMillis = timeout.toMillis() > lastStep ? timeout.toMillis() / lastStep : 1;
-
-        for (int s = 0; s <= lastStep; s++)
-        {
-            try
-            {
-                assertion.run();
-                logger.debug("Assertion succeeded.");
-                return;
-            }
-            catch (AssertionFailedError e)
-            {
-                if (s == lastStep)
-                {
-                    logger.debug("Assertion failed. No more waiting.");
-                    throw e;
-                }
-                logger.debug("Assertion failed. Waiting until it succeeds.", e);
-            }
-            try
-            {
-                Thread.sleep(delayMillis);
-            }
-            catch (InterruptedException e)
-            {
-                Thread.currentThread().interrupt();
-                fail("Thread has been interrupted.");
-            }
-        }
-
-        throw new IllegalStateException("Unexpected.");
-    }
     
     private class TestCallback implements JobLockRefreshCallback
     {

repository/src/test/java/org/alfresco/util/TestHelper.java

@@ -29,8 +29,13 @@ package org.alfresco.util;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import java.time.Duration;
+import java.util.Arrays;
 import java.util.function.Supplier;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
 /**
  * A helper class to create a concise test.
  *
@@ -39,6 +44,8 @@ import java.util.function.Supplier;
  */
 public class TestHelper
 {
+    private static final Log logger = LogFactory.getLog(TestHelper.class);
+
     /**
      * Checks the thrown exception is the expected exception.
      *
@@ -94,4 +101,59 @@ public class TestHelper
 
         return t;
     }
+
+    /**
+     * Waits for <b>{@code method}</b> to succeed until <b>({@code timeout})</b>.
+     * <p>
+     * If the method failed to succeed because of previous step of test is not finished yet,
+     * it waits and re-executes the given method again.
+     * This will continue until the method do not fail or the <b>{@code timeout}</b> has been reached.
+     *
+     * @param timeout               max time of wait.
+     * @param method                the method that is called for retry.
+     * @param expectedExceptions    array of excepted exception.
+     * @throws Exception            after failing to finish given method with success.
+     */
+    @SafeVarargs
+    public static void waitForMethodToFinish(
+            Duration timeout,
+            Runnable method,
+            Class<? extends Throwable> ... expectedExceptions)
+    {
+        logger.debug("Waiting for method to succeed.");
+        final long lastStep = 10;
+        final long delayMillis = timeout.toMillis() > lastStep ? timeout.toMillis() / lastStep : 1;
+
+        for (int step = 0; step <= lastStep; step++)
+        {
+            try
+            {
+                method.run();
+                logger.debug("Method succeeded.");
+                return;
+            } catch (Throwable e)
+            {
+                if(Arrays.stream(expectedExceptions).noneMatch(expEx -> expEx.isInstance(e)))
+                {
+                    throw e;
+                }
+                if (step == lastStep)
+                {
+                    logger.debug("Method failed - no more waiting.");
+                    throw e;
+                }
+                logger.debug("Method failed. Waiting until it succeeds.", e);
+            }
+            try
+            {
+                Thread.sleep(delayMillis);
+            } catch (InterruptedException e)
+            {
+                Thread.currentThread().interrupt();
+                fail("Thread has been interrupted.");
+            }
+        }
+
+        throw new IllegalStateException("Unexpected.");
+    }
 }