Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task configuration avoidance (WIP) (#269) #277

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ VER_PEGDOWN_DOCLET=1.3
# Used in multiple places
VER_DURIAN=1.2.0
VER_JUNIT=4.12
VER_ASSERTJ=3.5.2
VER_ASSERTJ=3.10.0
VER_MOCKITO=2.13.0

# Used for Maven Plugin
Expand Down
Binary file modified gradle/wrapper/gradle-wrapper.jar
Binary file not shown.
2 changes: 1 addition & 1 deletion gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.9-bin.zip
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-4.6-bin.zip
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import com.diffplug.spotless.FormatterFunc;
import com.diffplug.spotless.FormatterStep;
import com.diffplug.spotless.LineEnding;
import com.diffplug.spotless.Provisioner;
import com.diffplug.spotless.extra.EclipseBasedStepBuilder;
import com.diffplug.spotless.extra.EclipseBasedStepBuilder.State;
Expand Down Expand Up @@ -68,7 +69,7 @@ private static FormatterFunc apply(EclipseBasedStepBuilder.State state) throws E
Method method = formatterClazz.getMethod(FORMATTER_METHOD, String.class);
return input -> {
try {
return (String) method.invoke(formatter, input);
return LineEnding.toUnix((String) method.invoke(formatter, input));
jbduncan marked this conversation as resolved.
Show resolved Hide resolved
} catch (InvocationTargetException exceptionWrapper) {
Throwable throwable = exceptionWrapper.getTargetException();
Exception exception = (throwable instanceof Exception) ? (Exception) throwable : null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import java.io.File;

import org.assertj.core.api.SoftAssertions;
import org.junit.Test;

import com.diffplug.spotless.FormatterStep;
Expand Down Expand Up @@ -65,25 +66,26 @@ public abstract class EclipseCommonTests extends ResourceHarness {

@Test
public void testSupportedVersions() throws Exception {
String[] versions = getSupportedVersions();
for (String version : versions) {
SoftAssertions softly = new SoftAssertions();
for (String version : getSupportedVersions()) {
String input = getTestInput(version);
String expected = getTestExpectation(version);
File inputFile = setFile("someInputFile").toContent(input);
FormatterStep step = null;
try {
step = createStep(version);
} catch (Exception e) {
fail("Exception occured when instantiating step for version: " + version, e);
fail("Exception occurred when instantiating step for version: " + version, e);
}
String output = null;
try {
output = step.format(input, inputFile);
} catch (Exception e) {
fail("Exception occured when formatting input with version: " + version, e);
fail("Exception occurred when formatting input with version: " + version, e);
}
assertThat(output).as("Formatting output unexpected with version: " + version).isEqualTo(expected);
softly.assertThat(output).as("Formatting output unexpected with version: " + version).isEqualTo(expected);
}
softly.assertAll();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.gradle.api.execution.TaskExecutionGraph;
import org.gradle.api.plugins.BasePlugin;
import org.gradle.api.plugins.JavaBasePlugin;
import org.gradle.api.tasks.TaskProvider;

import com.diffplug.spotless.SpotlessCache;

Expand Down Expand Up @@ -57,62 +58,70 @@ public SpotlessExtension getExtension() {

@SuppressWarnings("rawtypes")
void createTasks(Project project) {
Task rootCheckTask = project.task(EXTENSION + CHECK);
rootCheckTask.setGroup(TASK_GROUP);
rootCheckTask.setDescription(CHECK_DESCRIPTION);
Task rootApplyTask = project.task(EXTENSION + APPLY);
rootApplyTask.setGroup(TASK_GROUP);
rootApplyTask.setDescription(APPLY_DESCRIPTION);
TaskProvider<Task> rootCheckTaskProvider = project.getTasks().register(
EXTENSION + CHECK,
task -> {
task.setGroup(TASK_GROUP);
task.setDescription(CHECK_DESCRIPTION);
});
TaskProvider<Task> rootApplyTaskProvider = project.getTasks().register(
EXTENSION + APPLY,
task -> {
task.setGroup(TASK_GROUP);
task.setDescription(APPLY_DESCRIPTION);
});

spotlessExtension.formats.forEach((key, value) -> {
// create the task that does the work
String taskName = EXTENSION + capitalize(key);
SpotlessTask spotlessTask = project.getTasks().create(taskName, SpotlessTask.class);
value.setupTask(spotlessTask);
TaskProvider<SpotlessTask> spotlessTaskProvider = project.getTasks().register(taskName, SpotlessTask.class);
spotlessTaskProvider.configure(value::setupTask);

// create the check and apply control tasks
Task checkTask = project.getTasks().create(taskName + CHECK);
Task applyTask = project.getTasks().create(taskName + APPLY);
TaskProvider<Task> checkTaskProvider = project.getTasks().register(taskName + CHECK);
TaskProvider<Task> applyTaskProvider = project.getTasks().register(taskName + APPLY);
// the root tasks depend on them
rootCheckTask.dependsOn(checkTask);
rootApplyTask.dependsOn(applyTask);
rootCheckTaskProvider.configure(rootCheckTask -> rootCheckTask.dependsOn(checkTaskProvider));
rootApplyTaskProvider.configure(rootApplyTask -> rootApplyTask.dependsOn(applyTaskProvider));
// and they depend on the work task
checkTask.dependsOn(spotlessTask);
applyTask.dependsOn(spotlessTask);
checkTaskProvider.configure(checkTask -> checkTask.dependsOn(spotlessTaskProvider));
applyTaskProvider.configure(applyTask -> applyTask.dependsOn(spotlessTaskProvider));

// when the task graph is ready, we'll configure the spotlessTask appropriately
// TODO: Consider swapping out the Closure below for a type-safe Action<TaskExecutionGraph>
project.getGradle().getTaskGraph().whenReady(new Closure(null) {
private static final long serialVersionUID = 1L;

// called by gradle
@SuppressFBWarnings("UMAC_UNCALLABLE_METHOD_OF_ANONYMOUS_CLASS")
public Object doCall(TaskExecutionGraph graph) {
if (graph.hasTask(checkTask)) {
spotlessTask.setCheck();
if (checkTaskProvider.isPresent() && graph.hasTask(checkTaskProvider.get())) {
spotlessTaskProvider.configure(SpotlessTask::setCheck);
}
if (graph.hasTask(applyTask)) {
spotlessTask.setApply();
if (applyTaskProvider.isPresent() && graph.hasTask(applyTaskProvider.get())) {
spotlessTaskProvider.configure(SpotlessTask::setApply);
}
return Closure.DONE;
}
});
});

// Add our check task as a dependency on the global check task
// getTasks() returns a "live" collection, so this works even if the
// task doesn't exist at the time this call is made
// Add our check task as a dependency on the global check task.
// getTasks() returns a "live" collection and configureEach() is lazy,
// so this works even if the task doesn't exist at the time this call
// is made.
if (spotlessExtension.enforceCheck) {
project.getTasks()
.matching(task -> task.getName().equals(JavaBasePlugin.CHECK_TASK_NAME))
.all(task -> task.dependsOn(rootCheckTask));
.configureEach(task -> task.dependsOn(rootCheckTaskProvider));
}

// clear spotless' cache when the user does a clean, but only after any spotless tasks
Task clean = project.getTasks().getByName(BasePlugin.CLEAN_TASK_NAME);
clean.doLast(unused -> SpotlessCache.clear());
TaskProvider<Task> cleanTaskProvider = project.getTasks().named(BasePlugin.CLEAN_TASK_NAME);
cleanTaskProvider.configure(task -> task.doLast(unused -> SpotlessCache.clear()));
project.getTasks()
.withType(SpotlessTask.class)
.all(task -> task.mustRunAfter(clean));
.configureEach(task -> task.mustRunAfter(cleanTaskProvider));
}

static String capitalize(String input) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,17 @@
*/
package com.diffplug.gradle.spotless;

import static org.assertj.core.api.Assertions.assertThat;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import org.assertj.core.api.Assertions;
import org.gradle.testkit.runner.BuildResult;
import org.gradle.testkit.runner.TaskOutcome;
import org.junit.Test;

import com.diffplug.common.base.CharMatcher;
import com.diffplug.common.base.Splitter;
import com.diffplug.common.base.StringPrinter;
import com.diffplug.spotless.LineEnding;

/** Tests the desired behavior from https://github.com/diffplug/spotless/issues/46. */
Expand Down Expand Up @@ -67,8 +65,8 @@ public void anyExceptionShouldFail() throws Exception {
"} // spotless");
setFile("README.md").toContent("This code is fubar.");
runWithFailure(
":spotlessMiscStep 'no swearing' found problem in 'README.md':",
"No swearing!",
":spotlessMisc",
"Step 'no swearing' found problem in 'README.md'",
"java.lang.RuntimeException: No swearing!");
}

Expand All @@ -79,7 +77,7 @@ public void unlessEnforceCheckIsFalse() throws Exception {
" enforceCheck false",
"} // spotless");
setFile("README.md").toContent("This code is fubar.");
runWithSuccess(":compileJava UP-TO-DATE");
runWithSuccess(":compileJava NO-SOURCE");
}

@Test
Expand Down Expand Up @@ -113,28 +111,25 @@ public void failsIfNeitherStepNorFileExempted() throws Exception {
"} // spotless");
setFile("README.md").toContent("This code is fubar.");
runWithFailure(
":spotlessMiscStep 'no swearing' found problem in 'README.md':",
"No swearing!",
":spotlessMisc",
"Step 'no swearing' found problem in 'README.md'",
"java.lang.RuntimeException: No swearing!");
}

private void runWithSuccess(String... messages) throws Exception {
private void runWithSuccess(String... messagePartsInOrder) throws Exception {
BuildResult result = gradleRunner().withArguments("check").build();
assertResultAndMessages(result, TaskOutcome.SUCCESS, messages);
assertResultAndOutputContainsMessages(result, TaskOutcome.SUCCESS, messagePartsInOrder);
}

private void runWithFailure(String... messages) throws Exception {
private void runWithFailure(String... messagePartsInOrder) throws Exception {
BuildResult result = gradleRunner().withArguments("check").buildAndFail();
assertResultAndMessages(result, TaskOutcome.FAILED, messages);
assertResultAndOutputContainsMessages(result, TaskOutcome.FAILED, messagePartsInOrder);
}

private void assertResultAndMessages(BuildResult result, TaskOutcome outcome, String... messages) {
String expectedToStartWith = StringPrinter.buildStringFromLines(messages).trim();
int numNewlines = CharMatcher.is('\n').countIn(expectedToStartWith);
List<String> actualLines = Splitter.on('\n').splitToList(LineEnding.toUnix(result.getOutput()));
String actualStart = String.join("\n", actualLines.subList(0, numNewlines + 1));
Assertions.assertThat(actualStart).isEqualTo(expectedToStartWith);
Assertions.assertThat(result.tasks(outcome).size() + result.tasks(TaskOutcome.UP_TO_DATE).size())
private void assertResultAndOutputContainsMessages(BuildResult result, TaskOutcome outcome, String... messagePartsInOrder) {
String actualOutput = LineEnding.toUnix(result.getOutput());
assertThat(actualOutput).containsSubsequence(messagePartsInOrder);
assertThat(result.tasks(outcome).size() + result.tasks(TaskOutcome.UP_TO_DATE).size() + result.tasks(TaskOutcome.NO_SOURCE).size())
.isEqualTo(result.getTasks().size());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ public void gitAttributes() throws IOException {

protected final GradleRunner gradleRunner() throws IOException {
return GradleRunner.create()
// Test against Gradle 2.14.1 in order to maintain backwards compatibility.
// https://github.com/diffplug/spotless/issues/161
.withGradleVersion("2.14.1")
// Gradle 4.9 required for Task Configuration Avoidance
// (https://github.com/diffplug/spotless/issues/269)
.withGradleVersion("4.9")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually keep this as withGradleVersion("2.14.1"). That's how we make sure that our legacy support doesn't break.

Then, I would make a new GradleTaskConfigurationAvoidanceTest, which uses gradle 4.9, and asserts that tasks are actually created lazily. I don't see a need to introduce the GradleTest plugin, but maybe @eriwen knows about value that I'm missing.

Copy link
Member

@JLLeitschuh JLLeitschuh Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nedtwigg @jbduncan Is there an easy way to run all the tests twice once with each version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be, but our gradle tests are already pretty darn slow. None of our steps rely on Gradle, so it's really just the SpotlessExtension logic that we need to test. We compile against 4.9, so there's no risk that we'll use API that doesn't exist in 4.9. But we might accidentally use API that was added after 2.14, and that's what we'll catch by always running against 2.14 - problems with 4.9 will be caught at compile-time.

For gradle == 4.9, it's a good idea to test and make sure that the task avoidance is doing what we intend it to do. That will have the added bonus of being an integration test for the 4.9+ logic in general.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all sounds good to me. I will revert this change and create GradleTaskConfigurationAvoidanceTest as requested. :)

.withProjectDir(rootFolder())
.withPluginClasspath();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
*/
package com.diffplug.gradle.spotless;

import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.io.IOException;

import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;

public class GroovyExtensionTest extends GradleIntegrationTest {
Expand Down Expand Up @@ -82,12 +82,9 @@ public void excludeJavaWithCustomTarget() throws IOException {
" }",
"}");

try {
gradleRunner().withArguments("spotlessApply").build();
Assert.fail("Exception expected when running 'excludeJava' in combination with 'target'.");
} catch (Throwable t) {
Assertions.assertThat(t).hasMessageContaining("'excludeJava' is not supported");
}
assertThatThrownBy(() -> gradleRunner().withArguments("spotlessApply", "--stacktrace").build())
.as("Exception expected when running 'excludeJava' in combination with 'target'.")
.hasMessageContaining("'excludeJava' is not supported");
}

@Test
Expand All @@ -103,12 +100,9 @@ public void groovyPluginMissingCheck() throws IOException {
" }",
"}");

try {
gradleRunner().withArguments("spotlessApply").build();
Assert.fail("Exception expected when using 'groovy' without 'target' if groovy-plugin is not applied.");
} catch (Throwable t) {
Assertions.assertThat(t).hasMessageContaining("must apply the groovy plugin before");
}
assertThatThrownBy(() -> gradleRunner().withArguments("spotlessApply", "--stacktrace").build())
.as("Exception expected when using 'groovy' without 'target' if groovy-plugin is not applied.")
.hasMessageContaining("must apply the groovy plugin before");
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ public void testWithCommonInterfaceForConfiguringLicences() throws IOException {
" }",
"}");
gradleRunner()
.withGradleVersion("4.6")
.withArguments("spotlessApply")
.forwardOutput()
.build();
Expand Down