From bfab11dbf69efc3809090319b1f7fe83c240efe1 Mon Sep 17 00:00:00 2001 From: Andrei Zaitcev Date: Tue, 17 Jul 2018 10:19:41 +0100 Subject: [PATCH 1/3] Makes counter inside PoolTestCaseFailureAccumulator thread safe, also adds logs to understand when a test is due to be executed again --- .../com/shazam/fork/model/TestCaseEvent.java | 18 ++- .../fork/device/DeviceTestFilesCleaner.java | 6 +- ...leManagerBasedDeviceTestFilesCleaner.java} | 13 ++- .../model/PoolTestCaseFailureAccumulator.java | 16 ++- .../fork/runner/FailedTestScheduler.java | 7 ++ .../fork/runner/OverallProgressReporter.java | 26 ++--- .../ReporterBasedFailedTestScheduler.java | 30 +++++ .../com/shazam/fork/runner/TestRetryer.java | 8 -- .../shazam/fork/runner/TestRetryerImpl.java | 31 ----- .../fork/runner/listeners/RetryListener.java | 35 +++--- .../listeners/TestRunListenersFactory.java | 16 +-- .../shazam/fork/system/io/FileManager.java | 6 + .../fork/system/io/ForkFileManager.java | 24 +++- .../FilesRetrieverBasedAggregatorTest.java | 4 +- .../device/FakeDeviceTestFilesCleaner.java | 30 +++++ .../fork/runner/FakeFailedTestScheduler.java | 24 ++++ .../runner/FakePoolTestCaseAccumulator.java | 25 ++++- .../fork/runner/FakeProgressReporter.java | 90 +++++++++++++++ .../shazam/fork/runner/FakeTestRetryer.java | 25 ----- .../runner/OverallProgressReporterTest.java | 38 ++++--- .../ReporterBasedFailedTestSchedulerTest.java | 106 ++++++++++++++++++ .../runner/listeners/RetryListenerTest.java | 42 +++++-- .../summary/FakeDeviceTestFilesRetriever.java | 2 +- .../fork/util/TestPipelineEmulator.java | 4 +- 24 files changed, 469 insertions(+), 157 deletions(-) rename fork-runner/src/main/java/com/shazam/fork/device/{DeviceTestFilesCleanerImpl.java => FileManagerBasedDeviceTestFilesCleaner.java} (61%) create mode 100644 fork-runner/src/main/java/com/shazam/fork/runner/FailedTestScheduler.java create mode 100644 fork-runner/src/main/java/com/shazam/fork/runner/ReporterBasedFailedTestScheduler.java delete mode 100644 fork-runner/src/main/java/com/shazam/fork/runner/TestRetryer.java delete mode 100644 fork-runner/src/main/java/com/shazam/fork/runner/TestRetryerImpl.java create mode 100644 fork-runner/src/test/java/com/shazam/fork/device/FakeDeviceTestFilesCleaner.java create mode 100644 fork-runner/src/test/java/com/shazam/fork/runner/FakeFailedTestScheduler.java create mode 100644 fork-runner/src/test/java/com/shazam/fork/runner/FakeProgressReporter.java delete mode 100644 fork-runner/src/test/java/com/shazam/fork/runner/FakeTestRetryer.java create mode 100644 fork-runner/src/test/java/com/shazam/fork/runner/ReporterBasedFailedTestSchedulerTest.java diff --git a/fork-common/src/main/java/com/shazam/fork/model/TestCaseEvent.java b/fork-common/src/main/java/com/shazam/fork/model/TestCaseEvent.java index 5788670f..a871eb59 100644 --- a/fork-common/src/main/java/com/shazam/fork/model/TestCaseEvent.java +++ b/fork-common/src/main/java/com/shazam/fork/model/TestCaseEvent.java @@ -13,14 +13,17 @@ import static org.apache.commons.lang3.builder.ToStringStyle.SIMPLE_STYLE; public class TestCaseEvent { - private final String testMethod; private final String testClass; private final boolean isIgnored; private final List permissionsToRevoke; private final Map properties; - private TestCaseEvent(String testMethod, String testClass, boolean isIgnored, List permissionsToRevoke, Map properties) { + private TestCaseEvent(String testMethod, + String testClass, + boolean isIgnored, + List permissionsToRevoke, + Map properties) { this.testMethod = testMethod; this.testClass = testClass; this.isIgnored = isIgnored; @@ -28,7 +31,16 @@ private TestCaseEvent(String testMethod, String testClass, boolean isIgnored, Li this.properties = properties; } - public static TestCaseEvent newTestCase(String testMethod, String testClass, boolean isIgnored, List permissionsToRevoke, Map properties) { + @Nonnull + public static TestCaseEvent newTestCase(@Nonnull String testClass, @Nonnull String testMethod) { + return newTestCase(testMethod, testClass, false, emptyList(), emptyMap()); + } + + public static TestCaseEvent newTestCase(String testMethod, + String testClass, + boolean isIgnored, + List permissionsToRevoke, + Map properties) { return new TestCaseEvent(testMethod, testClass, isIgnored, permissionsToRevoke, properties); } diff --git a/fork-runner/src/main/java/com/shazam/fork/device/DeviceTestFilesCleaner.java b/fork-runner/src/main/java/com/shazam/fork/device/DeviceTestFilesCleaner.java index 0f378b2e..f42e5316 100644 --- a/fork-runner/src/main/java/com/shazam/fork/device/DeviceTestFilesCleaner.java +++ b/fork-runner/src/main/java/com/shazam/fork/device/DeviceTestFilesCleaner.java @@ -1,7 +1,9 @@ package com.shazam.fork.device; -import com.android.ddmlib.testrunner.TestIdentifier; +import com.shazam.fork.model.TestCaseEvent; + +import javax.annotation.Nonnull; public interface DeviceTestFilesCleaner { - boolean deleteTraceFiles(TestIdentifier testIdentifier); + boolean deleteTraceFiles(@Nonnull TestCaseEvent testCase); } diff --git a/fork-runner/src/main/java/com/shazam/fork/device/DeviceTestFilesCleanerImpl.java b/fork-runner/src/main/java/com/shazam/fork/device/FileManagerBasedDeviceTestFilesCleaner.java similarity index 61% rename from fork-runner/src/main/java/com/shazam/fork/device/DeviceTestFilesCleanerImpl.java rename to fork-runner/src/main/java/com/shazam/fork/device/FileManagerBasedDeviceTestFilesCleaner.java index c7da3821..2ad2182c 100644 --- a/fork-runner/src/main/java/com/shazam/fork/device/DeviceTestFilesCleanerImpl.java +++ b/fork-runner/src/main/java/com/shazam/fork/device/FileManagerBasedDeviceTestFilesCleaner.java @@ -1,30 +1,31 @@ package com.shazam.fork.device; -import com.android.ddmlib.testrunner.TestIdentifier; import com.shazam.fork.model.Device; import com.shazam.fork.model.Pool; +import com.shazam.fork.model.TestCaseEvent; import com.shazam.fork.system.io.FileManager; import com.shazam.fork.system.io.FileType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Nonnull; import java.io.File; -public class DeviceTestFilesCleanerImpl implements DeviceTestFilesCleaner { - private static final Logger logger = LoggerFactory.getLogger(DeviceTestFilesCleanerImpl.class); +public class FileManagerBasedDeviceTestFilesCleaner implements DeviceTestFilesCleaner { + private static final Logger logger = LoggerFactory.getLogger(FileManagerBasedDeviceTestFilesCleaner.class); private final FileManager fileManager; private final Pool pool; private final Device device; - public DeviceTestFilesCleanerImpl(FileManager fileManager, Pool pool, Device device) { + public FileManagerBasedDeviceTestFilesCleaner(FileManager fileManager, Pool pool, Device device) { this.fileManager = fileManager; this.pool = pool; this.device = device; } @Override - public boolean deleteTraceFiles(TestIdentifier testIdentifier) { - File file = fileManager.getFile(FileType.TEST, pool.getName(), device.getSafeSerial(), testIdentifier); + public boolean deleteTraceFiles(@Nonnull TestCaseEvent testCase) { + File file = fileManager.getFile(FileType.TEST, pool, device, testCase); boolean isDeleted = file.delete(); if (!isDeleted) { logger.warn("Failed to delete a file %s", file.getAbsolutePath()); diff --git a/fork-runner/src/main/java/com/shazam/fork/model/PoolTestCaseFailureAccumulator.java b/fork-runner/src/main/java/com/shazam/fork/model/PoolTestCaseFailureAccumulator.java index 8966b0d3..e7aa6db2 100644 --- a/fork-runner/src/main/java/com/shazam/fork/model/PoolTestCaseFailureAccumulator.java +++ b/fork-runner/src/main/java/com/shazam/fork/model/PoolTestCaseFailureAccumulator.java @@ -1,18 +1,28 @@ +/* + * Copyright 2018 Shazam Entertainment Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. + * + * You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ + package com.shazam.fork.model; import com.google.common.base.Predicate; -import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Multimaps; import com.google.common.collect.SetMultimap; import static com.google.common.collect.FluentIterable.from; +import static com.google.common.collect.HashMultimap.create; /** * Class that keeps track of the number of times each testCase is executed for device. */ public class PoolTestCaseFailureAccumulator implements PoolTestCaseAccumulator { - - private SetMultimap map = HashMultimap.create(); + private SetMultimap map = Multimaps.synchronizedSetMultimap(create()); @Override public void record(Pool pool, TestCaseEvent testCaseEvent) { diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/FailedTestScheduler.java b/fork-runner/src/main/java/com/shazam/fork/runner/FailedTestScheduler.java new file mode 100644 index 00000000..516a59fb --- /dev/null +++ b/fork-runner/src/main/java/com/shazam/fork/runner/FailedTestScheduler.java @@ -0,0 +1,7 @@ +package com.shazam.fork.runner; + +import com.shazam.fork.model.TestCaseEvent; + +public interface FailedTestScheduler { + boolean rescheduleTestExecution(TestCaseEvent testCaseEvent); +} diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/OverallProgressReporter.java b/fork-runner/src/main/java/com/shazam/fork/runner/OverallProgressReporter.java index bbaec24c..d75d5e08 100644 --- a/fork-runner/src/main/java/com/shazam/fork/runner/OverallProgressReporter.java +++ b/fork-runner/src/main/java/com/shazam/fork/runner/OverallProgressReporter.java @@ -10,8 +10,9 @@ package com.shazam.fork.runner; -import com.shazam.fork.model.*; - +import com.shazam.fork.model.Pool; +import com.shazam.fork.model.PoolTestCaseAccumulator; +import com.shazam.fork.model.TestCaseEvent; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -115,12 +116,12 @@ private class RetryWatchdog { private final AtomicInteger totalAllowedRetryLeft; private final StringBuilder logBuilder = new StringBuilder(); - public RetryWatchdog(int totalAllowedRetryQuota, int retryPerTestCaseQuota) { + RetryWatchdog(int totalAllowedRetryQuota, int retryPerTestCaseQuota) { totalAllowedRetryLeft = new AtomicInteger(totalAllowedRetryQuota); maxRetryPerTestCaseQuota = retryPerTestCaseQuota; } - public boolean requestRetry(int currentSingleTestCaseFailures) { + boolean requestRetry(int currentSingleTestCaseFailures) { boolean totalAllowedRetryAvailable = totalAllowedRetryAvailable(); boolean singleTestAllowed = currentSingleTestCaseFailures <= maxRetryPerTestCaseQuota; boolean result = totalAllowedRetryAvailable && singleTestAllowed; @@ -134,15 +135,14 @@ private boolean totalAllowedRetryAvailable() { } private void log(int testCaseFailures, boolean singleTestAllowed, boolean result) { - if(logger.isDebugEnabled()) { - logBuilder.setLength(0); //clean up. - logBuilder.append("Retry requested ") - .append(result ? " and allowed. " : " but not allowed. ") - .append("Total retry left :").append(totalAllowedRetryLeft.get()) - .append(" and Single Test case retry left: ") - .append(singleTestAllowed ? maxRetryPerTestCaseQuota - testCaseFailures : 0); - logger.debug(logBuilder.toString()); - } + logBuilder.setLength(0); //clean up. + logBuilder.append("Retry requested ") + .append(result ? " and allowed. " : " but not allowed. ") + .append("Total retry left :").append(totalAllowedRetryLeft.get()) + .append(" and Single Test case retry left: ") + .append(singleTestAllowed ? maxRetryPerTestCaseQuota - testCaseFailures : 0); + logger.debug(logBuilder.toString()); + System.out.println(logBuilder.toString()); } } } diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/ReporterBasedFailedTestScheduler.java b/fork-runner/src/main/java/com/shazam/fork/runner/ReporterBasedFailedTestScheduler.java new file mode 100644 index 00000000..f1b9879c --- /dev/null +++ b/fork-runner/src/main/java/com/shazam/fork/runner/ReporterBasedFailedTestScheduler.java @@ -0,0 +1,30 @@ +package com.shazam.fork.runner; + +import com.shazam.fork.model.Pool; +import com.shazam.fork.model.TestCaseEvent; + +import java.util.Queue; + +public class ReporterBasedFailedTestScheduler implements FailedTestScheduler { + private final ProgressReporter progressReporter; + private final Pool pool; + private final Queue queueOfTestsInPool; + + public ReporterBasedFailedTestScheduler(ProgressReporter progressReporter, + Pool pool, + Queue queueOfTestsInPool) { + this.progressReporter = progressReporter; + this.pool = pool; + this.queueOfTestsInPool = queueOfTestsInPool; + } + + @Override + public boolean rescheduleTestExecution(TestCaseEvent testCaseEvent) { + progressReporter.recordFailedTestCase(pool, testCaseEvent); + if (progressReporter.requestRetry(pool, testCaseEvent)) { + queueOfTestsInPool.add(testCaseEvent); + return true; + } + return false; + } +} diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/TestRetryer.java b/fork-runner/src/main/java/com/shazam/fork/runner/TestRetryer.java deleted file mode 100644 index b650a370..00000000 --- a/fork-runner/src/main/java/com/shazam/fork/runner/TestRetryer.java +++ /dev/null @@ -1,8 +0,0 @@ -package com.shazam.fork.runner; - -import com.android.ddmlib.testrunner.TestIdentifier; -import com.shazam.fork.model.TestCaseEvent; - -public interface TestRetryer { - boolean rescheduleTestExecution(TestIdentifier testIdentifier, TestCaseEvent testCaseEvent); -} diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/TestRetryerImpl.java b/fork-runner/src/main/java/com/shazam/fork/runner/TestRetryerImpl.java deleted file mode 100644 index 321e162f..00000000 --- a/fork-runner/src/main/java/com/shazam/fork/runner/TestRetryerImpl.java +++ /dev/null @@ -1,31 +0,0 @@ -package com.shazam.fork.runner; - -import com.android.ddmlib.testrunner.TestIdentifier; -import com.shazam.fork.model.Pool; -import com.shazam.fork.model.TestCaseEvent; - -import java.util.Queue; - -import static com.shazam.fork.model.TestCaseEvent.newTestCase; - -public class TestRetryerImpl implements TestRetryer { - private final ProgressReporter progressReporter; - private final Pool pool; - private final Queue queueOfTestsInPool; - - public TestRetryerImpl(ProgressReporter progressReporter, Pool pool, Queue queueOfTestsInPool) { - this.progressReporter = progressReporter; - this.pool = pool; - this.queueOfTestsInPool = queueOfTestsInPool; - } - - @Override - public boolean rescheduleTestExecution(TestIdentifier testIdentifier, TestCaseEvent testCaseEvent) { - progressReporter.recordFailedTestCase(pool, newTestCase(testIdentifier)); - if (progressReporter.requestRetry(pool, newTestCase(testIdentifier))) { - queueOfTestsInPool.add(testCaseEvent); - return true; - } - return false; - } -} diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/RetryListener.java b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/RetryListener.java index f71faf58..f1f87234 100644 --- a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/RetryListener.java +++ b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/RetryListener.java @@ -15,47 +15,44 @@ import com.shazam.fork.model.Device; import com.shazam.fork.model.Pool; import com.shazam.fork.model.TestCaseEvent; -import com.shazam.fork.runner.TestRetryer; +import com.shazam.fork.runner.FailedTestScheduler; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.util.Map; import static com.google.common.base.Preconditions.checkNotNull; +import static com.shazam.fork.model.TestCaseEvent.newTestCase; public class RetryListener extends NoOpITestRunListener { private static final Logger logger = LoggerFactory.getLogger(RetryListener.class); private final Device device; - private final TestCaseEvent currentTestCaseEvent; - private final TestRetryer testRetryer; + private final FailedTestScheduler failedTestScheduler; private final Pool pool; private final DeviceTestFilesCleaner deviceTestFilesCleaner; - private TestIdentifier startedTest; - private TestIdentifier failedTest; + private TestCaseEvent startedTest; + private TestCaseEvent failedTest; public RetryListener(Pool pool, Device device, - TestCaseEvent currentTestCaseEvent, - TestRetryer testRetryer, + FailedTestScheduler failedTestScheduler, DeviceTestFilesCleaner deviceTestFilesCleaner) { checkNotNull(device); - checkNotNull(currentTestCaseEvent); checkNotNull(pool); - this.testRetryer = testRetryer; + this.failedTestScheduler = failedTestScheduler; this.device = device; - this.currentTestCaseEvent = currentTestCaseEvent; this.pool = pool; this.deviceTestFilesCleaner = deviceTestFilesCleaner; } @Override public void testStarted(TestIdentifier test) { - startedTest = test; + startedTest = newTestCase(test); } @Override public void testFailed(TestIdentifier test, String trace) { - failedTest = test; + failedTest = newTestCase(test); } @Override @@ -74,18 +71,18 @@ public void testRunEnded(long elapsedTime, Map runMetrics) { } } - private void rescheduleTestExecution(TestIdentifier test) { - if (testRetryer.rescheduleTestExecution(test, currentTestCaseEvent)) { - logger.info("Test " + test.toString() + " enqueued again into pool:" + pool.getName()); - removeFailureTraceFiles(test); + private void rescheduleTestExecution(TestCaseEvent testCase) { + if (failedTestScheduler.rescheduleTestExecution(testCase)) { + logger.info("Test " + testCase.getTestFullName() + " enqueued again into pool:" + pool.getName()); + removeFailureTraceFiles(testCase); } else { - logger.info("Test " + test.toString() + " failed on device " + device.getSafeSerial() + logger.info("Test " + testCase.getTestFullName() + " failed on device " + device.getSafeSerial() + " but retry is not allowed."); } } - private void removeFailureTraceFiles(TestIdentifier test) { - boolean isDeleted = deviceTestFilesCleaner.deleteTraceFiles(test); + private void removeFailureTraceFiles(TestCaseEvent testCase) { + boolean isDeleted = deviceTestFilesCleaner.deleteTraceFiles(testCase); if (!isDeleted) { logger.warn("Failed to remove a trace filed for a failed but enqueued again test"); } diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/TestRunListenersFactory.java b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/TestRunListenersFactory.java index caf0cb52..bbcc3ce4 100644 --- a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/TestRunListenersFactory.java +++ b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/TestRunListenersFactory.java @@ -13,12 +13,12 @@ import com.android.ddmlib.testrunner.ITestRunListener; import com.google.gson.Gson; import com.shazam.fork.Configuration; -import com.shazam.fork.device.DeviceTestFilesCleanerImpl; +import com.shazam.fork.device.FileManagerBasedDeviceTestFilesCleaner; import com.shazam.fork.model.Device; import com.shazam.fork.model.Pool; import com.shazam.fork.model.TestCaseEvent; import com.shazam.fork.runner.ProgressReporter; -import com.shazam.fork.runner.TestRetryerImpl; +import com.shazam.fork.runner.ReporterBasedFailedTestScheduler; import com.shazam.fork.system.io.FileManager; import java.io.File; @@ -56,18 +56,18 @@ public List createTestListeners(TestCaseEvent testCase, new LogCatTestRunListener(gson, fileManager, pool, device), new SlowWarningTestRunListener(), getScreenTraceTestRunListener(fileManager, pool, device), - buildRetryListener(testCase, device, pool, progressReporter, testCaseEventQueue), + buildRetryListener(device, pool, progressReporter, testCaseEventQueue), getCoverageTestRunListener(configuration, device, fileManager, pool, testCase)); } - private RetryListener buildRetryListener(TestCaseEvent testCase, - Device device, + private RetryListener buildRetryListener(Device device, Pool pool, ProgressReporter progressReporter, Queue testCaseEventQueue) { - TestRetryerImpl testRetryer = new TestRetryerImpl(progressReporter, pool, testCaseEventQueue); - DeviceTestFilesCleanerImpl deviceTestFilesCleaner = new DeviceTestFilesCleanerImpl(fileManager, pool, device); - return new RetryListener(pool, device, testCase, testRetryer, deviceTestFilesCleaner); + ReporterBasedFailedTestScheduler testScheduler = + new ReporterBasedFailedTestScheduler(progressReporter, pool, testCaseEventQueue); + FileManagerBasedDeviceTestFilesCleaner deviceTestFilesCleaner = new FileManagerBasedDeviceTestFilesCleaner(fileManager, pool, device); + return new RetryListener(pool, device, testScheduler, deviceTestFilesCleaner); } private ForkXmlTestRunListener getForkXmlTestRunListener(FileManager fileManager, diff --git a/fork-runner/src/main/java/com/shazam/fork/system/io/FileManager.java b/fork-runner/src/main/java/com/shazam/fork/system/io/FileManager.java index 065ae291..5f625ac2 100644 --- a/fork-runner/src/main/java/com/shazam/fork/system/io/FileManager.java +++ b/fork-runner/src/main/java/com/shazam/fork/system/io/FileManager.java @@ -17,6 +17,7 @@ import com.shazam.fork.model.Pool; import com.shazam.fork.model.TestCaseEvent; +import javax.annotation.Nonnull; import java.io.File; public interface FileManager { @@ -33,4 +34,9 @@ public interface FileManager { File[] getFiles(FileType fileType, Pool pool, Device device, TestIdentifier testIdentifier); File getFile(FileType fileType, String pool, String safeSerial, TestIdentifier testIdentifier); + + File getFile(@Nonnull FileType fileType, + @Nonnull Pool pool, + @Nonnull Device device, + @Nonnull TestCaseEvent testCase); } \ No newline at end of file diff --git a/fork-runner/src/main/java/com/shazam/fork/system/io/ForkFileManager.java b/fork-runner/src/main/java/com/shazam/fork/system/io/ForkFileManager.java index 019383d1..9dc72fad 100644 --- a/fork-runner/src/main/java/com/shazam/fork/system/io/ForkFileManager.java +++ b/fork-runner/src/main/java/com/shazam/fork/system/io/ForkFileManager.java @@ -8,12 +8,14 @@ import org.apache.commons.io.filefilter.PrefixFileFilter; import org.apache.commons.io.filefilter.SuffixFileFilter; +import javax.annotation.Nonnull; import java.io.File; import java.io.FileFilter; import java.io.IOException; import java.nio.file.Path; import static com.shazam.fork.CommonDefaults.FORK_SUMMARY_FILENAME_FORMAT; +import static com.shazam.fork.model.TestCaseEvent.newTestCase; import static com.shazam.fork.system.io.FileType.TEST; import static java.nio.file.Files.createDirectories; import static java.nio.file.Paths.get; @@ -51,7 +53,7 @@ public File createFile(FileType fileType, Pool pool, Device device, TestIdentifi public File createFile(FileType fileType, Pool pool, Device device, TestIdentifier testIdentifier) { try { Path directory = createDirectory(fileType, pool, device); - String filename = createFilenameForTest(testIdentifier, fileType); + String filename = createFilenameForTest(newTestCase(testIdentifier), fileType); return createFile(directory, filename); } catch (IOException e) { throw new CouldNotCreateDirectoryException(e); @@ -81,11 +83,25 @@ public File[] getFiles(FileType fileType, Pool pool, Device device, TestIdentifi @Override public File getFile(FileType fileType, String pool, String safeSerial, TestIdentifier testIdentifier) { - String filenameForTest = createFilenameForTest(testIdentifier, fileType); + String filenameForTest = createFilenameForTest(newTestCase(testIdentifier), fileType); Path path = get(output.getAbsolutePath(), fileType.getDirectory(), pool, safeSerial, filenameForTest); return path.toFile(); } + @Override + public File getFile(@Nonnull FileType fileType, + @Nonnull Pool pool, + @Nonnull Device device, + @Nonnull TestCaseEvent testCase) { + String filenameForTest = createFilenameForTest(testCase, fileType); + Path path = get(output.getAbsolutePath(), + fileType.getDirectory(), + pool.getName(), + device.getSafeSerial(), + filenameForTest); + return path.toFile(); + } + private Path createDirectory(FileType test, Pool pool, Device device) throws IOException { return createDirectories(getDirectory(test, pool, device)); } @@ -98,8 +114,8 @@ private File createFile(Path directory, String filename) { return new File(directory.toFile(), filename); } - private String createFilenameForTest(TestIdentifier testIdentifier, FileType fileType) { - return String.format("%s.%s", testIdentifier.toString(), fileType.getSuffix()); + private String createFilenameForTest(TestCaseEvent testCase, FileType fileType) { + return String.format("%s.%s", testCase.getTestFullName(), fileType.getSuffix()); } private String createFilenameForTest(TestIdentifier testIdentifier, FileType fileType, int sequenceNumber) { diff --git a/fork-runner/src/test/java/com/shazam/fork/aggregator/FilesRetrieverBasedAggregatorTest.java b/fork-runner/src/test/java/com/shazam/fork/aggregator/FilesRetrieverBasedAggregatorTest.java index 28dee44c..f755fa7b 100644 --- a/fork-runner/src/test/java/com/shazam/fork/aggregator/FilesRetrieverBasedAggregatorTest.java +++ b/fork-runner/src/test/java/com/shazam/fork/aggregator/FilesRetrieverBasedAggregatorTest.java @@ -16,7 +16,7 @@ import static com.shazam.fork.model.Device.Builder.aDevice; import static com.shazam.fork.model.Pool.Builder.aDevicePool; import static com.shazam.fork.model.TestCaseEvent.newTestCase; -import static com.shazam.fork.summary.FakeDeviceTestFilesRetriever.aFakeDeviceTestFilesRetriever; +import static com.shazam.fork.summary.FakeDeviceTestFilesRetriever.fakeDeviceTestFilesRetriever; import static com.shazam.fork.summary.TestResult.Builder.aTestResult; import static com.shazam.fork.summary.TestResult.SUMMARY_KEY_TOTAL_FAILURE_COUNT; import static java.util.Collections.emptyList; @@ -85,7 +85,7 @@ public class FilesRetrieverBasedAggregatorTest { newTestCase(new TestIdentifier("com.example.FatalCrashedTest", "doesJobProperly")) ); - private final FakeDeviceTestFilesRetriever fakeDeviceTestFilesRetriever = aFakeDeviceTestFilesRetriever(); + private final FakeDeviceTestFilesRetriever fakeDeviceTestFilesRetriever = fakeDeviceTestFilesRetriever(); private FilesRetrieverBasedAggregator aggregator; @Before diff --git a/fork-runner/src/test/java/com/shazam/fork/device/FakeDeviceTestFilesCleaner.java b/fork-runner/src/test/java/com/shazam/fork/device/FakeDeviceTestFilesCleaner.java new file mode 100644 index 00000000..f234a1fb --- /dev/null +++ b/fork-runner/src/test/java/com/shazam/fork/device/FakeDeviceTestFilesCleaner.java @@ -0,0 +1,30 @@ +/* + * Copyright 2018 Shazam Entertainment Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. + * + * You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ + +package com.shazam.fork.device; + +import com.shazam.fork.model.TestCaseEvent; + +import javax.annotation.Nonnull; + +public final class FakeDeviceTestFilesCleaner implements DeviceTestFilesCleaner { + private FakeDeviceTestFilesCleaner() { + } + + @Nonnull + public static FakeDeviceTestFilesCleaner fakeDeviceTestFilesCleaner() { + return new FakeDeviceTestFilesCleaner(); + } + + @Override + public boolean deleteTraceFiles(@Nonnull TestCaseEvent testCase) { + return false; + } +} diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/FakeFailedTestScheduler.java b/fork-runner/src/test/java/com/shazam/fork/runner/FakeFailedTestScheduler.java new file mode 100644 index 00000000..815e8744 --- /dev/null +++ b/fork-runner/src/test/java/com/shazam/fork/runner/FakeFailedTestScheduler.java @@ -0,0 +1,24 @@ +package com.shazam.fork.runner; + +import com.shazam.fork.model.TestCaseEvent; + +public class FakeFailedTestScheduler implements FailedTestScheduler { + private boolean result; + + private FakeFailedTestScheduler() { + } + + public static FakeFailedTestScheduler fakeFailedTestScheduler() { + return new FakeFailedTestScheduler(); + } + + public FakeFailedTestScheduler thatReturns(boolean result) { + this.result = result; + return this; + } + + @Override + public boolean rescheduleTestExecution(TestCaseEvent testCaseEvent) { + return result; + } +} diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/FakePoolTestCaseAccumulator.java b/fork-runner/src/test/java/com/shazam/fork/runner/FakePoolTestCaseAccumulator.java index 754aa45e..d57408e0 100644 --- a/fork-runner/src/test/java/com/shazam/fork/runner/FakePoolTestCaseAccumulator.java +++ b/fork-runner/src/test/java/com/shazam/fork/runner/FakePoolTestCaseAccumulator.java @@ -4,16 +4,29 @@ import com.shazam.fork.model.PoolTestCaseAccumulator; import com.shazam.fork.model.TestCaseEvent; +import javax.annotation.Nonnull; + public class FakePoolTestCaseAccumulator implements PoolTestCaseAccumulator { + private int poolCount; + private int testCaseCount; - private int count = 0; + private FakePoolTestCaseAccumulator() { + } - public static FakePoolTestCaseAccumulator aFakePoolTestCaseAccumulator(){ + @Nonnull + public static FakePoolTestCaseAccumulator fakePoolTestCaseAccumulator() { return new FakePoolTestCaseAccumulator(); } - public FakePoolTestCaseAccumulator thatAlwaysReturns(int count){ - this.count = count; + @Nonnull + public FakePoolTestCaseAccumulator thatAlwaysReturnsPoolCount(int count) { + poolCount = count; + return this; + } + + @Nonnull + public FakePoolTestCaseAccumulator thatAlwaysReturnsTestCaseCount(int count) { + testCaseCount = count; return this; } @@ -23,11 +36,11 @@ public void record(Pool pool, TestCaseEvent testCaseEvent) { @Override public int getCount(Pool pool, TestCaseEvent testCaseEvent) { - return count; + return poolCount; } @Override public int getCount(TestCaseEvent testCaseEvent) { - return count; + return testCaseCount; } } diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/FakeProgressReporter.java b/fork-runner/src/test/java/com/shazam/fork/runner/FakeProgressReporter.java new file mode 100644 index 00000000..2e31058b --- /dev/null +++ b/fork-runner/src/test/java/com/shazam/fork/runner/FakeProgressReporter.java @@ -0,0 +1,90 @@ +/* + * Copyright 2018 Shazam Entertainment Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. + * + * You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ + +package com.shazam.fork.runner; + +import com.shazam.fork.model.Pool; +import com.shazam.fork.model.TestCaseEvent; + +import javax.annotation.Nonnull; + +public final class FakeProgressReporter implements ProgressReporter { + private boolean canBeRescheduled; + + private FakeProgressReporter() { + } + + @Nonnull + public static FakeProgressReporter fakeProgressReporter() { + return new FakeProgressReporter(); + } + + @Nonnull + public FakeProgressReporter thatAlwaysDisallowTestToBeRescheduled() { + canBeRescheduled = false; + return this; + } + + @Nonnull + public FakeProgressReporter thatAlwaysAllowTestToBeRescheduled() { + canBeRescheduled = true; + return this; + } + + @Override + public void start() { + + } + + @Override + public void stop() { + + } + + @Override + public void addPoolProgress(Pool pool, PoolProgressTracker poolProgressTracker) { + + } + + @Override + public PoolProgressTracker getProgressTrackerFor(Pool pool) { + return null; + } + + @Override + public long millisSinceTestsStarted() { + return 0; + } + + @Override + public int getFailures() { + return 0; + } + + @Override + public float getProgress() { + return 0; + } + + @Override + public boolean requestRetry(Pool pool, TestCaseEvent testCaseEvent) { + return canBeRescheduled; + } + + @Override + public void recordFailedTestCase(Pool pool, TestCaseEvent testCase) { + + } + + @Override + public int getTestFailuresCount(Pool pool, TestCaseEvent testCase) { + return 0; + } +} diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/FakeTestRetryer.java b/fork-runner/src/test/java/com/shazam/fork/runner/FakeTestRetryer.java deleted file mode 100644 index 3df5a1e5..00000000 --- a/fork-runner/src/test/java/com/shazam/fork/runner/FakeTestRetryer.java +++ /dev/null @@ -1,25 +0,0 @@ -package com.shazam.fork.runner; - -import com.android.ddmlib.testrunner.TestIdentifier; -import com.shazam.fork.model.TestCaseEvent; - -public class FakeTestRetryer implements TestRetryer { - private boolean result; - - private FakeTestRetryer() { - } - - public static FakeTestRetryer fakeTestRetryer() { - return new FakeTestRetryer(); - } - - public FakeTestRetryer thatReturns(boolean result) { - this.result = result; - return this; - } - - @Override - public boolean rescheduleTestExecution(TestIdentifier testIdentifier, TestCaseEvent testCaseEvent) { - return result; - } -} diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/OverallProgressReporterTest.java b/fork-runner/src/test/java/com/shazam/fork/runner/OverallProgressReporterTest.java index d96a1c5a..9d588070 100644 --- a/fork-runner/src/test/java/com/shazam/fork/runner/OverallProgressReporterTest.java +++ b/fork-runner/src/test/java/com/shazam/fork/runner/OverallProgressReporterTest.java @@ -3,7 +3,6 @@ import com.shazam.fork.model.Device; import com.shazam.fork.model.Pool; import com.shazam.fork.model.TestCaseEvent; - import org.jmock.Expectations; import org.jmock.auto.Mock; import org.jmock.integration.junit4.JUnitRuleMockery; @@ -13,28 +12,28 @@ import static com.shazam.fork.model.Device.Builder.aDevice; import static com.shazam.fork.model.Pool.Builder.aDevicePool; import static com.shazam.fork.model.TestCaseEvent.newTestCase; -import static com.shazam.fork.runner.FakePoolTestCaseAccumulator.aFakePoolTestCaseAccumulator; +import static com.shazam.fork.runner.FakePoolTestCaseAccumulator.fakePoolTestCaseAccumulator; import static com.shazam.fork.runner.FakeProgressReporterTrackers.aFakeProgressReporterTrackers; -import static java.util.Collections.emptyList; -import static java.util.Collections.emptyMap; public class OverallProgressReporterTest { - - @Rule public JUnitRuleMockery mockery = new JUnitRuleMockery(); - @Mock private PoolProgressTracker mockPoolProgressTracker; - private final FakePoolTestCaseAccumulator fakeTestCasesAccumulator = aFakePoolTestCaseAccumulator(); + @Rule + public JUnitRuleMockery mockery = new JUnitRuleMockery(); + @Mock + private PoolProgressTracker mockPoolProgressTracker; + private final FakePoolTestCaseAccumulator fakeTestCasesAccumulator = fakePoolTestCaseAccumulator(); private final Device A_DEVICE = aDevice().build(); private final Pool A_POOL = aDevicePool() .addDevice(A_DEVICE) .build(); - private final TestCaseEvent A_TEST_CASE = newTestCase("aTestMethod", "aTestClass", false, emptyList(), emptyMap()); + private final TestCaseEvent A_TEST_CASE = + newTestCase("aTestClass", "aTestMethod"); private OverallProgressReporter overallProgressReporter; @Test - public void requestRetryIsAllowedIfFailedLessThanPermitted() throws Exception { - fakeTestCasesAccumulator.thatAlwaysReturns(0); + public void requestRetryIsAllowedIfFailedLessThanPermitted() { + fakeTestCasesAccumulator.thatAlwaysReturnsPoolCount(0); overallProgressReporter = new OverallProgressReporter(1, 1, aFakeProgressReporterTrackers().thatAlwaysReturns(mockPoolProgressTracker), fakeTestCasesAccumulator); @@ -47,8 +46,8 @@ public void requestRetryIsAllowedIfFailedLessThanPermitted() throws Exception { } @Test - public void requestRetryIsNotAllowedIfFailedMoreThanPermitted() throws Exception { - fakeTestCasesAccumulator.thatAlwaysReturns(2); + public void requestRetryIsNotAllowedIfFailedMoreThanGloballyPermitted() { + fakeTestCasesAccumulator.thatAlwaysReturnsTestCaseCount(2); overallProgressReporter = new OverallProgressReporter(1, 1, aFakeProgressReporterTrackers().thatAlwaysReturns(mockPoolProgressTracker), fakeTestCasesAccumulator); @@ -60,4 +59,17 @@ public void requestRetryIsNotAllowedIfFailedMoreThanPermitted() throws Exception overallProgressReporter.requestRetry(A_POOL, A_TEST_CASE); } + @Test + public void requestRetryIsNotAllowedIfFailedMoreThanPermitterPerTest() { + fakeTestCasesAccumulator.thatAlwaysReturnsTestCaseCount(2); + overallProgressReporter = new OverallProgressReporter(3, 1, + aFakeProgressReporterTrackers().thatAlwaysReturns(mockPoolProgressTracker), + fakeTestCasesAccumulator); + + mockery.checking(new Expectations() {{ + never(mockPoolProgressTracker).trackTestEnqueuedAgain(); + }}); + + overallProgressReporter.requestRetry(A_POOL, A_TEST_CASE); + } } diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/ReporterBasedFailedTestSchedulerTest.java b/fork-runner/src/test/java/com/shazam/fork/runner/ReporterBasedFailedTestSchedulerTest.java new file mode 100644 index 00000000..0b428ae2 --- /dev/null +++ b/fork-runner/src/test/java/com/shazam/fork/runner/ReporterBasedFailedTestSchedulerTest.java @@ -0,0 +1,106 @@ +/* + * Copyright 2018 Shazam Entertainment Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. + * + * You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ + +package com.shazam.fork.runner; + +import com.shazam.fork.model.Pool; +import com.shazam.fork.model.TestCaseEvent; +import org.jmock.Expectations; +import org.jmock.auto.Mock; +import org.jmock.integration.junit4.JUnitRuleMockery; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; + +import java.util.LinkedList; +import java.util.Queue; + +import static com.shazam.fork.model.Pool.Builder.aDevicePool; +import static com.shazam.fork.model.TestCaseEvent.newTestCase; +import static com.shazam.fork.runner.FakeProgressReporter.fakeProgressReporter; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; + +public class ReporterBasedFailedTestSchedulerTest { + @Rule + public JUnitRuleMockery mockery = new JUnitRuleMockery(); + @Mock + private ProgressReporter mockProgressReporter; + + private final FakeProgressReporter fakeProgressReporter = fakeProgressReporter(); + + private Queue testsQueue; + private Pool pool = aDevicePool() + .withName("aName") + .build(); + + private ReporterBasedFailedTestScheduler testScheduler; + + @Before + public void setUp() { + testsQueue = new LinkedList<>(); + } + + @Test + public void recordsFailedTestCase() { + testScheduler = new ReporterBasedFailedTestScheduler(mockProgressReporter, pool, testsQueue); + TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); + + mockery.checking(new Expectations() {{ + oneOf(mockProgressReporter).recordFailedTestCase(pool, testCase); + + allowing(mockProgressReporter).requestRetry(pool, testCase); + }}); + + testScheduler.rescheduleTestExecution(testCase); + } + + @Test + public void addsTestCaseToQueueWhenRetryIsAllowed() { + fakeProgressReporter.thatAlwaysAllowTestToBeRescheduled(); + testScheduler = new ReporterBasedFailedTestScheduler(fakeProgressReporter, pool, testsQueue); + TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); + + testScheduler.rescheduleTestExecution(testCase); + + assertThat(testsQueue, contains(testCase)); + } + + @Test + public void returnsTrueWhenTestCaseIsRescheduled() { + fakeProgressReporter.thatAlwaysAllowTestToBeRescheduled(); + testScheduler = new ReporterBasedFailedTestScheduler(fakeProgressReporter, pool, testsQueue); + TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); + + assertThat(testScheduler.rescheduleTestExecution(testCase), equalTo(true)); + } + + @Test + public void doesNotAddTestCaseToQueueWhenRetryIsNotAllowed() { + fakeProgressReporter.thatAlwaysDisallowTestToBeRescheduled(); + testScheduler = new ReporterBasedFailedTestScheduler(fakeProgressReporter, pool, testsQueue); + TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); + + testScheduler.rescheduleTestExecution(testCase); + + assertThat(testsQueue, not(contains(testCase))); + } + + @Test + public void returnsFalseWhenTestCaseIsNotRescheduled() { + fakeProgressReporter.thatAlwaysDisallowTestToBeRescheduled(); + testScheduler = new ReporterBasedFailedTestScheduler(fakeProgressReporter, pool, testsQueue); + TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); + + assertThat(testScheduler.rescheduleTestExecution(testCase), equalTo(false)); + } +} \ No newline at end of file diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/listeners/RetryListenerTest.java b/fork-runner/src/test/java/com/shazam/fork/runner/listeners/RetryListenerTest.java index 7908debc..ad45a7d8 100644 --- a/fork-runner/src/test/java/com/shazam/fork/runner/listeners/RetryListenerTest.java +++ b/fork-runner/src/test/java/com/shazam/fork/runner/listeners/RetryListenerTest.java @@ -2,10 +2,11 @@ import com.android.ddmlib.testrunner.TestIdentifier; import com.shazam.fork.device.DeviceTestFilesCleaner; +import com.shazam.fork.device.FakeDeviceTestFilesCleaner; import com.shazam.fork.model.Device; import com.shazam.fork.model.Pool; import com.shazam.fork.model.TestCaseEvent; -import com.shazam.fork.runner.TestRetryer; +import com.shazam.fork.runner.FailedTestScheduler; import com.shazam.fork.util.TestPipelineEmulator; import org.jmock.Expectations; import org.jmock.auto.Mock; @@ -13,6 +14,7 @@ import org.junit.Rule; import org.junit.Test; +import static com.shazam.fork.device.FakeDeviceTestFilesCleaner.fakeDeviceTestFilesCleaner; import static com.shazam.fork.model.Device.Builder.aDevice; import static com.shazam.fork.model.Pool.Builder.aDevicePool; import static com.shazam.fork.model.TestCaseEvent.newTestCase; @@ -22,9 +24,11 @@ public class RetryListenerTest { @Rule public JUnitRuleMockery mockery = new JUnitRuleMockery(); @Mock - private TestRetryer testRetryer; + private FailedTestScheduler mockFailedTestScheduler; @Mock - private DeviceTestFilesCleaner deviceTestFilesCleaner; + private DeviceTestFilesCleaner mockDeviceTestFilesCleaner; + + private final FakeDeviceTestFilesCleaner fakeDeviceTestFilesCleaner = fakeDeviceTestFilesCleaner(); private final Device device = aDevice().build(); private final Pool pool = aDevicePool() @@ -38,17 +42,17 @@ public class RetryListenerTest { @Test public void reschedulesTestIfTestRunFailedAndDeleteTraceFiles() { RetryListener retryListener = - new RetryListener(pool, device, fatalCrashedTestCaseEvent, testRetryer, deviceTestFilesCleaner); + new RetryListener(pool, device, mockFailedTestScheduler, mockDeviceTestFilesCleaner); mockery.checking(new Expectations() {{ - oneOf(testRetryer).rescheduleTestExecution(fatalCrashedTest, fatalCrashedTestCaseEvent); + oneOf(mockFailedTestScheduler).rescheduleTestExecution(fatalCrashedTestCaseEvent); will(returnValue(true)); - oneOf(deviceTestFilesCleaner).deleteTraceFiles(fatalCrashedTest); + oneOf(mockDeviceTestFilesCleaner).deleteTraceFiles(fatalCrashedTestCaseEvent); }}); TestPipelineEmulator emulator = testPipelineEmulator() - .withFatalErrorMessage("fatal error") + .withTestRunFailed("fatal error") .build(); emulator.emulateFor(retryListener, fatalCrashedTest); } @@ -56,17 +60,33 @@ public void reschedulesTestIfTestRunFailedAndDeleteTraceFiles() { @Test public void doesNotDeleteTraceFilesIfCannotRescheduleTestAfterTestRunFailed() { RetryListener retryListener = - new RetryListener(pool, device, fatalCrashedTestCaseEvent, testRetryer, deviceTestFilesCleaner); + new RetryListener(pool, device, mockFailedTestScheduler, mockDeviceTestFilesCleaner); mockery.checking(new Expectations() {{ - oneOf(testRetryer).rescheduleTestExecution(fatalCrashedTest, fatalCrashedTestCaseEvent); + oneOf(mockFailedTestScheduler).rescheduleTestExecution(fatalCrashedTestCaseEvent); will(returnValue(false)); - never(deviceTestFilesCleaner).deleteTraceFiles(fatalCrashedTest); + never(mockDeviceTestFilesCleaner).deleteTraceFiles(fatalCrashedTestCaseEvent); + }}); + + TestPipelineEmulator emulator = testPipelineEmulator() + .withTestRunFailed("fatal error") + .build(); + emulator.emulateFor(retryListener, fatalCrashedTest); + } + + @Test + public void reschedulesTestWhenTestFailsAndThenTestRunCrashes() { + RetryListener retryListener = + new RetryListener(pool, device, mockFailedTestScheduler, fakeDeviceTestFilesCleaner); + + mockery.checking(new Expectations() {{ + oneOf(mockFailedTestScheduler).rescheduleTestExecution(fatalCrashedTestCaseEvent); }}); TestPipelineEmulator emulator = testPipelineEmulator() - .withFatalErrorMessage("fatal error") + .withTestFailed("assert exception") + .withTestRunFailed("fatal error") .build(); emulator.emulateFor(retryListener, fatalCrashedTest); } diff --git a/fork-runner/src/test/java/com/shazam/fork/summary/FakeDeviceTestFilesRetriever.java b/fork-runner/src/test/java/com/shazam/fork/summary/FakeDeviceTestFilesRetriever.java index 29f1948e..44de2392 100644 --- a/fork-runner/src/test/java/com/shazam/fork/summary/FakeDeviceTestFilesRetriever.java +++ b/fork-runner/src/test/java/com/shazam/fork/summary/FakeDeviceTestFilesRetriever.java @@ -14,7 +14,7 @@ public class FakeDeviceTestFilesRetriever implements DeviceTestFilesRetriever { private FakeDeviceTestFilesRetriever() { } - public static FakeDeviceTestFilesRetriever aFakeDeviceTestFilesRetriever() { + public static FakeDeviceTestFilesRetriever fakeDeviceTestFilesRetriever() { return new FakeDeviceTestFilesRetriever(); } diff --git a/fork-runner/src/test/java/com/shazam/fork/util/TestPipelineEmulator.java b/fork-runner/src/test/java/com/shazam/fork/util/TestPipelineEmulator.java index 392194ef..2db63a1e 100644 --- a/fork-runner/src/test/java/com/shazam/fork/util/TestPipelineEmulator.java +++ b/fork-runner/src/test/java/com/shazam/fork/util/TestPipelineEmulator.java @@ -35,12 +35,12 @@ public static Builder testPipelineEmulator() { return new Builder(); } - public Builder withTrace(String trace) { + public Builder withTestFailed(String trace) { this.trace = trace; return this; } - public Builder withFatalErrorMessage(String fatalErrorMessage) { + public Builder withTestRunFailed(String fatalErrorMessage) { this.fatalErrorMessage = fatalErrorMessage; return this; } From e8a0838c4a9e9f4d44394ff0edeaf95121d78883 Mon Sep 17 00:00:00 2001 From: Andrei Zaitcev Date: Wed, 1 Aug 2018 14:12:24 +0100 Subject: [PATCH 2/3] Repalces factories in TestCaseEvent with a builder pattern. --- .../com/shazam/fork/model/TestCaseEvent.java | 114 +++++++++++++----- .../shazam/fork/suite/TestSuiteLoader.java | 24 +++- .../fork/suite/TestSuiteLoaderTest.java | 54 ++++++--- .../listeners/ForkXmlTestRunListener.java | 3 +- .../fork/runner/listeners/RetryListener.java | 5 +- .../fork/system/io/ForkFileManager.java | 5 +- .../FilesRetrieverBasedAggregatorTest.java | 33 +++-- .../PoolTestCaseAccumulatorTestFailure.java | 39 +++--- .../runner/OverallProgressReporterTest.java | 8 +- .../ReporterBasedFailedTestSchedulerTest.java | 11 +- .../runner/listeners/RetryListenerTest.java | 3 +- 11 files changed, 197 insertions(+), 102 deletions(-) diff --git a/fork-common/src/main/java/com/shazam/fork/model/TestCaseEvent.java b/fork-common/src/main/java/com/shazam/fork/model/TestCaseEvent.java index a871eb59..28060d94 100644 --- a/fork-common/src/main/java/com/shazam/fork/model/TestCaseEvent.java +++ b/fork-common/src/main/java/com/shazam/fork/model/TestCaseEvent.java @@ -4,11 +4,14 @@ import com.google.common.base.Objects; import javax.annotation.Nonnull; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; -import static java.util.Collections.emptyList; -import static java.util.Collections.emptyMap; +import static com.shazam.fork.model.TestCaseEvent.Builder.testCaseEvent; +import static java.util.Collections.unmodifiableList; +import static java.util.Collections.unmodifiableMap; import static org.apache.commons.lang3.builder.ToStringBuilder.reflectionToString; import static org.apache.commons.lang3.builder.ToStringStyle.SIMPLE_STYLE; @@ -19,48 +22,34 @@ public class TestCaseEvent { private final List permissionsToRevoke; private final Map properties; - private TestCaseEvent(String testMethod, - String testClass, - boolean isIgnored, - List permissionsToRevoke, - Map properties) { - this.testMethod = testMethod; - this.testClass = testClass; - this.isIgnored = isIgnored; - this.permissionsToRevoke = permissionsToRevoke; - this.properties = properties; + private TestCaseEvent(Builder builder) { + this.testClass = builder.testClass; + this.testMethod = builder.testMethod; + this.isIgnored = builder.isIgnored; + this.permissionsToRevoke = builder.permissionsToRevoke; + this.properties = builder.properties; } @Nonnull - public static TestCaseEvent newTestCase(@Nonnull String testClass, @Nonnull String testMethod) { - return newTestCase(testMethod, testClass, false, emptyList(), emptyMap()); - } - - public static TestCaseEvent newTestCase(String testMethod, - String testClass, - boolean isIgnored, - List permissionsToRevoke, - Map properties) { - return new TestCaseEvent(testMethod, testClass, isIgnored, permissionsToRevoke, properties); - } - - public static TestCaseEvent newTestCase(@Nonnull TestIdentifier testIdentifier) { - return newTestCase(testIdentifier, false); - } - - public static TestCaseEvent newTestCase(@Nonnull TestIdentifier testIdentifier, boolean isIgnored) { - return new TestCaseEvent(testIdentifier.getTestName(), testIdentifier.getClassName(), isIgnored, - emptyList(), emptyMap()); + public static TestCaseEvent from(@Nonnull TestIdentifier testIdentifier) { + return testCaseEvent() + .withTestClass(testIdentifier.getClassName()) + .withTestMethod(testIdentifier.getTestName()) + .withIsIgnored(false) + .build(); } + @Nonnull public String getTestFullName() { return testClass + "#" + testMethod; } + @Nonnull public String getTestMethod() { return testMethod; } + @Nonnull public String getTestClass() { return testClass; } @@ -69,12 +58,14 @@ public boolean isIgnored() { return isIgnored; } + @Nonnull public List getPermissionsToRevoke() { - return permissionsToRevoke; + return unmodifiableList(permissionsToRevoke); } + @Nonnull public Map getProperties() { - return properties; + return unmodifiableMap(properties); } @Override @@ -99,4 +90,61 @@ public boolean equals(Object obj) { public String toString() { return reflectionToString(this, SIMPLE_STYLE); } + + public static class Builder { + private String testClass; + private String testMethod; + private boolean isIgnored; + private List permissionsToRevoke = new ArrayList<>(); + private Map properties = new HashMap<>(); + + @Nonnull + public static Builder testCaseEvent(@Nonnull TestIdentifier testIdentifier) { + return testCaseEvent() + .withTestClass(testIdentifier.getClassName()) + .withTestMethod(testIdentifier.getTestName()); + } + + @Nonnull + public static Builder testCaseEvent() { + return new Builder(); + } + + @Nonnull + public Builder withTestClass(@Nonnull String testClass) { + this.testClass = testClass; + return this; + } + + @Nonnull + public Builder withTestMethod(@Nonnull String testMethod) { + this.testMethod = testMethod; + return this; + } + + @Nonnull + public Builder withIsIgnored(boolean isIgnored) { + this.isIgnored = isIgnored; + return this; + } + + @Nonnull + public Builder withPermissionsToRevoke(@Nonnull List permissionsToRevoke) { + this.permissionsToRevoke.clear(); + this.permissionsToRevoke.addAll(permissionsToRevoke); + return this; + } + + @Nonnull + public Builder withProperties(@Nonnull Map properties) { + this.properties.clear(); + this.properties.putAll(properties); + return this; + } + + @Nonnull + public TestCaseEvent build() { + return new TestCaseEvent(this); + } + } } diff --git a/fork-common/src/main/java/com/shazam/fork/suite/TestSuiteLoader.java b/fork-common/src/main/java/com/shazam/fork/suite/TestSuiteLoader.java index 34a01db0..ce975dfc 100644 --- a/fork-common/src/main/java/com/shazam/fork/suite/TestSuiteLoader.java +++ b/fork-common/src/main/java/com/shazam/fork/suite/TestSuiteLoader.java @@ -12,17 +12,25 @@ import com.shazam.fork.io.DexFileExtractor; import com.shazam.fork.model.TestCaseEvent; -import org.jf.dexlib.*; +import org.jf.dexlib.AnnotationDirectoryItem; +import org.jf.dexlib.AnnotationItem; +import org.jf.dexlib.AnnotationSetItem; +import org.jf.dexlib.ClassDefItem; import org.jf.dexlib.EncodedValue.AnnotationEncodedSubValue; import org.jf.dexlib.EncodedValue.ArrayEncodedValue; import org.jf.dexlib.EncodedValue.EncodedValue; import org.jf.dexlib.EncodedValue.StringEncodedValue; +import org.jf.dexlib.StringIdItem; import javax.annotation.Nonnull; import java.io.File; -import java.util.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.List; +import java.util.Map; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; +import static com.shazam.fork.model.TestCaseEvent.Builder.testCaseEvent; import static java.lang.Math.min; import static java.util.Arrays.stream; import static java.util.Collections.emptyList; @@ -83,10 +91,16 @@ private TestCaseEvent convertToTestCaseEvent(ClassDefItem classDefItem, String testMethod = method.method.getMethodName().getStringValue(); AnnotationItem[] annotations = method.annotationSet.getAnnotations(); String testClass = getClassName(classDefItem); - boolean ignored = isClassIgnored(annotationDirectoryItem) || isMethodIgnored(annotations); + boolean isIgnored = isClassIgnored(annotationDirectoryItem) || isMethodIgnored(annotations); List permissionsToRevoke = getPermissionsToRevoke(annotations); Map properties = getTestProperties(annotations); - return newTestCase(testMethod, testClass, ignored, permissionsToRevoke, properties); + return testCaseEvent() + .withTestClass(testClass) + .withTestMethod(testMethod) + .withIsIgnored(isIgnored) + .withPermissionsToRevoke(permissionsToRevoke) + .withProperties(properties) + .build(); } private String getClassName(ClassDefItem classDefItem) { diff --git a/fork-common/src/test/java/com/shazam/fork/suite/TestSuiteLoaderTest.java b/fork-common/src/test/java/com/shazam/fork/suite/TestSuiteLoaderTest.java index 5f039f2c..a96bb7b2 100644 --- a/fork-common/src/test/java/com/shazam/fork/suite/TestSuiteLoaderTest.java +++ b/fork-common/src/test/java/com/shazam/fork/suite/TestSuiteLoaderTest.java @@ -26,44 +26,45 @@ import static com.shazam.fork.io.FakeDexFileExtractor.fakeDexFileExtractor; import static com.shazam.fork.io.Files.convertFileToDexFile; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; +import static com.shazam.fork.model.TestCaseEvent.Builder.testCaseEvent; import static com.shazam.fork.suite.FakeTestClassMatcher.fakeTestClassMatcher; import static com.shazam.shazamcrest.MatcherAssert.assertThat; import static com.shazam.shazamcrest.matcher.Matchers.sameBeanAs; import static java.util.Arrays.asList; -import static java.util.Collections.*; +import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonMap; import static org.hamcrest.Matchers.hasItems; /** * This test is based on the tests.dex file, which contains test classes with the following code: * 
- *{@literal}@Ignore
- *public class IgnoredClassTest {
+ * {@literal}@Ignore
+ * public class IgnoredClassTest {
  *    {@literal}@Test
  *    public void methodOfAnIgnoredTestClass() {
  *    }
- *}
- *
- *public class ClassWithNoIgnoredMethodsTest {
+ * }
+ * 

+ * public class ClassWithNoIgnoredMethodsTest {
  *    {@literal}@Test
  *    public void firstTestMethod() {
  *    }
- *
+ * 

  *    {@literal}@Test
  *    public void secondTestMethod() {
  *    }
- *}
- *
- *public class ClassWithSomeIgnoredMethodsTest {
+ * }
+ * 

+ * public class ClassWithSomeIgnoredMethodsTest {
  *    {@literal}@Test
  *    public void nonIgnoredTestMethod() {
  *    }
- *
+ * 

  *    {@literal}@Test
  *    {@literal}@Ignore
  *    public void ignoredTestMethod() {
  *    }
- *}
+ * }
  *
*/ public class TestSuiteLoaderTest { @@ -81,7 +82,7 @@ private DexFile testDexFile() { } @Before - public void setUp() throws Exception { + public void setUp() { testSuiteLoader = new TestSuiteLoader(ANY_INSTRUMENTATION_APK_FILE, fakeDexFileExtractor, fakeTestClassMatcher); } @@ -125,16 +126,35 @@ public void populatesTestProperties() throws Exception { @Nonnull private Matcher sameTestEventAs(String testMethod, String testClass, Map properties) { - return sameBeanAs(newTestCase(testMethod, testClass, false, emptyList(), properties)); + return sameBeanAs( + testCaseEvent() + .withTestClass(testClass) + .withTestMethod(testMethod) + .withProperties(properties) + .build() + ); } @Nonnull private Matcher sameTestEventAs(String testMethod, String testClass, boolean isIgnored) { - return sameBeanAs(newTestCase(testMethod, testClass, isIgnored, emptyList(), emptyMap())); + return sameBeanAs( + testCaseEvent() + .withTestClass(testClass) + .withTestMethod(testMethod) + .withIsIgnored(isIgnored) + .build() + ); } @Nonnull private Matcher sameTestEventAs(String testMethod, String testClass, boolean isIgnored, List permissions) { - return sameBeanAs(newTestCase(testMethod, testClass, isIgnored, permissions, emptyMap())); + return sameBeanAs( + testCaseEvent() + .withTestClass(testClass) + .withTestMethod(testMethod) + .withIsIgnored(isIgnored) + .withPermissionsToRevoke(permissions) + .build() + ); } } diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/ForkXmlTestRunListener.java b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/ForkXmlTestRunListener.java index b5a6d092..953319ef 100755 --- a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/ForkXmlTestRunListener.java +++ b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/ForkXmlTestRunListener.java @@ -27,7 +27,6 @@ import javax.annotation.Nonnull; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; import static com.shazam.fork.summary.TestResult.SUMMARY_KEY_TOTAL_FAILURE_COUNT; public class ForkXmlTestRunListener extends XmlTestRunListener { @@ -69,7 +68,7 @@ protected Map getPropertiesAttributes() { ImmutableMap.Builder mapBuilder = ImmutableMap.builder() .putAll(super.getPropertiesAttributes()); if (test != null) { - int testFailuresCount = progressReporter.getTestFailuresCount(pool, newTestCase(test)); + int testFailuresCount = progressReporter.getTestFailuresCount(pool, TestCaseEvent.from(test)); if (testFailuresCount > 0) { mapBuilder.put(SUMMARY_KEY_TOTAL_FAILURE_COUNT, Integer.toString(testFailuresCount)); } diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/RetryListener.java b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/RetryListener.java index f1f87234..ad456203 100644 --- a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/RetryListener.java +++ b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/RetryListener.java @@ -22,7 +22,6 @@ import java.util.Map; import static com.google.common.base.Preconditions.checkNotNull; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; public class RetryListener extends NoOpITestRunListener { private static final Logger logger = LoggerFactory.getLogger(RetryListener.class); @@ -47,12 +46,12 @@ public RetryListener(Pool pool, @Override public void testStarted(TestIdentifier test) { - startedTest = newTestCase(test); + startedTest = TestCaseEvent.from(test); } @Override public void testFailed(TestIdentifier test, String trace) { - failedTest = newTestCase(test); + failedTest = TestCaseEvent.from(test); } @Override diff --git a/fork-runner/src/main/java/com/shazam/fork/system/io/ForkFileManager.java b/fork-runner/src/main/java/com/shazam/fork/system/io/ForkFileManager.java index 9dc72fad..9885f489 100644 --- a/fork-runner/src/main/java/com/shazam/fork/system/io/ForkFileManager.java +++ b/fork-runner/src/main/java/com/shazam/fork/system/io/ForkFileManager.java @@ -15,7 +15,6 @@ import java.nio.file.Path; import static com.shazam.fork.CommonDefaults.FORK_SUMMARY_FILENAME_FORMAT; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; import static com.shazam.fork.system.io.FileType.TEST; import static java.nio.file.Files.createDirectories; import static java.nio.file.Paths.get; @@ -53,7 +52,7 @@ public File createFile(FileType fileType, Pool pool, Device device, TestIdentifi public File createFile(FileType fileType, Pool pool, Device device, TestIdentifier testIdentifier) { try { Path directory = createDirectory(fileType, pool, device); - String filename = createFilenameForTest(newTestCase(testIdentifier), fileType); + String filename = createFilenameForTest(TestCaseEvent.from(testIdentifier), fileType); return createFile(directory, filename); } catch (IOException e) { throw new CouldNotCreateDirectoryException(e); @@ -83,7 +82,7 @@ public File[] getFiles(FileType fileType, Pool pool, Device device, TestIdentifi @Override public File getFile(FileType fileType, String pool, String safeSerial, TestIdentifier testIdentifier) { - String filenameForTest = createFilenameForTest(newTestCase(testIdentifier), fileType); + String filenameForTest = createFilenameForTest(TestCaseEvent.from(testIdentifier), fileType); Path path = get(output.getAbsolutePath(), fileType.getDirectory(), pool, safeSerial, filenameForTest); return path.toFile(); } diff --git a/fork-runner/src/test/java/com/shazam/fork/aggregator/FilesRetrieverBasedAggregatorTest.java b/fork-runner/src/test/java/com/shazam/fork/aggregator/FilesRetrieverBasedAggregatorTest.java index f755fa7b..82e509ea 100644 --- a/fork-runner/src/test/java/com/shazam/fork/aggregator/FilesRetrieverBasedAggregatorTest.java +++ b/fork-runner/src/test/java/com/shazam/fork/aggregator/FilesRetrieverBasedAggregatorTest.java @@ -1,6 +1,5 @@ package com.shazam.fork.aggregator; -import com.android.ddmlib.testrunner.TestIdentifier; import com.shazam.fork.model.Device; import com.shazam.fork.model.Pool; import com.shazam.fork.model.TestCaseEvent; @@ -15,11 +14,10 @@ import static com.google.common.collect.Lists.newArrayList; import static com.shazam.fork.model.Device.Builder.aDevice; import static com.shazam.fork.model.Pool.Builder.aDevicePool; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; +import static com.shazam.fork.model.TestCaseEvent.Builder.testCaseEvent; import static com.shazam.fork.summary.FakeDeviceTestFilesRetriever.fakeDeviceTestFilesRetriever; import static com.shazam.fork.summary.TestResult.Builder.aTestResult; import static com.shazam.fork.summary.TestResult.SUMMARY_KEY_TOTAL_FAILURE_COUNT; -import static java.util.Collections.emptyList; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.hasItem; @@ -77,12 +75,29 @@ public class FilesRetrieverBasedAggregatorTest { ); private final Collection testCaseEvents = newArrayList( - newTestCase(new TestIdentifier("com.example.CompletedClassTest", "doesJobProperly")), - newTestCase(new TestIdentifier("com.example.CompletedClassTest2", "doesJobProperly")), - newTestCase("doesJobProperly", "com.example.FailedClassTest", false, - emptyList(), TEST_METRICS_FOR_FAILED_TEST), - newTestCase(new TestIdentifier("com.example.IgnoredClassTest", "doesJobProperly"), true), - newTestCase(new TestIdentifier("com.example.FatalCrashedTest", "doesJobProperly")) + testCaseEvent() + .withTestClass("com.example.CompletedClassTest") + .withTestMethod("doesJobProperly") + .build(), + testCaseEvent() + .withTestClass("com.example.CompletedClassTest2") + .withTestMethod("doesJobProperly") + .build(), + testCaseEvent() + .withTestClass("com.example.FailedClassTest") + .withTestMethod("doesJobProperly") + .withProperties(TEST_METRICS_FOR_FAILED_TEST) + .withIsIgnored(false) + .build(), + testCaseEvent() + .withTestClass("com.example.IgnoredClassTest") + .withTestMethod("doesJobProperly") + .withIsIgnored(true) + .build(), + testCaseEvent() + .withTestClass("com.example.FatalCrashedTest") + .withTestMethod("doesJobProperly") + .build() ); private final FakeDeviceTestFilesRetriever fakeDeviceTestFilesRetriever = fakeDeviceTestFilesRetriever(); diff --git a/fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseAccumulatorTestFailure.java b/fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseAccumulatorTestFailure.java index 9369a33c..98adb503 100644 --- a/fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseAccumulatorTestFailure.java +++ b/fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseAccumulatorTestFailure.java @@ -4,14 +4,12 @@ import org.junit.Test; import static com.shazam.fork.model.Device.Builder.aDevice; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; -import static java.util.Collections.emptyList; -import static java.util.Collections.emptyMap; +import static com.shazam.fork.model.Pool.Builder.aDevicePool; +import static com.shazam.fork.model.TestCaseEvent.Builder.testCaseEvent; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.MatcherAssert.assertThat; public class PoolTestCaseAccumulatorTestFailure { - private final Device A_DEVICE = aDevice() .withSerial("a_device") .build(); @@ -19,29 +17,33 @@ public class PoolTestCaseAccumulatorTestFailure { .withSerial("another_device") .build(); - private final Pool A_POOL = Pool.Builder.aDevicePool() + private final Pool A_POOL = aDevicePool() .withName("a_pool") .addDevice(A_DEVICE) .build(); - - private final Pool ANOTHER_POOL = Pool.Builder.aDevicePool() + private final Pool ANOTHER_POOL = aDevicePool() .withName("another_pool") .addDevice(ANOTHER_DEVICE) .build(); - - private final TestCaseEvent A_TEST_CASE = newTestCase("a_method", "a_class", false, emptyList(), emptyMap()); - private final TestCaseEvent ANOTHER_TEST_CASE = newTestCase("another_method", "a_class", false, emptyList(), emptyMap()); - PoolTestCaseFailureAccumulator subject; + private final TestCaseEvent A_TEST_CASE = testCaseEvent() + .withTestClass("a_class") + .withTestMethod("a_method") + .build(); + private final TestCaseEvent ANOTHER_TEST_CASE = testCaseEvent() + .withTestClass("a_class") + .withTestMethod("another_method") + .build(); + + private PoolTestCaseFailureAccumulator subject; @Before - public void setUp() throws Exception { + public void setUp() { subject = new PoolTestCaseFailureAccumulator(); } @Test - public void shouldAggregateCountForSameTestCaseAcrossMultipleDevices() throws Exception { - + public void shouldAggregateCountForSameTestCaseAcrossMultipleDevices() { subject.record(A_POOL, A_TEST_CASE); subject.record(A_POOL, A_TEST_CASE); @@ -51,7 +53,7 @@ public void shouldAggregateCountForSameTestCaseAcrossMultipleDevices() throws Ex } @Test - public void shouldCountTestsPerPool() throws Exception { + public void shouldCountTestsPerPool() { subject.record(A_POOL, A_TEST_CASE); subject.record(A_POOL, A_TEST_CASE); @@ -61,8 +63,7 @@ public void shouldCountTestsPerPool() throws Exception { } @Test - public void shouldAggregateCountForSameTestCaseAcrossMultiplePools() throws Exception { - + public void shouldAggregateCountForSameTestCaseAcrossMultiplePools() { subject.record(A_POOL, A_TEST_CASE); subject.record(ANOTHER_POOL, A_TEST_CASE); @@ -72,7 +73,7 @@ public void shouldAggregateCountForSameTestCaseAcrossMultiplePools() throws Exce } @Test - public void shouldNotReturnTestCasesForDifferentPool() throws Exception { + public void shouldNotReturnTestCasesForDifferentPool() { subject.record(A_POOL, A_TEST_CASE); int actualCountForAnotherDevice = subject.getCount(ANOTHER_POOL, A_TEST_CASE); @@ -81,7 +82,7 @@ public void shouldNotReturnTestCasesForDifferentPool() throws Exception { } @Test - public void shouldAccumulateDifferentTestCasesForSamePool() throws Exception { + public void shouldAccumulateDifferentTestCasesForSamePool() { subject.record(A_POOL, A_TEST_CASE); subject.record(A_POOL, ANOTHER_TEST_CASE); diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/OverallProgressReporterTest.java b/fork-runner/src/test/java/com/shazam/fork/runner/OverallProgressReporterTest.java index 9d588070..be6778a4 100644 --- a/fork-runner/src/test/java/com/shazam/fork/runner/OverallProgressReporterTest.java +++ b/fork-runner/src/test/java/com/shazam/fork/runner/OverallProgressReporterTest.java @@ -11,7 +11,7 @@ import static com.shazam.fork.model.Device.Builder.aDevice; import static com.shazam.fork.model.Pool.Builder.aDevicePool; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; +import static com.shazam.fork.model.TestCaseEvent.Builder.testCaseEvent; import static com.shazam.fork.runner.FakePoolTestCaseAccumulator.fakePoolTestCaseAccumulator; import static com.shazam.fork.runner.FakeProgressReporterTrackers.aFakeProgressReporterTrackers; @@ -26,8 +26,10 @@ public class OverallProgressReporterTest { private final Pool A_POOL = aDevicePool() .addDevice(A_DEVICE) .build(); - private final TestCaseEvent A_TEST_CASE = - newTestCase("aTestClass", "aTestMethod"); + private final TestCaseEvent A_TEST_CASE = testCaseEvent() + .withTestClass("aTestClass") + .withTestMethod("aTestMethod") + .build(); private OverallProgressReporter overallProgressReporter; diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/ReporterBasedFailedTestSchedulerTest.java b/fork-runner/src/test/java/com/shazam/fork/runner/ReporterBasedFailedTestSchedulerTest.java index 0b428ae2..16bd248a 100644 --- a/fork-runner/src/test/java/com/shazam/fork/runner/ReporterBasedFailedTestSchedulerTest.java +++ b/fork-runner/src/test/java/com/shazam/fork/runner/ReporterBasedFailedTestSchedulerTest.java @@ -23,7 +23,7 @@ import java.util.Queue; import static com.shazam.fork.model.Pool.Builder.aDevicePool; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; +import static com.shazam.fork.model.TestCaseEvent.Builder.testCaseEvent; import static com.shazam.fork.runner.FakeProgressReporter.fakeProgressReporter; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; @@ -44,6 +44,10 @@ public class ReporterBasedFailedTestSchedulerTest { .build(); private ReporterBasedFailedTestScheduler testScheduler; + private TestCaseEvent testCase = testCaseEvent() + .withTestClass("com.shazam.fork.TestClass") + .withTestMethod("testMethod") + .build(); @Before public void setUp() { @@ -53,7 +57,6 @@ public void setUp() { @Test public void recordsFailedTestCase() { testScheduler = new ReporterBasedFailedTestScheduler(mockProgressReporter, pool, testsQueue); - TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); mockery.checking(new Expectations() {{ oneOf(mockProgressReporter).recordFailedTestCase(pool, testCase); @@ -68,7 +71,6 @@ public void recordsFailedTestCase() { public void addsTestCaseToQueueWhenRetryIsAllowed() { fakeProgressReporter.thatAlwaysAllowTestToBeRescheduled(); testScheduler = new ReporterBasedFailedTestScheduler(fakeProgressReporter, pool, testsQueue); - TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); testScheduler.rescheduleTestExecution(testCase); @@ -79,7 +81,6 @@ public void addsTestCaseToQueueWhenRetryIsAllowed() { public void returnsTrueWhenTestCaseIsRescheduled() { fakeProgressReporter.thatAlwaysAllowTestToBeRescheduled(); testScheduler = new ReporterBasedFailedTestScheduler(fakeProgressReporter, pool, testsQueue); - TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); assertThat(testScheduler.rescheduleTestExecution(testCase), equalTo(true)); } @@ -88,7 +89,6 @@ public void returnsTrueWhenTestCaseIsRescheduled() { public void doesNotAddTestCaseToQueueWhenRetryIsNotAllowed() { fakeProgressReporter.thatAlwaysDisallowTestToBeRescheduled(); testScheduler = new ReporterBasedFailedTestScheduler(fakeProgressReporter, pool, testsQueue); - TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); testScheduler.rescheduleTestExecution(testCase); @@ -99,7 +99,6 @@ public void doesNotAddTestCaseToQueueWhenRetryIsNotAllowed() { public void returnsFalseWhenTestCaseIsNotRescheduled() { fakeProgressReporter.thatAlwaysDisallowTestToBeRescheduled(); testScheduler = new ReporterBasedFailedTestScheduler(fakeProgressReporter, pool, testsQueue); - TestCaseEvent testCase = newTestCase("com.shazam.fork.TestClass", "testMethod"); assertThat(testScheduler.rescheduleTestExecution(testCase), equalTo(false)); } diff --git a/fork-runner/src/test/java/com/shazam/fork/runner/listeners/RetryListenerTest.java b/fork-runner/src/test/java/com/shazam/fork/runner/listeners/RetryListenerTest.java index ad45a7d8..c917cd6b 100644 --- a/fork-runner/src/test/java/com/shazam/fork/runner/listeners/RetryListenerTest.java +++ b/fork-runner/src/test/java/com/shazam/fork/runner/listeners/RetryListenerTest.java @@ -17,7 +17,6 @@ import static com.shazam.fork.device.FakeDeviceTestFilesCleaner.fakeDeviceTestFilesCleaner; import static com.shazam.fork.model.Device.Builder.aDevice; import static com.shazam.fork.model.Pool.Builder.aDevicePool; -import static com.shazam.fork.model.TestCaseEvent.newTestCase; import static com.shazam.fork.util.TestPipelineEmulator.Builder.testPipelineEmulator; public class RetryListenerTest { @@ -37,7 +36,7 @@ public class RetryListenerTest { .build(); private final TestIdentifier fatalCrashedTest = new TestIdentifier("com.example.FatalCrashedTest", "testMethod"); - private final TestCaseEvent fatalCrashedTestCaseEvent = newTestCase(fatalCrashedTest); + private final TestCaseEvent fatalCrashedTestCaseEvent = TestCaseEvent.from(fatalCrashedTest); @Test public void reschedulesTestIfTestRunFailedAndDeleteTraceFiles() { From 476e39d4d454c1caa68436d41450905d484975fd Mon Sep 17 00:00:00 2001 From: Andrei Zaitcev Date: Wed, 1 Aug 2018 14:37:24 +0100 Subject: [PATCH 3/3] Updates a counter when a test run fails --- .../model/PoolTestCaseFailureAccumulator.java | 66 +++++++++---------- .../listeners/ProgressTestRunListener.java | 2 +- ...> PoolTestCaseFailureAccumulatorTest.java} | 2 +- 3 files changed, 34 insertions(+), 36 deletions(-) rename fork-runner/src/test/java/com/shazam/fork/model/{PoolTestCaseAccumulatorTestFailure.java => PoolTestCaseFailureAccumulatorTest.java} (98%) diff --git a/fork-runner/src/main/java/com/shazam/fork/model/PoolTestCaseFailureAccumulator.java b/fork-runner/src/main/java/com/shazam/fork/model/PoolTestCaseFailureAccumulator.java index e7aa6db2..e0489a48 100644 --- a/fork-runner/src/main/java/com/shazam/fork/model/PoolTestCaseFailureAccumulator.java +++ b/fork-runner/src/main/java/com/shazam/fork/model/PoolTestCaseFailureAccumulator.java @@ -10,43 +10,48 @@ package com.shazam.fork.model; -import com.google.common.base.Predicate; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Multimaps; import com.google.common.collect.SetMultimap; -import static com.google.common.collect.FluentIterable.from; +import java.util.function.Predicate; + import static com.google.common.collect.HashMultimap.create; /** * Class that keeps track of the number of times each testCase is executed for device. */ public class PoolTestCaseFailureAccumulator implements PoolTestCaseAccumulator { - private SetMultimap map = Multimaps.synchronizedSetMultimap(create()); + private final SetMultimap testCaseCounters = create(); @Override public void record(Pool pool, TestCaseEvent testCaseEvent) { - if (!map.containsKey(pool)) { - map.put(pool, createNew(testCaseEvent)); - } + synchronized (testCaseCounters) { + if (!testCaseCounters.containsKey(pool)) { + testCaseCounters.put(pool, createNew(testCaseEvent)); + } - if (!from(map.get(pool)).anyMatch(isSameTestCase(testCaseEvent))) { - map.get(pool).add( - createNew(testCaseEvent) - .withIncreasedCount()); - } else { - from(map.get(pool)) - .firstMatch(isSameTestCase(testCaseEvent)).get() - .increaseCount(); + boolean hasCountedBefore = testCaseCounters.get(pool).stream() + .anyMatch(isSameTestCase(testCaseEvent)); + if (hasCountedBefore) { + testCaseCounters.get(pool).stream() + .filter(isSameTestCase(testCaseEvent)) + .findFirst() + .ifPresent(TestCaseEventCounter::increaseCount); + } else { + testCaseCounters.get(pool).add(createNew(testCaseEvent).withIncreasedCount()); + } } } @Override public int getCount(Pool pool, TestCaseEvent testCaseEvent) { - if (map.containsKey(pool)) { - return from(map.get(pool)) - .firstMatch(isSameTestCase(testCaseEvent)).or(TestCaseEventCounter.EMPTY) - .getCount(); + if (testCaseCounters.containsKey(pool)) { + synchronized (testCaseCounters) { + return testCaseCounters.get(pool).stream() + .filter(isSameTestCase(testCaseEvent)) + .findFirst() + .orElse(TestCaseEventCounter.EMPTY) + .getCount(); + } } else { return 0; } @@ -54,26 +59,19 @@ public int getCount(Pool pool, TestCaseEvent testCaseEvent) { @Override public int getCount(TestCaseEvent testCaseEvent) { - int result = 0; - ImmutableList counters = from(map.values()) - .filter(isSameTestCase(testCaseEvent)).toList(); - for (TestCaseEventCounter counter : counters) { - result += counter.getCount(); + synchronized (testCaseCounters) { + return testCaseCounters.values().stream() + .filter(isSameTestCase(testCaseEvent)) + .mapToInt(TestCaseEventCounter::getCount) + .sum(); } - return result; } private static TestCaseEventCounter createNew(final TestCaseEvent testCaseEvent) { return new TestCaseEventCounter(testCaseEvent, 0); } - private static Predicate isSameTestCase(final TestCaseEvent testCaseEvent) { - return new Predicate() { - @Override - public boolean apply(TestCaseEventCounter input) { - return input.getTestCaseEvent() != null - && testCaseEvent.equals(input.getTestCaseEvent()); - } - }; + private static Predicate isSameTestCase(TestCaseEvent testCaseEvent) { + return testCaseEventCounter -> testCaseEventCounter.getTestCaseEvent().equals(testCaseEvent); } } diff --git a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/ProgressTestRunListener.java b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/ProgressTestRunListener.java index 0069e83c..984f15dd 100644 --- a/fork-runner/src/main/java/com/shazam/fork/runner/listeners/ProgressTestRunListener.java +++ b/fork-runner/src/main/java/com/shazam/fork/runner/listeners/ProgressTestRunListener.java @@ -58,7 +58,7 @@ public void testEnded(TestIdentifier test, Map testMetrics) { @Override public void testRunFailed(String errorMessage) { - + poolProgressTracker.failedTest(); } @Override diff --git a/fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseAccumulatorTestFailure.java b/fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseFailureAccumulatorTest.java similarity index 98% rename from fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseAccumulatorTestFailure.java rename to fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseFailureAccumulatorTest.java index 98adb503..573ba1cd 100644 --- a/fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseAccumulatorTestFailure.java +++ b/fork-runner/src/test/java/com/shazam/fork/model/PoolTestCaseFailureAccumulatorTest.java @@ -9,7 +9,7 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.MatcherAssert.assertThat; -public class PoolTestCaseAccumulatorTestFailure { +public class PoolTestCaseFailureAccumulatorTest { private final Device A_DEVICE = aDevice() .withSerial("a_device") .build();