Skip to content

Commit

Permalink
CURATOR-541
Browse files Browse the repository at this point in the history 
The retry code in BaseClassForTests was hopelessly broken - I don't know for how long. Reworked it so that it does the right thing now (hopefully). Storing the retry count as an attribute wasn't working. The new method stores it as a field in the test class and makes sure that it's always correct. It should only ever be true when actually retrying and, given that TestNG always calls the retry method, it can be reset after a retry fails.
  • Loading branch information
randgalt committed Sep 21, 2019
1 parent db399d4 commit 3bef5e9
Showing 1 changed file with 40 additions and 104 deletions.
144 changes: 40 additions & 104 deletions curator-test/src/main/java/org/apache/curator/test/BaseClassForTests.java
View file
Expand Up @@ -23,22 +23,21 @@
import org.slf4j.LoggerFactory;
import org.testng.IInvokedMethod;
import org.testng.IInvokedMethodListener2;
import org.testng.IRetryAnalyzer;
import org.testng.ITestContext;
import org.testng.ITestResult;
import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.BeforeSuite;
import java.io.IOException;
import java.net.BindException;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;

public class BaseClassForTests
{
protected volatile TestingServer server;

private final Logger log = LoggerFactory.getLogger(getClass());
private final AtomicBoolean isRetrying = new AtomicBoolean(false);

private static final String INTERNAL_PROPERTY_DONT_LOG_CONNECTION_ISSUES;
private static final String INTERNAL_PROPERTY_REMOVE_WATCHERS_IN_FOREGROUND;
Expand Down Expand Up @@ -85,7 +84,33 @@ public class BaseClassForTests
@BeforeSuite(alwaysRun = true)
public void beforeSuite(ITestContext context)
{
context.getSuite().addListener(new MethodListener(log));
IInvokedMethodListener2 methodListener2 = new IInvokedMethodListener2()
{
@Override
public void beforeInvocation(IInvokedMethod method, ITestResult testResult)
{
method.getTestMethod().setRetryAnalyzer(BaseClassForTests.this::retry);
}

@Override
public void beforeInvocation(IInvokedMethod method, ITestResult testResult, ITestContext context)
{
beforeInvocation(method, testResult);
}

@Override
public void afterInvocation(IInvokedMethod method, ITestResult testResult, ITestContext context)
{
// NOP
}

@Override
public void afterInvocation(IInvokedMethod method, ITestResult testResult)
{
// NOP
}
};
context.getSuite().addListener(methodListener2);
}

@BeforeMethod
Expand Down Expand Up @@ -139,113 +164,24 @@ public void teardown() throws Exception
}
}

private static class RetryContext
{
final AtomicBoolean isRetrying = new AtomicBoolean(false);
final AtomicInteger runVersion = new AtomicInteger(0);
}

private static class RetryAnalyzer implements IRetryAnalyzer
private boolean retry(ITestResult result)
{
private final Logger log;
private final RetryContext retryContext;

RetryAnalyzer(Logger log, RetryContext retryContext)
if ( result.isSuccess() || isRetrying.get() )
{
this.log = log;
this.retryContext = Objects.requireNonNull(retryContext, "retryContext cannot be null");
isRetrying.set(false);
return false;
}

@Override
public boolean retry(ITestResult result)
result.setStatus(ITestResult.SKIP);
if ( result.getThrowable() != null )
{
if ( result.isSuccess() || retryContext.isRetrying.get() )
{
retryContext.isRetrying.set(false);
return false;
}

result.setStatus(ITestResult.SKIP);
if ( result.getThrowable() != null )
{
log.error("Retrying 1 time", result.getThrowable());
}
else
{
log.error("Retrying 1 time");
}
retryContext.isRetrying.set(true);
return true;
log.error("Retrying 1 time", result.getThrowable());
}
}

private static class MethodListener implements IInvokedMethodListener2
{
private final Logger log;

private static final String ATTRIBUTE_NAME = "__curator";

MethodListener(Logger log)
{
this.log = log;
}

@Override
public void beforeInvocation(IInvokedMethod method, ITestResult testResult)
{
// NOP
}

@Override
public void afterInvocation(IInvokedMethod method, ITestResult testResult)
else
{
// NOP
}

@Override
public void beforeInvocation(IInvokedMethod method, ITestResult testResult, ITestContext context)
{
if ( method.getTestMethod().isBeforeMethodConfiguration() )
{
RetryContext retryContext = (RetryContext)context.getAttribute(ATTRIBUTE_NAME);
if ( retryContext == null )
{
retryContext = new RetryContext();
context.setAttribute(ATTRIBUTE_NAME, retryContext);
}
}
else if ( method.isTestMethod() )
{
RetryContext retryContext = (RetryContext)context.getAttribute(ATTRIBUTE_NAME);
if ( retryContext != null )
{
method.getTestMethod().setRetryAnalyzer(new RetryAnalyzer(log, retryContext));
}
}
}

@Override
public void afterInvocation(IInvokedMethod method, ITestResult testResult, ITestContext context)
{
if ( method.isTestMethod() )
{
RetryContext retryContext = (RetryContext)context.getAttribute(ATTRIBUTE_NAME);
if ( retryContext == null )
{
log.error("No retryContext!");
}
else
{
if ( testResult.isSuccess() || (testResult.getStatus() == ITestResult.FAILURE) )
{
retryContext.isRetrying.set(false);
if ( retryContext.runVersion.incrementAndGet() > 1 )
{
context.setAttribute(ATTRIBUTE_NAME, null);
}
}
}
}
log.error("Retrying 1 time");
}
isRetrying.set(true);
return true;
}
}

0 comments on commit 3bef5e9

Please sign in to comment.