Switch to JDT compiler; other static analysis changes (#395)

- Eliminate double compilation in server (maven-compiler plus takari-lifecycle) - Switch to Eclipse JDT compiler to restore lots of missing warnings - Avoids unhelpful annotation warnings (JDK-6999068) - Also warns about a lot of new things (eg imports) - Increase javac warnings threshold to match JDT warnings - Fix some compiler warnings - Fix new FindBugs warnings triggered by JDT's bytecode - Change ArgumentCaptor to handle varargs in test - Lower FindBugs warnings thresholds to 0 - Use warnings parser for Kotlin (custom) - Update detekt, assertj - Add workaround for detekt/detekt#89 - Simplify Kotlin functions with detekt - Remove gitdescribe duplicate logging - Improve activation of staticAnalysis Maven profile
zanata · Jun 20, 2017 · 6f0ceee · 6f0ceee
1 parent bb33af7
commit 6f0ceee
Show file tree

Hide file tree

Showing 24 changed files with 268 additions and 142 deletions.
diff --git a/Jenkinsfile b/Jenkinsfile
@@ -274,21 +274,19 @@ timestamps {
           // TODO reduce unstableTotal thresholds as bugs are eliminated
           step([$class: 'FindBugsPublisher',
                 pattern: '**/findbugsXml.xml',
-                unstableTotalAll: '467',
-                unstableTotalHigh: '47',
-                unstableTotalNormal: '420',
-                unstableTotalLow: '0'])
+                unstableTotalAll: '0'])
 
           step([$class: 'WarningsPublisher',
                 consoleParsers: [
-                        [parserName: 'Java Compiler (javac)'],    // 400 warnings
+                        [parserName: 'Java Compiler (javac)'],
+                        [parserName: 'kotlin'],
 //                        [parserName: 'JavaDoc'],
 //                        [parserName: 'Maven'], // ~279 warnings, but too variable
                         // TODO check integration test warnings (EAP and WildFly)
                         //[parserName: 'appserver log messages'], // 119 warnings
                         //[parserName: 'browser warnings'],       // 0 warnings
                 ],
-                unstableTotalAll: '4',
+                unstableTotalAll: '1366',
                 unstableTotalHigh: '0',
           ])
           // TODO check integration test warnings (EAP and WildFly)

diff --git a/api/zanata-common-api/src/main/java/org/zanata/common/DocumentType.java b/api/zanata-common-api/src/main/java/org/zanata/common/DocumentType.java
@@ -143,7 +143,10 @@ private static Set<DocumentType> fromExtension(boolean source,
     /**
      * Create a document type enum constant with the given list of default extensions.
      */
-    DocumentType(@Nonnull String... extensions) throws IllegalArgumentException {
+    // Don't use @Nonnull in the constructor signature or you'll cause
+    // https://bugs.eclipse.org/bugs/show_bug.cgi?id=388314 and
+    // https://bugs.openjdk.java.net/browse/JDK-8024694
+    DocumentType(String... extensions) throws IllegalArgumentException {
         Map<String, String> extensionsMap = new HashMap<>();
         for (String extension : extensions) {
             extensionsMap.put(extension, extension);
@@ -154,7 +157,10 @@ private static Set<DocumentType> fromExtension(boolean source,
     /**
      * Create a document type enum constant with the given list of default extensions.
      */
-    DocumentType(@Nonnull Map<String, String> extensions) throws IllegalArgumentException {
+    // Don't use @Nonnull in the constructor signature or you'll cause
+    // https://bugs.eclipse.org/bugs/show_bug.cgi?id=388314 and
+    // https://bugs.openjdk.java.net/browse/JDK-8024694
+    DocumentType(Map<String, String> extensions) throws IllegalArgumentException {
         this.extensions = unmodifiableMap(extensions);
     }
 

diff --git a/build-tools/pom.xml b/build-tools/pom.xml
@@ -65,7 +65,7 @@
         <groupId>io.takari.maven.plugins</groupId>
         <artifactId>takari-lifecycle-plugin</artifactId>
         <!-- Unfortunately, we can't inherit this version from parent: -->
-        <version>1.12.2</version>
+        <version>1.12.4</version>
         <extensions>true</extensions>
         <configuration>
           <proc>proc</proc>

diff --git a/build-tools/src/main/resources/zanata-build-tools/detekt.yml b/build-tools/src/main/resources/zanata-build-tools/detekt.yml
@@ -4,19 +4,24 @@ build:
   warningThreshold: 5
   failThreshold: 10
   weights:
-    code-smell: 2
+    complexity: 2
+    formatting: 0
     LongParameterList: 1
+    comments: 0.5
 
 potential-bugs:
   active: true
+  DuplicateCaseInWhenExpression:
+    active: true
+  EqualsWithHashCodeExist:
+    active: true
+  ExplicitGarbageCollectionCall:
+    active: true
 
 exceptions:
   active: false
 
-comments:
-  active: true
-
-empty:
+empty-blocks:
   active: true
 
 complexity:
@@ -46,7 +51,8 @@ formatting:
   useTabs: false
   Indentation:
     active: true
-    autoCorrect: true
+    # workaround for https://github.com/arturbosch/detekt/issues/89
+    indentSize: "4"
   ConsecutiveBlankLines:
     active: true
     autoCorrect: true
@@ -80,18 +86,27 @@ formatting:
   OptionalUnit:
     active: true
     autoCorrect: true
+  ExpressionBodySyntax:
+    active: false
+    autoCorrect: true
+  ExpressionBodySyntaxLineBreaks:
+    active: false
+    autoCorrect: true
 
 style:
   active: true
   WildcardImport:
     active: true
+  MaxLineLength:
+    active: true
+    maxLineLength: 120
   NamingConventionViolation:
     active: true
     variablePattern: '^(_)?[a-z$][a-zA-Z$0-9]*$'
     constantPattern: '^([A-Z_]*|serialVersionUID)$'
     methodPattern: '^[a-z$][a-zA-Z$0-9]*$'
     classPattern: '[A-Z$][a-zA-Z$]*'
-    enumEntryPattern: '^[A-Z$][A-Z_$]*$'
+    enumEntryPattern: '^[A-Z$][a-zA-Z_$]*$'
 
 comments:
   active: true
@@ -103,3 +118,12 @@ comments:
     active: false
   NoDocOverPublicMethod:
     active: false
+
+# *experimental feature*
+# Migration rules can be defined in the same config file or a new one
+migration:
+  active: false
+  imports:
+    # your.package.Class: new.package.or.Class
+    # for example:
+    # io.gitlab.arturbosch.detekt.api.Rule: io.gitlab.arturbosch.detekt.rule.Rule
diff --git a/.../zanata-client-commands/src/main/java/org/zanata/client/commands/push/RawPushCommand.java b/.../zanata-client-commands/src/main/java/org/zanata/client/commands/push/RawPushCommand.java
@@ -656,7 +656,7 @@ private String calculateFileHash(File srcFile) {
             } finally {
                 fileStream.close();
             }
-            @SuppressWarnings("UnnecessaryLocalVariable")
+            //noinspection UnnecessaryLocalVariable
             String md5hash = new String(Hex.encodeHex(md.digest()));
             return md5hash;
         } catch (NoSuchAlgorithmException | IOException e) {

diff --git a/...nt/zanata-client-commands/src/test/java/org/zanata/client/commands/UpdateCheckerTest.java b/...nt/zanata-client-commands/src/test/java/org/zanata/client/commands/UpdateCheckerTest.java
@@ -49,7 +49,7 @@ public class UpdateCheckerTest {
     @Captor
     private ArgumentCaptor<String> outputStringCaptor;
     @Captor
-    private ArgumentCaptor<Object[]> outputArgsCaptor;
+    private ArgumentCaptor<Object> outputArgsCaptor;
     private File marker;
 
     @Before
@@ -166,7 +166,7 @@ public void canCheckForUpdates() throws Exception {
         List<String> allValues = outputStringCaptor.getAllValues();
         String lastMessage = allValues.get(allValues.size() - 1);
         assertThat(lastMessage, Matchers.equalTo(expectedString));
-        assertThat(outputArgsCaptor.getValue(), Matchers.<Object> equalTo("3.3.2"));
+        assertThat(outputArgsCaptor.getAllValues(), Matchers.contains("3.3.2"));
 
         Properties props = new Properties();
         props.load(new FileReader(marker));

diff --git a/client/zanata-maven-plugin/pom.xml b/client/zanata-maven-plugin/pom.xml
@@ -126,6 +126,20 @@
 
   <build>
     <plugins>
+      <plugin>
+        <artifactId>maven-compiler-plugin</artifactId>
+        <configuration>
+          <!-- Override maven.compiler.compilerId=jdt -->
+          <compilerId>eclipse</compilerId>
+        </configuration>
+        <dependencies>
+          <dependency>
+            <groupId>org.codehaus.plexus</groupId>
+            <artifactId>plexus-compiler-eclipse</artifactId>
+            <version>2.7</version>
+          </dependency>
+        </dependencies>
+      </plugin>
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-plugin-plugin</artifactId>

diff --git a/client/zanata-maven-plugin/src/main/java/org/zanata/maven/AbstractPushPullMojo.java b/client/zanata-maven-plugin/src/main/java/org/zanata/maven/AbstractPushPullMojo.java
@@ -102,7 +102,6 @@ private String toMavenModuleID(MavenProject module) {
     /**
      * The projects in the reactor.
      */
-    @SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
     @Parameter(defaultValue = "${reactorProjects}", readonly = true)
     private List<MavenProject> reactorProjects;
 

diff --git a/parent/pom.xml b/parent/pom.xml
@@ -191,6 +191,10 @@
     <required.jvm>1.8</required.jvm>
     <maven.compiler.target>${required.jvm}</maven.compiler.target>
     <maven.compiler.showWarnings>true</maven.compiler.showWarnings>
+    <!-- Tell takari to default to JDT (unless overridden on command line)
+         Note that maven-compiler-plugin doesn't support this compilerId, so
+         it must be overridden in zanata-maven-plugin. -->
+    <maven.compiler.compilerId>jdt</maven.compiler.compilerId>
 
     <!--
       Version of Java used for API compatibility checking. Should
@@ -208,6 +212,7 @@
     <mdep.analyze.skip>true</mdep.analyze.skip>
     <checkstyle.skip>true</checkstyle.skip>
     <animal.sniffer.skip>true</animal.sniffer.skip>
+    <restrict.skip>true</restrict.skip>
 
     <hamcrest.version>1.3</hamcrest.version>
     <findbugs.maven.version>3.0.1</findbugs.maven.version>
@@ -224,7 +229,6 @@
 
     <checkstyle.maven.version>2.17</checkstyle.maven.version>
     <checkstyle.version>7.2</checkstyle.version>
-    <checkstyle.skip>false</checkstyle.skip>
     <checkstyle.config.location>zanata-build-tools/checkstyle.xml</checkstyle.config.location>
     <checkstyle.suppressions.location>zanata-build-tools/checkstyle-suppressions.xml</checkstyle.suppressions.location>
     <!-- <checkstyle.enable.rules.summary>false</checkstyle.enable.rules.summary> -->
@@ -247,7 +251,7 @@
         <groupId>io.takari.maven.plugins</groupId>
         <artifactId>takari-lifecycle-plugin</artifactId>
         <!-- See also build-tools pom -->
-        <version>1.12.2</version>
+        <version>1.12.4</version>
         <extensions>true</extensions>
         <configuration>
           <proc>proc</proc>
@@ -638,11 +642,20 @@
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-antrun-plugin</artifactId>
-        <configuration>
-          <target>
-            <echo>[describe] ${gitDescribe}</echo>
-          </target>
-        </configuration>
+        <executions>
+          <execution>
+            <id>gitdescribe</id>
+            <phase>initialize</phase>
+            <goals>
+              <goal>run</goal>
+            </goals>
+            <configuration>
+              <target>
+                <echo>[describe] ${gitDescribe}</echo>
+              </target>
+            </configuration>
+          </execution>
+        </executions>
       </plugin>
 
       <plugin>
@@ -704,7 +717,7 @@
         </plugin>
         <plugin>
           <artifactId>maven-compiler-plugin</artifactId>
-          <version>3.3</version>
+          <version>3.5.1</version>
         </plugin>
         <plugin>
           <artifactId>maven-dependency-plugin</artifactId>
@@ -915,18 +928,20 @@
       <activation>
         <property>
           <name>staticAnalysis</name>
+          <value>true</value>
         </property>
       </activation>
       <build>
 
       </build>
       <properties>
+        <animal.sniffer.skip>false</animal.sniffer.skip>
+        <checkstyle.skip>false</checkstyle.skip>
         <dupfind.skip>false</dupfind.skip>
         <enforcer.skip>false</enforcer.skip>
         <findbugs.skip>false</findbugs.skip>
         <mdep.analyze.skip>false</mdep.analyze.skip>
-        <checkstyle.skip>false</checkstyle.skip>
-        <animal.sniffer.skip>false</animal.sniffer.skip>
+        <restrict.skip>false</restrict.skip>
       </properties>
     </profile>
 
@@ -1068,9 +1083,15 @@
       </activation>
       <properties>
         <allow.deploy.skip>false</allow.deploy.skip>
-        <checkstyle.skip>true</checkstyle.skip>
-        <enforcer.skip>true</enforcer.skip>
-        <findbugs.skip>true</findbugs.skip>
+
+        <animal.sniffer.skip>false</animal.sniffer.skip>
+        <checkstyle.skip>false</checkstyle.skip>
+        <dupfind.skip>false</dupfind.skip>
+        <enforcer.skip>false</enforcer.skip>
+        <findbugs.skip>false</findbugs.skip>
+        <mdep.analyze.skip>false</mdep.analyze.skip>
+        <restrict.skip>false</restrict.skip>
+
         <mavensign.expand.skip>*</mavensign.expand.skip>
         <mavensign.sign.skip>*</mavensign.sign.skip>
         <skipTests>true</skipTests>

diff --git a/server/functional-test/pom.xml b/server/functional-test/pom.xml
@@ -431,6 +431,10 @@
       <artifactId>validation-api</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>javax.enterprise</groupId>
+      <artifactId>cdi-api</artifactId>
+    </dependency>
 
     <dependency>
       <groupId>org.apache.httpcomponents</groupId>

diff --git a/server/functional-test/src/test/java/org/zanata/util/HasEmailRule.java b/server/functional-test/src/test/java/org/zanata/util/HasEmailRule.java
@@ -28,9 +28,6 @@
 import java.util.concurrent.TimeUnit;
 import javax.mail.internet.MimeMultipart;
 import org.junit.rules.ExternalResource;
-import org.junit.rules.TestRule;
-import org.junit.runner.Description;
-import org.junit.runners.model.Statement;
 import org.subethamail.wiser.Wiser;
 import org.subethamail.wiser.WiserMessage;
 import com.google.common.base.Throwables;
@@ -47,7 +44,6 @@ public class HasEmailRule extends ExternalResource {
     private static final Object wiserLock = new Object();
     private static volatile Wiser wiser;
 
-    @SuppressWarnings("ThrowableResultOfMethodCallIgnored")
     @Override
     protected void before() throws Throwable {
         super.before();
@@ -70,6 +66,7 @@ protected void before() throws Throwable {
                         throw new RuntimeException("Error binding port "
                                 + portNum + ". See log for more info.", e);
                     }
+                    throw e;
                 }
                 HasEmailRule.wiser = w;
                 // NB we never call wiser.stop() because we want the email

diff --git a/server/gwt-editor/src/main/java/org/zanata/webtrans/client/view/TransMemoryView.java b/server/gwt-editor/src/main/java/org/zanata/webtrans/client/view/TransMemoryView.java
@@ -283,8 +283,8 @@ private SimplePanel createTargetPanel(TransMemoryResultItem object) {
             panel.addStyleName(style.approved());
         } else if (object.getMatchType() == MatchType.TranslatedInternal) {
             panel.addStyleName(style.translated());
-        } else if (object.getMatchType() == MatchType.Imported) {
-            // TODO Add a style
+        //} else if (object.getMatchType() == MatchType.Imported) {
+            // TODO Add a style for imported/TMX matches
         }
 
         SafeHtml safeHtml =