Avoid calling prepareListener once per attempt
Prepare the listener preparation once for the retry session
to make it cleaner.
Test: unit tests
Bug: 112664318
Change-Id: I1d3fd6d9a7b19504011386d0eb681bd84deafb6c
diff --git a/src/com/android/tradefed/testtype/suite/GranularRetriableTestWrapper.java b/src/com/android/tradefed/testtype/suite/GranularRetriableTestWrapper.java
index 5d7d060..e52eebe 100644
--- a/src/com/android/tradefed/testtype/suite/GranularRetriableTestWrapper.java
+++ b/src/com/android/tradefed/testtype/suite/GranularRetriableTestWrapper.java
@@ -72,10 +72,10 @@
private IInvocationContext mModuleInvocationContext;
private IConfiguration mModuleConfiguration;
private ModuleListener mMainGranularRunListener;
+ private RetryLogSaverResultForwarder mRetryAttemptForwarder;
private List<ITestInvocationListener> mModuleLevelListeners;
private ILogSaver mLogSaver;
private String mModuleId;
- private boolean mIsMetricCollectorInitialized;
private int mMaxRunLimit;
// Tracking of the metrics
@@ -97,7 +97,6 @@
mTest = test;
mMainGranularRunListener = new ModuleListener(mainListener);
mFailureListener = failureListener;
- mIsMetricCollectorInitialized = false;
mModuleLevelListeners = moduleLevelListeners;
mMaxRunLimit = maxRunLimit;
}
@@ -173,7 +172,7 @@
* TestFailureListener}, and wrapped by RunMetricsCollector and Module MetricCollector (if
* not initialized).
*/
- private ITestInvocationListener prepareRunListener(int attemptNumber) {
+ private ITestInvocationListener initializeListeners() {
List<ITestInvocationListener> currentTestListener = new ArrayList<>();
// Add all the module level listeners, including TestFailureListener
if (mModuleLevelListeners != null) {
@@ -181,52 +180,40 @@
}
currentTestListener.add(mMainGranularRunListener);
- ITestInvocationListener runListener =
- new LogSaverResultForwarder(mLogSaver, currentTestListener) {
- // Propagate the attempt number to TestRunStarted.
- @Override
- public void testRunStarted(String runName, int testCount) {
- testRunStarted(runName, testCount, attemptNumber);
- }
- };
+ mRetryAttemptForwarder = new RetryLogSaverResultForwarder(mLogSaver, currentTestListener);
+ ITestInvocationListener runListener = mRetryAttemptForwarder;
if (mFailureListener != null) {
- mFailureListener.setLogger(runListener);
+ mFailureListener.setLogger(mRetryAttemptForwarder);
currentTestListener.add(mFailureListener);
}
- // TODO: For RunMetricCollector and moduleMetricCollector, we only gather the
- // metrics in the first run. This part can be improved if we want to gather metrics for
- // every run.
- if (!mIsMetricCollectorInitialized) {
- if (mRunMetricCollectors != null) {
- // Module only init the collectors here to avoid triggering the collectors when
- // replaying the cached events at the end. This ensure metrics are capture at
- // the proper time in the invocation.
- for (IMetricCollector collector : mRunMetricCollectors) {
- runListener = collector.init(mModuleInvocationContext, runListener);
- }
- }
- // The module collectors itself are added: this list will be very limited.
- for (IMetricCollector collector : mModuleConfiguration.getMetricCollectors()) {
+ if (mRunMetricCollectors != null) {
+ // Module only init the collectors here to avoid triggering the collectors when
+ // replaying the cached events at the end. This ensure metrics are capture at
+ // the proper time in the invocation.
+ for (IMetricCollector collector : mRunMetricCollectors) {
runListener = collector.init(mModuleInvocationContext, runListener);
}
- mIsMetricCollectorInitialized = true;
}
+ // The module collectors itself are added: this list will be very limited.
+ for (IMetricCollector collector : mModuleConfiguration.getMetricCollectors()) {
+ runListener = collector.init(mModuleInvocationContext, runListener);
+ }
+
return runListener;
}
/**
- * Schedule a series of {@link IRemoteTest} "run". TODO: Customize the retry strategy; Each run
- * is granularized to a subset of the whole testcases.
+ * Schedule a series of {@link IRemoteTest#run(ITestInvocationListener)}.
*
* @param listener The ResultForwarder listener which contains a new moduleListener for each
* run.
*/
@Override
public void run(ITestInvocationListener listener) throws DeviceNotAvailableException {
+ ITestInvocationListener allListeners = initializeListeners();
// First do the regular run, not retried.
- int initAttempt = 0;
- intraModuleRun(initAttempt);
+ intraModuleRun(allListeners);
if (mMaxRunLimit <= 1) {
return;
@@ -234,10 +221,9 @@
// If the very first attempt failed, then don't proceed.
if (RetryStrategy.RERUN_UNTIL_FAILURE.equals(mRetryStrategy)) {
- Set<TestDescription> lastRun = getFailedTestCases(initAttempt);
+ Set<TestDescription> lastRun = getFailedTestCases(0);
// If we encountered a failure
- if (!lastRun.isEmpty()
- || mMainGranularRunListener.hasRunCrashedAtAttempt(initAttempt)) {
+ if (!lastRun.isEmpty() || mMainGranularRunListener.hasRunCrashedAtAttempt(0)) {
CLog.w("%s failed after the first run. Stopping.", lastRun);
return;
}
@@ -303,7 +289,7 @@
}
// Run the tests again
- intraModuleRun(attemptNumber);
+ intraModuleRun(allListeners);
Set<TestDescription> lastRun = getFailedTestCases(attemptNumber);
if (shouldHandleFailure(mRetryStrategy)) {
@@ -378,13 +364,9 @@
}
}
- /**
- * The workflow for each individual {@link IRemoteTest} run. TODO: When this function is called,
- * the IRemoteTest should already has the subset of testcases identified.
- */
- @VisibleForTesting
- final void intraModuleRun(int attemptNumber) throws DeviceNotAvailableException {
- ITestInvocationListener runListener = prepareRunListener(attemptNumber);
+ /** The workflow for each individual {@link IRemoteTest} run. */
+ private final void intraModuleRun(ITestInvocationListener runListener)
+ throws DeviceNotAvailableException {
try {
mTest.run(runListener);
} catch (RuntimeException re) {
@@ -401,6 +383,8 @@
CLog.w(due);
CLog.w("Proceeding to the next test.");
runListener.testRunFailed(due.getMessage());
+ } finally {
+ mRetryAttemptForwarder.incrementAttempt();
}
}
@@ -471,4 +455,25 @@
public ModuleListener getResultListener() {
return mMainGranularRunListener;
}
+
+ /** Forwarder that also handles passing the current attempt we are at. */
+ private class RetryLogSaverResultForwarder extends LogSaverResultForwarder {
+
+ private int mAttemptNumber = 0;
+
+ public RetryLogSaverResultForwarder(
+ ILogSaver logSaver, List<ITestInvocationListener> listeners) {
+ super(logSaver, listeners);
+ }
+
+ @Override
+ public void testRunStarted(String runName, int testCount) {
+ testRunStarted(runName, testCount, mAttemptNumber);
+ }
+
+ /** Increment the attempt number. */
+ public void incrementAttempt() {
+ mAttemptNumber++;
+ }
+ }
}
diff --git a/tests/src/com/android/tradefed/testtype/suite/GranularRetriableTestWrapperTest.java b/tests/src/com/android/tradefed/testtype/suite/GranularRetriableTestWrapperTest.java
index 6d8e39a..26c91e5 100644
--- a/tests/src/com/android/tradefed/testtype/suite/GranularRetriableTestWrapperTest.java
+++ b/tests/src/com/android/tradefed/testtype/suite/GranularRetriableTestWrapperTest.java
@@ -18,6 +18,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
import com.android.ddmlib.testrunner.TestResult.TestStatus;
import com.android.tradefed.config.IConfiguration;
@@ -248,43 +249,25 @@
}
/**
- * Test the intra module "run" triggers IRemoteTest run method with a dedicated ModuleListener.
+ * Test that the intra module retry does not inhibit DeviceNotAvailableException. They are
+ * bubbled up to the top.
*/
@Test
- public void testIntraModuleRun_pass() throws Exception {
- TestDescription fakeTestCase = new TestDescription("ClassFoo", "TestFoo");
-
- GranularRetriableTestWrapper granularTestWrapper =
- createGranularTestWrapper(new FakeTest(), 99);
- granularTestWrapper.setRetryStrategy(RetryStrategy.RETRY_TEST_CASE_FAILURE);
- assertEquals(0, granularTestWrapper.getTestRunResultCollected().size());
- granularTestWrapper.intraModuleRun(0);
- assertEquals(1, granularTestWrapper.getTestRunResultCollected().size());
- assertEquals(1, granularTestWrapper.getFinalTestRunResults().size());
- Set<TestDescription> completedTests =
- granularTestWrapper.getFinalTestRunResults().get(0).getCompletedTests();
- assertEquals(1, completedTests.size());
- assertTrue(completedTests.contains(fakeTestCase));
-
- // No retried run because all tests passed
- assertEquals(0, granularTestWrapper.getRetrySuccess());
- assertEquals(0, granularTestWrapper.getRetryFailed());
- assertEquals(0, granularTestWrapper.getRetryTime());
- }
-
- /**
- * Test that the intra module "run" method catches DeviceNotAvailableException and raises it
- * after record the tests.
- */
- @Test(expected = DeviceNotAvailableException.class)
public void testIntraModuleRun_catchDeviceNotAvailableException() throws Exception {
IRemoteTest mockTest = Mockito.mock(IRemoteTest.class);
Mockito.doThrow(new DeviceNotAvailableException("fake message", "serial"))
.when(mockTest)
.run(Mockito.any(ITestInvocationListener.class));
+
GranularRetriableTestWrapper granularTestWrapper = createGranularTestWrapper(mockTest, 1);
- // Verify.
- granularTestWrapper.intraModuleRun(0);
+ granularTestWrapper.setRetryStrategy(RetryStrategy.RETRY_TEST_CASE_FAILURE);
+ try {
+ granularTestWrapper.run(new CollectingTestListener());
+ fail("Should have thrown an exception.");
+ } catch (DeviceNotAvailableException expected) {
+ // Expected
+ assertEquals("fake message", expected.getMessage());
+ }
}
/**
@@ -303,7 +286,7 @@
}
};
GranularRetriableTestWrapper granularTestWrapper = createGranularTestWrapper(test, 1);
- granularTestWrapper.intraModuleRun(0);
+ granularTestWrapper.run(new CollectingTestListener());
TestRunResult attempResults =
granularTestWrapper.getTestRunResultCollected().get(RUN_NAME).get(0);
assertTrue(attempResults.isRunFailure());
