Add failOnError parameter (#153)

* Add warningOnly parameter * Remove unncessary logs property * prefix property * change log validation in it * Add option to README * Rename option to failOnWarning (and reverse behavior) * failOnWarning -> failOnError * Fix misspelled dir
spotify · Jun 14, 2022 · 51083a6 · 51083a6
1 parent b67e622
commit 51083a6
Show file tree

Hide file tree

Showing 16 changed files with 255 additions and 13 deletions.
diff --git a/README.md b/README.md
@@ -138,11 +138,12 @@ example:
 
 ### check Options
 
-`displayFiles` default = true. Display the list of the files that are not compliant
+`displayFiles` default = true. Display the list of the files that are not compliant.
 
-`displayLimit` default = 100. Number of files to display that are not compliant`
+`displayLimit` default = 100. Number of files to display that are not compliant.
+
+`failOnError` default = true. Fail the build if non-compliant files are found.
 
-`style` sets the formatter style to be _google_ or _aosp_. By default this is 'google'. Projects using Android conventions may prefer `aosp`.
 
 example to not display the non-compliant files:
 ```xml
@@ -190,6 +191,29 @@ example to limit the display up to 10 files
 </build>
 ```
 
+example to only warn about non-compliant files instead of failing the build
+```xml
+<build>
+    <plugins>
+        <plugin>
+            <groupId>com.spotify.fmt</groupId>
+            <artifactId>fmt-maven-plugin</artifactId>
+            <version>VERSION</version>
+            <configuration>
+                <failOnError>false</failOnError>
+            </configuration>
+            <executions>
+                <execution>
+                    <goals>
+                        <goal>check</goal>
+                    </goals>
+                </execution>
+            </executions>
+        </plugin>
+    </plugins>
+</build>
+```
+
 ### Command line
 
 You can also use it on the command line

diff --git a/pom.xml b/pom.xml
@@ -123,6 +123,12 @@
             <version>0.28</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+            <version>4.3.1</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

diff --git a/src/main/java/com/spotify/fmt/Check.java b/src/main/java/com/spotify/fmt/Check.java
@@ -3,6 +3,7 @@
 import static java.lang.Math.max;
 import static java.lang.String.format;
 
+import java.util.function.Consumer;
 import org.apache.maven.plugin.MojoFailureException;
 import org.apache.maven.plugins.annotations.LifecyclePhase;
 import org.apache.maven.plugins.annotations.Mojo;
@@ -23,6 +24,10 @@ public class Check extends AbstractFMT {
   @Parameter(defaultValue = "100", property = "displayLimit")
   private int displayLimit;
 
+  /** Fail build for non-compliant formatting */
+  @Parameter(defaultValue = "true", property = "fmt.failOnError")
+  private boolean failOnError;
+
   /**
    * Post Execute action. It is called at the end of the execute method. Subclasses can add extra
    * checks.
@@ -32,29 +37,34 @@ public class Check extends AbstractFMT {
    */
   @Override
   protected void postExecute(FormattingResult result) throws MojoFailureException {
+    Consumer<String> messageConsumer = failOnError ? getLog()::error : getLog()::warn;
     if (!result.nonComplyingFiles().isEmpty()) {
       String message =
-          "Found " + result.nonComplyingFiles().size() + " non-complying files, failing build";
-      getLog().error(message);
-      getLog()
-          .error("To fix formatting errors, run \"mvn com.spotify.fmt:fmt-maven-plugin:format\"");
+          "Found "
+              + result.nonComplyingFiles().size()
+              + " non-complying files"
+              + (failOnError ? ", failing build" : "");
+      messageConsumer.accept(message);
+      messageConsumer.accept(
+          "To fix formatting errors, run \"mvn com.spotify.fmt:fmt-maven-plugin:format\"");
       // do not support limit < 1
       displayLimit = max(1, displayLimit);
 
       // Display first displayLimit files not formatted
       if (displayFiles) {
         result.nonComplyingFiles().stream()
             .limit(displayLimit)
-            .forEach(path -> getLog().error("Non complying file: " + path));
+            .forEach(path -> messageConsumer.accept("Non complying file: " + path));
 
         if (result.nonComplyingFiles().size() > displayLimit) {
-          getLog()
-              .error(
-                  format(
-                      "... and %d more files.", result.nonComplyingFiles().size() - displayLimit));
+          messageConsumer.accept(
+              format("... and %d more files.", result.nonComplyingFiles().size() - displayLimit));
         }
       }
-      throw new MojoFailureException(message);
+
+      if (failOnError) {
+        throw new MojoFailureException(message);
+      }
     }
   }
 

diff --git a/src/test/java/com/spotify/fmt/FMTTest.java b/src/test/java/com/spotify/fmt/FMTTest.java
@@ -3,14 +3,18 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assume.assumeFalse;
 import static org.junit.Assume.assumeTrue;
+import static org.mockito.AdditionalMatchers.not;
 
 import java.io.File;
 import java.util.List;
 import org.apache.commons.io.IOUtils;
+import org.apache.maven.plugin.Mojo;
 import org.apache.maven.plugin.MojoFailureException;
+import org.apache.maven.plugin.logging.Log;
 import org.apache.maven.plugin.testing.MojoRule;
 import org.junit.Rule;
 import org.junit.Test;
+import org.mockito.Mockito;
 
 public class FMTTest {
   private static String FORMAT = "format";
@@ -198,6 +202,7 @@ public void checkFailsWhenNotFormatted() throws Exception {
   @Test
   public void checkSucceedsWhenFormatted() throws Exception {
     Check check = loadMojo("check_formatted", CHECK);
+
     check.execute();
   }
 
@@ -213,6 +218,37 @@ public void checkFailsWhenFormattingFails() throws Exception {
     check.execute();
   }
 
+  @Test
+  public void checkWarnsWhenNotFormattedAndConfiguredWithFailOnErrorFalse() throws Exception {
+    Check check = loadMojo("failonerrorfalse_notformatted", CHECK);
+    Log logSpy = setupLogSpy(check);
+
+    check.execute();
+
+    Mockito.verify(logSpy).warn(Mockito.contains("Non complying file"));
+  }
+
+  @Test
+  public void checkDoesNotWarnWhenFormattedAndConfiguredWithFailOnErrorFalse() throws Exception {
+    Check check = loadMojo("failonerrorfalse_formatted", CHECK);
+    Log logSpy = setupLogSpy(check);
+
+    check.execute();
+
+    Mockito.verify(logSpy).warn(not(Mockito.contains("Non complying file")));
+  }
+
+  @Test(expected = MojoFailureException.class)
+  public void checkFailsAndLogsErrorWhenFormattingFailsAndConfiguredWithWarningOnlyFalse()
+      throws Exception {
+    Check check = loadMojo("failonerrortrue_notformatted", CHECK);
+    Log logSpy = setupLogSpy(check);
+
+    check.execute();
+
+    Mockito.verify(logSpy).error(Mockito.contains("Non complying file"));
+  }
+
   @SuppressWarnings("unchecked")
   private <T extends AbstractFMT> T loadMojo(String pomFilePath, String goal) throws Exception {
     File pomFile = loadPom(pomFilePath);
@@ -226,6 +262,12 @@ private File loadPom(String folderName) {
     return new File("src/test/resources/", folderName);
   }
 
+  private Log setupLogSpy(Mojo mojo) {
+    Log spy = Mockito.spy(mojo.getLog());
+    mojo.setLog(spy);
+    return spy;
+  }
+
   private static boolean javaRuntimeStronglyEncapsulatesByDefault() {
     return Runtime.version().compareTo(Runtime.Version.parse("16")) >= 0;
   }

diff --git a/src/test/resources/failonerrorfalse_formatted/invoker.properties b/src/test/resources/failonerrorfalse_formatted/invoker.properties
@@ -0,0 +1,2 @@
+invoker.goals = ${project.groupId}:${project.artifactId}:${project.version}:check
+invoker.streamLogs = false
diff --git a/src/test/resources/failonerrorfalse_formatted/pom.xml b/src/test/resources/failonerrorfalse_formatted/pom.xml
@@ -0,0 +1,41 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <modelVersion>4.0.0</modelVersion>
+
+    <groupId>org.apache.maven.plugin.my.unit</groupId>
+    <artifactId>project-to-test</artifactId>
+    <version>1.0.0</version>
+    <packaging>jar</packaging>
+    <name>Test MyMojo</name>
+
+    <dependencies>
+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <version>4.13.1</version>
+            <scope>test</scope>
+        </dependency>
+    </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>com.spotify.fmt</groupId>
+                <artifactId>fmt-maven-plugin</artifactId>
+                <version>2.12</version>
+                <configuration>
+                    <failOnError>false</failOnError>
+                </configuration>
+                <executions>
+                    <execution>
+                        <goals>
+                            <goal>check</goal>
+                        </goals>
+                    </execution>
+                </executions>
+            </plugin>
+        </plugins>
+    </build>
+
+</project>
diff --git a/src/test/resources/failonerrorfalse_formatted/postbuild.groovy b/src/test/resources/failonerrorfalse_formatted/postbuild.groovy
@@ -0,0 +1,2 @@
+String buildLog = new File("${basedir}/build.log").getText("UTF-8")
+assert !buildLog.contains("Non complying file")
diff --git a/src/test/resources/failonerrorfalse_formatted/src/main/java/HelloWorld1.java b/src/test/resources/failonerrorfalse_formatted/src/main/java/HelloWorld1.java
@@ -0,0 +1,7 @@
+package notestsource.src.main.java;
+
+public class HelloWorld1 {
+  public static void main(String[] args) {
+    System.out.println("Hello World!");
+  }
+}
diff --git a/src/test/resources/failonerrorfalse_notformatted/invoker.properties b/src/test/resources/failonerrorfalse_notformatted/invoker.properties
@@ -0,0 +1,2 @@
+invoker.goals = ${project.groupId}:${project.artifactId}:${project.version}:check
+invoker.streamLogs = false
diff --git a/src/test/resources/failonerrorfalse_notformatted/pom.xml b/src/test/resources/failonerrorfalse_notformatted/pom.xml
@@ -0,0 +1,41 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <modelVersion>4.0.0</modelVersion>
+
+    <groupId>org.apache.maven.plugin.my.unit</groupId>
+    <artifactId>project-to-test</artifactId>
+    <version>1.0.0</version>
+    <packaging>jar</packaging>
+    <name>Test MyMojo</name>
+
+    <dependencies>
+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <version>4.13.1</version>
+            <scope>test</scope>
+        </dependency>
+    </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>com.spotify.fmt</groupId>
+                <artifactId>fmt-maven-plugin</artifactId>
+                <version>2.12</version>
+                <configuration>
+                    <failOnError>false</failOnError>
+                </configuration>
+                <executions>
+                    <execution>
+                        <goals>
+                            <goal>check</goal>
+                        </goals>
+                    </execution>
+                </executions>
+            </plugin>
+        </plugins>
+    </build>
+
+</project>
diff --git a/src/test/resources/failonerrorfalse_notformatted/postbuild.groovy b/src/test/resources/failonerrorfalse_notformatted/postbuild.groovy
@@ -0,0 +1,3 @@
+String buildLog = new File("${basedir}/build.log").getText("UTF-8")
+// Would want to assert that it's 'WARN....Non complying', but could not get the regex to work.
+assert buildLog.contains("Non complying file")
diff --git a/src/test/resources/failonerrorfalse_notformatted/src/main/java/HelloWorld1.java b/src/test/resources/failonerrorfalse_notformatted/src/main/java/HelloWorld1.java
@@ -0,0 +1,8 @@
+package notestsource.src.main.java;
+
+public
+class HelloWorld1 {
+  public static    void main(String[] args) {
+    System.out.println("Hello World!");;
+  }
+}
diff --git a/src/test/resources/failonerrortrue_notformatted/invoker.properties b/src/test/resources/failonerrortrue_notformatted/invoker.properties
@@ -0,0 +1,2 @@
+invoker.goals = ${project.groupId}:${project.artifactId}:${project.version}:check
+invoker.buildResult = failure
diff --git a/src/test/resources/failonerrortrue_notformatted/pom.xml b/src/test/resources/failonerrortrue_notformatted/pom.xml
@@ -0,0 +1,41 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <modelVersion>4.0.0</modelVersion>
+
+    <groupId>org.apache.maven.plugin.my.unit</groupId>
+    <artifactId>project-to-test</artifactId>
+    <version>1.0.0</version>
+    <packaging>jar</packaging>
+    <name>Test MyMojo</name>
+
+    <dependencies>
+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <version>4.13.1</version>
+            <scope>test</scope>
+        </dependency>
+    </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>com.spotify.fmt</groupId>
+                <artifactId>fmt-maven-plugin</artifactId>
+                <version>2.12</version>
+                <configuration>
+                    <failOnError>true</failOnError>
+                </configuration>
+                <executions>
+                    <execution>
+                        <goals>
+                            <goal>check</goal>
+                        </goals>
+                    </execution>
+                </executions>
+            </plugin>
+        </plugins>
+    </build>
+
+</project>
diff --git a/src/test/resources/failonerrortrue_notformatted/postbuild.groovy b/src/test/resources/failonerrortrue_notformatted/postbuild.groovy
@@ -0,0 +1,3 @@
+String buildLog = new File("${basedir}/build.log").getText("UTF-8")
+// Would want to assert that it's 'ERROR....Non complying', but could not get the regex to work.
+assert buildLog.contains("Non complying file")
diff --git a/src/test/resources/failonerrortrue_notformatted/src/main/java/HelloWorld1.java b/src/test/resources/failonerrortrue_notformatted/src/main/java/HelloWorld1.java
@@ -0,0 +1,8 @@
+package notestsource.src.main.java;
+
+public
+class HelloWorld1 {
+  public static    void main(String[] args) {
+    System.out.println("Hello World!");;
+  }
+}
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		invoker.goals = ${project.groupId}:${project.artifactId}:${project.version}:check
		invoker.streamLogs = false
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		String buildLog = new File("${basedir}/build.log").getText("UTF-8")
		assert !buildLog.contains("Non complying file")
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		invoker.goals = ${project.groupId}:${project.artifactId}:${project.version}:check
		invoker.buildResult = failure