From 7f1c5ff1fba61aade5390d109c3be63716614981 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Rowicki?= Date: Wed, 26 May 2021 22:07:44 +0200 Subject: [PATCH] Retry new or modified tests based on git diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The best time to resolve flaky test is once you wrote that test. This is how Google looks for flakes in their monorepo. At that moment you have the best knowledge to solve such tests. Otherwise after month of running such test the cognitive load to fix it is very high. This PR adds a way to discover all changed test classes with diff to master branch and retry them until the maxRetry or to the first fail execution. Signed-off-by: Michał Rowicki --- plugin/build.gradle.kts | 2 + .../testretry/TestRetryTaskExtension.java | 16 + .../config/DefaultTestRetryTaskExtension.java | 7 + .../config/TestRetryTaskExtensionAdapter.java | 6 + .../internal/config/TestTaskConfigurer.java | 13 +- .../internal/executer/GitRepository.java | 103 +++++++ .../internal/executer/RetryTestExecuter.java | 9 +- .../executer/RetryTestResultProcessor.java | 13 +- .../testretry/ModifiedGroovyFuncTest.groovy | 279 ++++++++++++++++++ 9 files changed, 439 insertions(+), 9 deletions(-) create mode 100644 plugin/src/main/java/org/gradle/testretry/internal/executer/GitRepository.java create mode 100644 plugin/src/test/groovy/org/gradle/testretry/ModifiedGroovyFuncTest.groovy diff --git a/plugin/build.gradle.kts b/plugin/build.gradle.kts index 326adaa0..0c831101 100644 --- a/plugin/build.gradle.kts +++ b/plugin/build.gradle.kts @@ -34,12 +34,14 @@ configurations.getByName("compileOnly").extendsFrom(plugin) dependencies { plugin("org.ow2.asm:asm:9.0") + plugin("org.eclipse.jgit:org.eclipse.jgit:5.11.1.202105131744-r") testImplementation(gradleTestKit()) testImplementation(localGroovy()) testImplementation("org.spockframework:spock-core:1.3-groovy-2.5") testImplementation("net.sourceforge.nekohtml:nekohtml:1.9.22") testImplementation("org.ow2.asm:asm:8.0.1") + testImplementation("org.eclipse.jgit:org.eclipse.jgit:5.11.1.202105131744-r") codenarc("org.codenarc:CodeNarc:1.0") } diff --git a/plugin/src/main/java/org/gradle/testretry/TestRetryTaskExtension.java b/plugin/src/main/java/org/gradle/testretry/TestRetryTaskExtension.java index 46acdaf6..4dc9e229 100644 --- a/plugin/src/main/java/org/gradle/testretry/TestRetryTaskExtension.java +++ b/plugin/src/main/java/org/gradle/testretry/TestRetryTaskExtension.java @@ -69,6 +69,22 @@ public interface TestRetryTaskExtension { */ Property getMaxFailures(); + /** + * Whether tests that has been modified should be retried. + *

+ * Discovers modification of test by git diff with local master branch. + * All test cases in modified class are rerun until rich maxRetries or + * to the first fail execution. + *

+ * This setting defaults to {@code false}, + * which disables that feature. + *

+ * This setting has no effect if gradle isn't run in git repository context. + * + * @return whether modified/new tests should be retried + */ + Property getModifiedTestRetry(); + /** * The filter for specifying which tests may be retried. */ diff --git a/plugin/src/main/java/org/gradle/testretry/internal/config/DefaultTestRetryTaskExtension.java b/plugin/src/main/java/org/gradle/testretry/internal/config/DefaultTestRetryTaskExtension.java index 3dc0eef8..f5202b0f 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/config/DefaultTestRetryTaskExtension.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/config/DefaultTestRetryTaskExtension.java @@ -26,6 +26,7 @@ public class DefaultTestRetryTaskExtension implements TestRetryTaskExtension { private final Property failOnPassedAfterRetry; + private final Property modifiedTestRetry; private final Property maxRetries; private final Property maxFailures; private final Filter filter; @@ -33,6 +34,7 @@ public class DefaultTestRetryTaskExtension implements TestRetryTaskExtension { @Inject public DefaultTestRetryTaskExtension(ObjectFactory objects) { this.failOnPassedAfterRetry = objects.property(Boolean.class); + this.modifiedTestRetry = objects.property(Boolean.class); this.maxRetries = objects.property(Integer.class); this.maxFailures = objects.property(Integer.class); this.filter = new FilterImpl(objects); @@ -42,6 +44,11 @@ public Property getFailOnPassedAfterRetry() { return failOnPassedAfterRetry; } + @Override + public Property getModifiedTestRetry() { + return modifiedTestRetry; + } + public Property getMaxRetries() { return maxRetries; } diff --git a/plugin/src/main/java/org/gradle/testretry/internal/config/TestRetryTaskExtensionAdapter.java b/plugin/src/main/java/org/gradle/testretry/internal/config/TestRetryTaskExtensionAdapter.java index c3976c8a..6fe3228a 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/config/TestRetryTaskExtensionAdapter.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/config/TestRetryTaskExtensionAdapter.java @@ -34,6 +34,7 @@ public final class TestRetryTaskExtensionAdapter { private static final int DEFAULT_MAX_RETRIES = 0; private static final int DEFAULT_MAX_FAILURES = 0; private static final boolean DEFAULT_FAIL_ON_PASSED_AFTER_RETRY = false; + private static final boolean DEFAULT_MODIFIED_TEST_RETRY = false; private final ProviderFactory providerFactory; private final TestRetryTaskExtension extension; @@ -63,6 +64,7 @@ private void initialize(TestRetryTaskExtension extension, boolean gradle51OrLate if (gradle51OrLater) { extension.getMaxRetries().convention(DEFAULT_MAX_RETRIES); extension.getMaxFailures().convention(DEFAULT_MAX_FAILURES); + extension.getModifiedTestRetry().convention(DEFAULT_MODIFIED_TEST_RETRY); extension.getFailOnPassedAfterRetry().convention(DEFAULT_FAIL_ON_PASSED_AFTER_RETRY); extension.getFilter().getIncludeClasses().convention(Collections.emptySet()); extension.getFilter().getIncludeAnnotationClasses().convention(Collections.emptySet()); @@ -103,6 +105,10 @@ public int getMaxFailures() { return read(extension.getMaxFailures(), DEFAULT_MAX_FAILURES); } + public boolean getModifiedTestRetry() { + return read(extension.getModifiedTestRetry(), DEFAULT_MODIFIED_TEST_RETRY); + } + public Set getIncludeClasses() { return read(extension.getFilter().getIncludeClasses(), Collections.emptySet()); } diff --git a/plugin/src/main/java/org/gradle/testretry/internal/config/TestTaskConfigurer.java b/plugin/src/main/java/org/gradle/testretry/internal/config/TestTaskConfigurer.java index 81cc5b02..c6f8d87b 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/config/TestTaskConfigurer.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/config/TestTaskConfigurer.java @@ -29,6 +29,7 @@ import org.gradle.util.VersionNumber; import org.jetbrains.annotations.NotNull; +import java.io.File; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -47,14 +48,14 @@ public static void configureTestTask(Test test, ObjectFactory objectFactory, Pro test.getInputs().property("retry.failOnPassedAfterRetry", adapter.getFailOnPassedAfterRetryInput()); test.getExtensions().add(TestRetryTaskExtension.class, TestRetryTaskExtension.NAME, extension); - test.doFirst(new ConditionalTaskAction(new InitTaskAction(adapter, objectFactory))); + test.doFirst(new ConditionalTaskAction(new InitTaskAction(adapter, objectFactory, test.getProject().getRootDir()))); test.doLast(new ConditionalTaskAction(new FinalizeTaskAction())); } - private static RetryTestExecuter createRetryTestExecuter(Test task, TestRetryTaskExtensionAdapter extension, ObjectFactory objectFactory) { + private static RetryTestExecuter createRetryTestExecuter(Test task, TestRetryTaskExtensionAdapter extension, ObjectFactory objectFactory, File rootDir) { TestExecuter delegate = getTestExecuter(task); Instantiator instantiator = invoke(declaredMethod(AbstractTestTask.class, "getInstantiator"), task); - return new RetryTestExecuter(task, extension, delegate, instantiator, objectFactory); + return new RetryTestExecuter(task, extension, delegate, instantiator, objectFactory, rootDir); } private static TestExecuter getTestExecuter(Test task) { @@ -117,15 +118,17 @@ private static class InitTaskAction implements Action { private final TestRetryTaskExtensionAdapter adapter; private final ObjectFactory objectFactory; + private final File rootDir; - public InitTaskAction(TestRetryTaskExtensionAdapter adapter, ObjectFactory objectFactory) { + public InitTaskAction(TestRetryTaskExtensionAdapter adapter, ObjectFactory objectFactory, File rootDir) { this.adapter = adapter; this.objectFactory = objectFactory; + this.rootDir = rootDir; } @Override public void execute(@NotNull Test task) { - RetryTestExecuter retryTestExecuter = createRetryTestExecuter(task, adapter, objectFactory); + RetryTestExecuter retryTestExecuter = createRetryTestExecuter(task, adapter, objectFactory, rootDir); setTestExecuter(task, retryTestExecuter); } } diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/GitRepository.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/GitRepository.java new file mode 100644 index 00000000..fa3987ef --- /dev/null +++ b/plugin/src/main/java/org/gradle/testretry/internal/executer/GitRepository.java @@ -0,0 +1,103 @@ +/* + * Copyright 2019 the original author or authors. + * + * 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 org.gradle.testretry.internal.executer; + +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.diff.DiffEntry; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.treewalk.AbstractTreeIterator; +import org.eclipse.jgit.treewalk.CanonicalTreeParser; +import org.gradle.testretry.internal.config.TestRetryTaskExtensionAdapter; + +import java.io.File; +import java.io.IOException; +import java.util.List; + +import static java.util.stream.Collectors.toList; + +public class GitRepository { + + private final List modifiedFilesPaths; + + private GitRepository(List modifiedFilesPaths) { + this.modifiedFilesPaths = modifiedFilesPaths; + } + + public boolean wasModified(String path) { + return modifiedFilesPaths.contains(path); + } + + static class NoRepository extends GitRepository { + private NoRepository() { + super(null); + } + + @Override + public boolean wasModified(String path) { + return false; + } + } + + static GitRepository create(File rootDir, TestRetryTaskExtensionAdapter extension) { + if (!extension.getModifiedTestRetry()) { + return new NoRepository(); + } + try ( + Git git = Git.open(rootDir) + ) { + Repository repository = git.getRepository(); + AbstractTreeIterator oldTree = getTreeIterator("refs/heads/master", repository); + AbstractTreeIterator newTree = getTreeIterator("HEAD", repository); + List diffs = git.diff() + .setOldTree(oldTree) + .setNewTree(newTree) + .call(); + return new GitRepository( + diffs.stream() + .map(DiffEntry::getNewPath) + .map(it -> { //src/test/java/acme/SuccessfulTests.java + final int firstSlash = it.indexOf('/'); //src/ + final int secondSlash = it.indexOf('/', firstSlash + 1); //test/ + final int thirdSlash = it.indexOf('/', secondSlash + 1); //java/ + final int extensionIndex = it.lastIndexOf('.'); //.java + final String packageClass = it.substring(thirdSlash + 1, extensionIndex); // acme/SuccessfulTests + return packageClass.replace('/', '.'); //acme.SuccessfulTests + } + ) + .collect(toList()) + ); + } catch (IOException | GitAPIException e) { + return new NoRepository(); + } + } + + private static AbstractTreeIterator getTreeIterator(String name, Repository repository) + throws IOException { + final ObjectId id = repository.resolve(name); + if (id == null) { + throw new IllegalArgumentException(name); + } + final CanonicalTreeParser p = new CanonicalTreeParser(); + try (ObjectReader or = repository.newObjectReader(); RevWalk walk = new RevWalk(repository)) { + p.reset(or, walk.parseTree(id)); + return p; + } + } +} diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java index 36244039..9649fc10 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java @@ -27,6 +27,7 @@ import org.gradle.testretry.internal.filter.AnnotationInspectorImpl; import org.gradle.testretry.internal.filter.RetryFilter; +import java.io.File; import java.util.stream.Collectors; public final class RetryTestExecuter implements TestExecuter { @@ -35,6 +36,7 @@ public final class RetryTestExecuter implements TestExecuter delegate; private final Test testTask; private final TestFrameworkTemplate frameworkTemplate; + private final GitRepository gitRepository; private RoundResult lastResult; @@ -43,11 +45,13 @@ public RetryTestExecuter( TestRetryTaskExtensionAdapter extension, TestExecuter delegate, Instantiator instantiator, - ObjectFactory objectFactory + ObjectFactory objectFactory, + File rootDir ) { this.extension = extension; this.delegate = delegate; this.testTask = task; + this.gitRepository = GitRepository.create(rootDir, extension); this.frameworkTemplate = new TestFrameworkTemplate( testTask, instantiator, @@ -81,7 +85,8 @@ public void execute(JvmTestExecutionSpec spec, TestResultProcessor testResultPro filter, frameworkTemplate.testsReader, testResultProcessor, - maxFailures + maxFailures, + gitRepository ); int retryCount = 0; diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java index 6170b555..7a6deb88 100644 --- a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java +++ b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java @@ -37,6 +37,7 @@ final class RetryTestResultProcessor implements TestResultProcessor { private final TestResultProcessor delegate; private final int maxFailures; + private final GitRepository gitRepository; private boolean lastRetry; private boolean hasRetryFilteredFailures; @@ -44,6 +45,7 @@ final class RetryTestResultProcessor implements TestResultProcessor { private TestNames currentRoundFailedTests = new TestNames(); private TestNames previousRoundFailedTests = new TestNames(); + private TestNames currentRoundFilteredTests = new TestNames(); private Object rootTestDescriptorId; @@ -52,13 +54,15 @@ final class RetryTestResultProcessor implements TestResultProcessor { RetryFilter filter, TestsReader testsReader, TestResultProcessor delegate, - int maxFailures + int maxFailures, + GitRepository gitRepository ) { this.testFrameworkStrategy = testFrameworkStrategy; this.filter = filter; this.testsReader = testsReader; this.delegate = delegate; this.maxFailures = maxFailures; + this.gitRepository = gitRepository; } @Override @@ -89,6 +93,10 @@ public void completed(Object testId, TestCompleteEvent testCompleteEvent) { if (failedInPreviousRound && testCompleteEvent.getResultType() == SKIPPED) { currentRoundFailedTests.add(className, name); } + if (gitRepository.wasModified(className) && !isClassDescriptor(descriptor) + && !currentRoundFilteredTests.remove(className, name)) { + currentRoundFailedTests.add(className, name); + } if (isClassDescriptor(descriptor)) { previousRoundFailedTests.remove(className, n -> { @@ -130,9 +138,10 @@ public void failure(Object testId, Throwable throwable) { if (descriptor != null) { String className = descriptor.getClassName(); if (className != null) { - if (filter.canRetry(className)) { + if (filter.canRetry(className) && !gitRepository.wasModified(className)) { currentRoundFailedTests.add(className, descriptor.getName()); } else { + currentRoundFilteredTests.add(className, descriptor.getName()); hasRetryFilteredFailures = true; } } diff --git a/plugin/src/test/groovy/org/gradle/testretry/ModifiedGroovyFuncTest.groovy b/plugin/src/test/groovy/org/gradle/testretry/ModifiedGroovyFuncTest.groovy new file mode 100644 index 00000000..21d9f7f7 --- /dev/null +++ b/plugin/src/test/groovy/org/gradle/testretry/ModifiedGroovyFuncTest.groovy @@ -0,0 +1,279 @@ +/* + * Copyright 2019 the original author or authors. + * + * 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 org.gradle.testretry + +import groovy.json.StringEscapeUtils +import org.eclipse.jgit.api.Git +import org.eclipse.jgit.dircache.DirCache +import org.eclipse.jgit.lib.ObjectId +import org.eclipse.jgit.lib.ObjectReader +import org.eclipse.jgit.lib.Repository +import org.eclipse.jgit.revwalk.RevCommit +import org.eclipse.jgit.revwalk.RevWalk +import org.eclipse.jgit.treewalk.AbstractTreeIterator +import org.eclipse.jgit.treewalk.CanonicalTreeParser +import spock.lang.Unroll + +class ModifiedGroovyFuncTest extends AbstractGeneralPluginFuncTest { + private Git git + + @Unroll + def "has no effect when is disabled (gradle version #gradleVersion)"() { + when: + buildFile << """ + test.retry.modifiedTestRetry = false + """ + + successfulTest() + + then: + def result = gradleRunner(gradleVersion).build() + + and: + result.output.count('PASSED') == 1 + + where: + gradleVersion << GRADLE_VERSIONS_UNDER_TEST + } + + @Unroll + def "has no effect when is enabled in none git context (gradle version #gradleVersion)"() { + when: + buildFile << """ + test.retry.modifiedTestRetry = true + test.retry.maxRetries = 1 + """ + + successfulTest() + + then: + def result = gradleRunner(gradleVersion).build() + + and: + result.output.count('PASSED') == 1 + + where: + gradleVersion << GRADLE_VERSIONS_UNDER_TEST + } + + @Unroll + def "has no effect for untracked files (gradle version #gradleVersion)"() { + when: + buildFile << """ + test.retry.modifiedTestRetry = true + test.retry.maxRetries = 1 + """ + + initGit() + + successfulTest() + + then: + def result = gradleRunner(gradleVersion).build() + + and: + result.output.count('PASSED') == 1 + + where: + gradleVersion << GRADLE_VERSIONS_UNDER_TEST + } + + @Unroll + def "has no effect for staged files (gradle version #gradleVersion)"() { + when: + buildFile << """ + test.retry.modifiedTestRetry = true + test.retry.maxRetries = 1 + """ + + initGit() + + successfulTest() + + stageSuccessfulTest() + + then: + def result = gradleRunner(gradleVersion).build() + + and: + result.output.count('PASSED') == 1 + + where: + gradleVersion << GRADLE_VERSIONS_UNDER_TEST + } + + @Unroll + def "retries all commited test cases based on diff with master (gradle version #gradleVersion)"() { + when: + buildFile << """ + test.retry.modifiedTestRetry = true + test.retry.maxRetries = 1 + """ + + initGit() + + successfulTest() + + commitInNewBranch() + + then: + def result = gradleRunner(gradleVersion).build() + + and: + result.output.count('PASSED') == 2 + + where: + gradleVersion << GRADLE_VERSIONS_UNDER_TEST + } + + @Unroll + def "has no effect when the very first test fails (gradle version #gradleVersion)"() { + when: + buildFile << """ + test.retry.modifiedTestRetry = true + test.retry.maxRetries = 10 + """ + + initGit() + + failedTest() + + commitInNewBranch() + + then: + def result = gradleRunner(gradleVersion).buildAndFail() + + and: + // 1 individual tests FAILED + 1 overall task FAILED + 1 overall build FAILED + result.output.count('FAILED') == 1 + 1 + 1 + + where: + gradleVersion << GRADLE_VERSIONS_UNDER_TEST + } + + @Unroll + def "retries until the first fail (gradle version #gradleVersion)"() { + when: + buildFile << """ + test.retry.modifiedTestRetry = true + test.retry.maxRetries = 10 + """ + + initGit() + + flakyOnRetry() + + commitInNewBranch() + + then: + def result = gradleRunner(gradleVersion).buildAndFail() + + and: + result.output.count('PASSED') == 1 + result.output.count('FAILED') == 1 + 1 + 1 + + where: + gradleVersion << GRADLE_VERSIONS_UNDER_TEST + } + + void flakyOnRetry() { + writeTestSource flakyOnNthExecutionAssertClass() + writeTestSource(""" + package acme; + + public class FlakyTests { + @org.junit.Test + public void flaky() { + ${flakyAssert()} + } + } + """) + } + + String flakyAssert(String id = "id", int failures = 1) { + return "acme.FlakyOnNthExecutionAssert.flakyAssert(\"${StringEscapeUtils.escapeJava(id)}\", $failures);" + } + + String flakyOnNthExecutionAssertClass() { + """ + package acme; + + import java.nio.file.*; + + public class FlakyOnNthExecutionAssert { + public static void flakyAssert(String id, int failures) { + Path marker = Paths.get("build/marker.file." + id); + try { + if (Files.exists(marker)) { + int counter = Integer.parseInt(new String(Files.readAllBytes(marker))); + if (++counter == failures) { + throw new RuntimeException("fail me!"); + } + Files.write(marker, Integer.toString(counter).getBytes()); + } else { + Files.write(marker, "0".getBytes()); + } + } catch (java.io.IOException e) { + throw new java.io.UncheckedIOException(e); + } + } + } + """ + } + + void initGit() { + git = Git.init().setDirectory(testProjectDir.root).call() + addAll() + commit("init") + } + + void stageSuccessfulTest() { + addAll() + } + + void commitInNewBranch() { + git.checkout() + .setCreateBranch(true) + .setName("new-branch") + .call() + addAll() + commit("new test") + } + + RevCommit commit(String message) { + git.commit() + .setMessage(message) + .setAuthor("John", "Doe") + .call() + } + + DirCache addAll() { + git.add().addFilepattern(".").call() + } + + + private AbstractTreeIterator getTreeIterator(String name, Repository repository) throws IOException { + final ObjectId id = repository.resolve(name) + if (id == null) { + throw new IllegalArgumentException(name) + } + final CanonicalTreeParser p = new CanonicalTreeParser() + ObjectReader or = repository.newObjectReader() + RevWalk walk = new RevWalk(repository) + p.reset(or, walk.parseTree(id)) + return p + } +}