From f113724f9a3e17e67a9712403b6bc51e113a45ee Mon Sep 17 00:00:00 2001 From: Domenico Sibilio Date: Tue, 1 Feb 2022 09:14:53 +0100 Subject: [PATCH] ACS-2245 Mitigate intermittent failure of FixedAclUpdaterTest (#916) * ACS-2245 - Trigger CI build * ACS-2245 - Add RepeatAtMostRule for flaky tests * ACS-2245 - Add RepeatAtMostRuleTest to the AllUnitTestsSuite * ACS-2245 - Rename RepeatAtMostRule to RetryAtMostRule --- .../java/org/alfresco/AllUnitTestsSuite.java | 1 + .../permissions/FixedAclUpdaterTest.java | 57 ++++++-- .../repo/security/SecurityTestSuite.java | 2 +- .../util/test/junitrules/RetryAtMostRule.java | 132 +++++++++++++++++ .../test/junitrules/RetryAtMostRuleTest.java | 138 ++++++++++++++++++ 5 files changed, 316 insertions(+), 14 deletions(-) create mode 100644 repository/src/test/java/org/alfresco/util/test/junitrules/RetryAtMostRule.java create mode 100644 repository/src/test/java/org/alfresco/util/test/junitrules/RetryAtMostRuleTest.java diff --git a/repository/src/test/java/org/alfresco/AllUnitTestsSuite.java b/repository/src/test/java/org/alfresco/AllUnitTestsSuite.java index 3392456a8a..0c113bcaee 100644 --- a/repository/src/test/java/org/alfresco/AllUnitTestsSuite.java +++ b/repository/src/test/java/org/alfresco/AllUnitTestsSuite.java @@ -119,6 +119,7 @@ import org.junit.runners.Suite; org.alfresco.util.schemacomp.validator.SchemaVersionValidatorTest.class, org.alfresco.util.schemacomp.validator.TypeNameOnlyValidatorTest.class, org.alfresco.util.test.OmittedTestClassFinderUnitTest.class, + org.alfresco.util.test.junitrules.RetryAtMostRuleTest.class, org.alfresco.util.test.junitrules.TemporaryMockOverrideTest.class, org.alfresco.repo.search.impl.solr.AbstractSolrQueryHTTPClientTest.class, org.alfresco.repo.search.impl.solr.SpellCheckDecisionManagerTest.class, diff --git a/repository/src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java b/repository/src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java index 091618e5f1..5bd0796347 100644 --- a/repository/src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java +++ b/repository/src/test/java/org/alfresco/repo/domain/permissions/FixedAclUpdaterTest.java @@ -4,27 +4,32 @@ * %% * Copyright (C) 2005 - 2021 Alfresco Software Limited * %% - * This file is part of the Alfresco software. - * If the software was purchased under a paid Alfresco license, the terms of - * the paid license agreement will prevail. Otherwise, the software is + * This file is part of the Alfresco software. + * If the software was purchased under a paid Alfresco license, the terms of + * the paid license agreement will prevail. Otherwise, the software is * provided under the following open source license terms: - * + * * Alfresco is free software: you can redistribute it and/or modify * it under the terms of the GNU Lesser General Public License as published by * the Free Software Foundation, either version 3 of the License, or * (at your option) any later version. - * + * * Alfresco is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU Lesser General Public License for more details. - * + * * You should have received a copy of the GNU Lesser General Public License * along with Alfresco. If not, see . * #L% */ package org.alfresco.repo.domain.permissions; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -61,23 +66,26 @@ import org.alfresco.service.cmr.security.PermissionService; import org.alfresco.service.namespace.QName; import org.alfresco.util.ApplicationContextHelper; import org.alfresco.util.Pair; +import org.alfresco.util.test.junitrules.RetryAtMostRule; +import org.alfresco.util.test.junitrules.RetryAtMostRule.RetryAtMost; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.context.ApplicationContext; import org.springframework.dao.ConcurrencyFailureException; -import junit.framework.TestCase; - /** * Test class for {@link FixedAclUpdater} - * + * * @author Andreea Dragoi * @author sglover * @since 4.2.7 * */ -public class FixedAclUpdaterTest extends TestCase +public class FixedAclUpdaterTest { private static final Logger LOG = LoggerFactory.getLogger(FixedAclUpdaterTest.class); @@ -103,7 +111,10 @@ public class FixedAclUpdaterTest extends TestCase private static String TEST_GROUP_NAME_FULL = PermissionService.GROUP_PREFIX + TEST_GROUP_NAME; private static String DEFAULT_PERMISSION = PermissionService.CONTRIBUTOR; - @Override + @Rule + public RetryAtMostRule retryAtMostRule = new RetryAtMostRule(); + + @Before public void setUp() throws Exception { ctx = ApplicationContextHelper.getApplicationContext(); @@ -126,7 +137,7 @@ public class FixedAclUpdaterTest extends TestCase setFixedAclMaxTransactionTime(permissionsDaoComponent, homeFolderNodeRef, maxTransactionTime); } - @Override + @After public void tearDown() throws Exception { AuthenticationUtil.clearCurrentSecurityContext(); @@ -202,6 +213,7 @@ public class FixedAclUpdaterTest extends TestCase * Test setting permissions explicitly as async */ @Test + @RetryAtMost(3) public void testAsync() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncFolder"); @@ -248,6 +260,7 @@ public class FixedAclUpdaterTest extends TestCase * MNT-21847 - Create a new content in folder that has the aspect applied */ @Test + @RetryAtMost(3) public void testAsyncWithNodeCreation() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeCreationFolder"); @@ -278,6 +291,7 @@ public class FixedAclUpdaterTest extends TestCase * MNT-22009 - Delete node that has the aspect applied before job runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeDeletion() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeDeletionFolder"); @@ -368,6 +382,7 @@ public class FixedAclUpdaterTest extends TestCase * MNT-22040 - Copy node that has the aspect applied before job runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeCopy() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeCopyOriginFolder"); @@ -447,6 +462,7 @@ public class FixedAclUpdaterTest extends TestCase * Copy node that has the aspect to another folder that also has the aspect applied before job runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeCopyToPendingFolder() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeCopyOriginFolder"); @@ -538,6 +554,7 @@ public class FixedAclUpdaterTest extends TestCase * runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeCopyParentToChildPendingFolder() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeCopyOriginFolder"); @@ -649,6 +666,7 @@ public class FixedAclUpdaterTest extends TestCase * runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeMoveChildToChildPendingFolder() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeMoveChildToChildPendingFolderOrigin"); @@ -734,6 +752,7 @@ public class FixedAclUpdaterTest extends TestCase * ACL */ @Test + @RetryAtMost(3) public void testAsyncWithErrorsForceSharedACL() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithErrorsForceSharedACL"); @@ -792,6 +811,7 @@ public class FixedAclUpdaterTest extends TestCase * MNT-22040 - Move node that has the aspect applied before job runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeMove() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeMoveOriginFolder"); @@ -868,6 +888,7 @@ public class FixedAclUpdaterTest extends TestCase * Move node that has the aspect to another folder that also has the aspect applied before job runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeMoveToPendingFolder() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeMoveOriginFolder"); @@ -956,6 +977,7 @@ public class FixedAclUpdaterTest extends TestCase * Lock node that has the aspect applied before job runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeLock() { NodeRef folderRef = createFolderHierarchyInRootForFileTests("testAsyncWithNodeLockFolder"); @@ -985,6 +1007,7 @@ public class FixedAclUpdaterTest extends TestCase * Checkout a node for editing that has the aspect applied before job runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeCheckout() { NodeRef folderRef = createFolderHierarchyInRootForFileTests("testAsyncWithNodeCheckoutFolder"); @@ -1015,6 +1038,7 @@ public class FixedAclUpdaterTest extends TestCase * Update the permissions of a node that has the aspect applied (new permissions: fixed) */ @Test + @RetryAtMost(3) public void testAsyncWithNodeUpdatePermissionsFixed() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeUpdatePermissionsFixedFolder"); @@ -1056,6 +1080,7 @@ public class FixedAclUpdaterTest extends TestCase * Update the permissions of a node that has the aspect applied (new permissions: shared) */ @Test + @RetryAtMost(3) public void testAsyncWithNodeUpdatePermissionsShared() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithNodeUpdatePermissionsSharedFolder"); @@ -1094,6 +1119,7 @@ public class FixedAclUpdaterTest extends TestCase * Update the permissions of the parent of a node that has the aspect applied (new permissions: fixed) */ @Test + @RetryAtMost(3) public void testAsyncWithParentUpdatePermissionsFixed() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithParentUpdatePermissionsFixedFolder"); @@ -1135,6 +1161,7 @@ public class FixedAclUpdaterTest extends TestCase * Update the permissions of the parent of a node that has the aspect applied (new permissions: shared) */ @Test + @RetryAtMost(3) public void testAsyncWithParentUpdatePermissionsShared() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncWithParentUpdatePermissionsSharedFolder"); @@ -1173,6 +1200,7 @@ public class FixedAclUpdaterTest extends TestCase } @Test + @RetryAtMost(3) public void testAsyncCascadeUpdatePermissions() { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncCascadeUpdatePermissionsFolder"); @@ -1225,6 +1253,7 @@ public class FixedAclUpdaterTest extends TestCase * Update the content of a node that has the aspect applied before job runs */ @Test + @RetryAtMost(3) public void testAsyncWithNodeContentUpdate() { NodeRef folderRef = createFolderHierarchyInRootForFileTests("testAsyncWithNodeContentUpdateFolder"); @@ -1257,6 +1286,7 @@ public class FixedAclUpdaterTest extends TestCase * Test setting permissions concurrently to actually cause the expected concurrency exception */ @Test + @RetryAtMost(3) public void testAsyncConcurrentPermissionsUpdate() throws Throwable { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncConcurrentPermissionsUpdateFolder"); @@ -1328,6 +1358,7 @@ public class FixedAclUpdaterTest extends TestCase * exception but the job should be able to recover */ @Test + @RetryAtMost(3) public void testAsyncConcurrentUpdateAndJob() throws Throwable { NodeRef folderRef = createFolderHierarchyInRootForFolderTests("testAsyncConcurrentUpdateAndJobFolder"); @@ -1802,7 +1833,7 @@ public class FixedAclUpdaterTest extends TestCase /** * Creates a level in folder/file hierarchy. Intermediate levels will contain folders and last ones files - * + * * @param fileFolderService * @param parent * - parent node of the of hierarchy level diff --git a/repository/src/test/java/org/alfresco/repo/security/SecurityTestSuite.java b/repository/src/test/java/org/alfresco/repo/security/SecurityTestSuite.java index 2509caecc7..4382fa91cc 100644 --- a/repository/src/test/java/org/alfresco/repo/security/SecurityTestSuite.java +++ b/repository/src/test/java/org/alfresco/repo/security/SecurityTestSuite.java @@ -99,7 +99,7 @@ public class SecurityTestSuite extends TestSuite suite.addTest(new JUnit4TestAdapter(HomeFolderProviderSynchronizerTest.class)); suite.addTest(new JUnit4TestAdapter(AlfrescoSSLSocketFactoryTest.class)); - suite.addTestSuite(FixedAclUpdaterTest.class); + suite.addTest(new JUnit4TestAdapter(FixedAclUpdaterTest.class)); suite.addTestSuite(DefaultRemoteUserMapperTest.class); suite.addTestSuite(IdentityServiceRemoteUserMapperTest.class); diff --git a/repository/src/test/java/org/alfresco/util/test/junitrules/RetryAtMostRule.java b/repository/src/test/java/org/alfresco/util/test/junitrules/RetryAtMostRule.java new file mode 100644 index 0000000000..1ca090c579 --- /dev/null +++ b/repository/src/test/java/org/alfresco/util/test/junitrules/RetryAtMostRule.java @@ -0,0 +1,132 @@ +/* + * #%L + * Alfresco Repository + * %% + * Copyright (C) 2005 - 2022 Alfresco Software Limited + * %% + * This file is part of the Alfresco software. + * If the software was purchased under a paid Alfresco license, the terms of + * the paid license agreement will prevail. Otherwise, the software is + * provided under the following open source license terms: + * + * Alfresco is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Alfresco is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Alfresco. If not, see . + * #L% + */ +package org.alfresco.util.test.junitrules; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.junit.Test; +import org.junit.rules.TestRule; +import org.junit.runner.Description; +import org.junit.runners.model.Statement; + +/** + * This JUnit rule can be used to turn existing test code into retryable tests. + * The test methods marked with the {@link RetryAtMost} annotation will be attempted at most the specified + * amount of times, stopping at the first successful execution. + * + * @author Domenico Sibilio + */ +public class RetryAtMostRule implements TestRule +{ + private static final Log LOG = LogFactory.getLog(RetryAtMostRule.class); + + @Override + public Statement apply(final Statement statement, final Description description) + { + RetryAtMost retryAtMost = description.getAnnotation(RetryAtMost.class); + + if (retryAtMost != null) + { + return new RetryAtMostTestStatement(statement, description, retryAtMost.value()); + } + + return statement; + } + + private static class RetryAtMostTestStatement extends Statement + { + private final Statement statement; + private final Description description; + private final int retryCount; + + private RetryAtMostTestStatement(Statement statement, Description description, int retryCount) + { + this.statement = statement; + this.description = description; + this.retryCount = retryCount; + } + + @Override + public void evaluate() throws Throwable + { + validate(); + for (int i = 0; i < retryCount; i++) + { + try + { + LOG.debug( + "Retryable testing configured for method: " + description.getMethodName() + + " // Attempt #" + (i + 1)); + statement.evaluate(); + break; // stop at the first successful execution + } + catch (Throwable t) + { + // ignore failed test runs unless it's the last possible execution + if (isLastExecution(i)) + { + throw t; + } + } + } + } + + private void validate() + { + if (retryCount < 1) + { + String methodName = description.getMethodName(); + throw new IllegalArgumentException( + "Invalid value for @RetryAtMost on method " + methodName + ": " + retryCount + " is less than 1."); + } + } + + private boolean isLastExecution(int i) + { + return i == retryCount - 1; + } + } + + /** + * This annotation is a marker used to identify a JUnit @{@link Test} method as a retryable test. + */ + @Target(ElementType.METHOD) + @Retention(RetentionPolicy.RUNTIME) + @Documented + public @interface RetryAtMost + { + /** + * @return The amount of times a test will be attempted, at most. + */ + int value() default 1; + } +} diff --git a/repository/src/test/java/org/alfresco/util/test/junitrules/RetryAtMostRuleTest.java b/repository/src/test/java/org/alfresco/util/test/junitrules/RetryAtMostRuleTest.java new file mode 100644 index 0000000000..f49f861094 --- /dev/null +++ b/repository/src/test/java/org/alfresco/util/test/junitrules/RetryAtMostRuleTest.java @@ -0,0 +1,138 @@ +/* + * #%L + * Alfresco Repository + * %% + * Copyright (C) 2005 - 2022 Alfresco Software Limited + * %% + * This file is part of the Alfresco software. + * If the software was purchased under a paid Alfresco license, the terms of + * the paid license agreement will prevail. Otherwise, the software is + * provided under the following open source license terms: + * + * Alfresco is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Alfresco is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Alfresco. If not, see . + * #L% + */ +package org.alfresco.util.test.junitrules; + +import static org.junit.Assert.assertSame; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; + +import java.lang.annotation.Annotation; +import java.util.concurrent.atomic.AtomicInteger; + +import org.alfresco.util.test.junitrules.RetryAtMostRule.RetryAtMost; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TestName; +import org.junit.runner.Description; +import org.junit.runner.RunWith; +import org.junit.runners.model.Statement; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +/** + * Test class for {@link RetryAtMostRule}. + * + * @author Domenico Sibilio + */ +@RunWith(MockitoJUnitRunner.class) +public class RetryAtMostRuleTest +{ + + private static final String ANNOTATION_WITH_NEGATIVE_VALUE = "annotationRetryAtMostNegativeTimes"; + private static final String ANNOTATION_RETRY_AT_MOST_THRICE = "annotationRetryAtMostThrice"; + private static final AtomicInteger EXECUTION_COUNT = new AtomicInteger(0); + @Rule + public RetryAtMostRule retryAtMostRule = new RetryAtMostRule(); + @Rule + public TestName testNameRule = new TestName(); + @Mock + private Statement statementMock; + + @Test + public void testSucceedOnFirstAttempt() throws Throwable + { + Description description = Description.createTestDescription(RetryAtMostRuleTest.class.getSimpleName(), + testNameRule.getMethodName(), getAnnotationByMethodName(ANNOTATION_RETRY_AT_MOST_THRICE)); + + Statement statement = retryAtMostRule.apply(statementMock, description); + statement.evaluate(); + verify(statementMock, times(1)).evaluate(); + } + + @Test + public void testSucceedOnSecondAttempt() throws Throwable + { + doThrow(new AssertionError("First execution should fail")).doNothing().when(statementMock).evaluate(); + + Description description = Description.createTestDescription(RetryAtMostRuleTest.class.getSimpleName(), + testNameRule.getMethodName(), getAnnotationByMethodName(ANNOTATION_RETRY_AT_MOST_THRICE)); + + Statement statement = retryAtMostRule.apply(statementMock, description); + statement.evaluate(); + verify(statementMock, times(2)).evaluate(); + } + + @Test + @RetryAtMost(3) + public void testSucceedOnThirdAttempt() + { + int currentExecution = EXECUTION_COUNT.incrementAndGet(); + assertSame("This test should be executed 3 times", 3, currentExecution); + } + + @Test(expected = AssertionError.class) + public void testFailAfterMaxAttempts() throws Throwable + { + doThrow(new AssertionError("All executions should fail")).when(statementMock).evaluate(); + + Description description = Description.createTestDescription(RetryAtMostRuleTest.class.getSimpleName(), + testNameRule.getMethodName(), getAnnotationByMethodName(ANNOTATION_RETRY_AT_MOST_THRICE)); + + Statement statement = retryAtMostRule.apply(statementMock, description); + statement.evaluate(); + } + + @Test(expected = IllegalArgumentException.class) + public void testInvalidRetryAtMostTimes() throws Throwable + { + Description description = Description.createTestDescription(RetryAtMostRuleTest.class.getSimpleName(), + testNameRule.getMethodName(), getAnnotationByMethodName(ANNOTATION_WITH_NEGATIVE_VALUE)); + + Statement statement = retryAtMostRule.apply(statementMock, description); + statement.evaluate(); + verifyNoInteractions(statementMock); + } + + private Annotation getAnnotationByMethodName(String methodName) throws NoSuchMethodException + { + return this.getClass().getMethod(methodName).getAnnotation(RetryAtMost.class); + } + + @RetryAtMost(-1) + public void annotationRetryAtMostNegativeTimes() + { + // intentionally empty + } + + @RetryAtMost(3) + public void annotationRetryAtMostThrice() + { + // intentionally empty + } + +} \ No newline at end of file