[MCHECKSTYLE-365] Correct counts in Rules section

When the site report is generated, in the Rules section, the plug-in groups together the violation counts for rules that have the same name and message, but that have different severity levels – specifically where one of the rules doesn't specify a severity level and uses the default (error). This results in both rules being listed with different properties and severity levels, but with the same violation count (which is incorrect). This change allows the rule matching to fall through to the most specific match rather than short-circuiting the match on success. This corrects the case where two rules of the same name have the same message but different severities and allows the match to proceed to check further conditions, such as the severity. In addition, this change also ensures that the severity is retrieved in the same way (by providing a default of "error" when missing) as the logic that displays the rule rows in the Rules section. Both of these changes are required to properly correct the issue.
apache · Feb 3, 2019 · eee0ba1 · eee0ba1
1 parent f6b9efc
commit eee0ba1
Show file tree

Hide file tree

Showing 6 changed files with 244 additions and 6 deletions.
diff --git a/src/it/MCHECKSTYLE-365/invoker.properties b/src/it/MCHECKSTYLE-365/invoker.properties
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you 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.
+
+invoker.goals=clean site
+# checkstyleRules requires Maven 3+
+invoker.maven.version = 3.0+
diff --git a/src/it/MCHECKSTYLE-365/pom.xml b/src/it/MCHECKSTYLE-365/pom.xml
@@ -0,0 +1,111 @@
+<?xml version="1.0"?>
+<!--
+  ~ Licensed to the Apache Software Foundation (ASF) under one
+  ~ or more contributor license agreements.  See the NOTICE file
+  ~ distributed with this work for additional information
+  ~ regarding copyright ownership.  The ASF licenses this file
+  ~ to you 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
+  ~
+  -->
+<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>com.basistech</groupId>
+    <artifactId>MCHECKSTYLE-365</artifactId>
+    <description>Tests that the report generates the correct counts for duplicate rules with different severities.</description>
+    <version>1.0-SNAPSHOT</version>
+    <properties>
+        <maven.compiler.source>1.8</maven.compiler.source>
+        <maven.compiler.target>1.8</maven.compiler.target>
+        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+    </properties>
+    <build>
+        <pluginManagement>
+            <plugins>
+                <plugin>
+                    <artifactId>maven-site-plugin</artifactId>
+                    <version>3.7.1</version>
+                </plugin>
+                <plugin>
+                    <artifactId>maven-checkstyle-plugin</artifactId>
+                    <version>@pom.version@</version>
+                    <configuration>
+                        <checkstyleRules>
+                            <module name="Checker">
+                                <property name="charset" value="UTF-8" />
+
+                                <property name="fileExtensions" value="java,properties"/>
+
+                                <!-- rules using a custom message string, which changes the behaviour -->
+                                <module name="RegexpSingleline">
+                                    <!-- Check for trailing whitespace in .java files - default severity of error -->
+                                    <property name="format" value="\s+$" />
+                                    <property name="fileExtensions" value="java"/>
+                                    <property name="message" value="message"/>
+                                </module>
+                                <module name="RegexpSingleline">
+                                    <!-- Check for trailing whitespace in .properties files - severity of info -->
+                                    <property name="format" value="\s+$" />
+                                    <property name="fileExtensions" value="properties"/>
+                                    <property name="severity" value="info" />
+                                    <property name="message" value="message"/>
+                                </module>
+
+                                <!-- rules without a custom message string -->
+                                <module name="FileTabCharacter">
+                                    <!-- Check for tabs in .java files - default severity of error -->
+                                    <property name="eachLine" value="false"/>
+                                    <property name="fileExtensions" value="java"/>
+                                </module>
+                                <module name="FileTabCharacter">
+                                    <!-- Check for tabs in .properties files - severity of info -->
+                                    <property name="eachLine" value="false"/>
+                                    <property name="fileExtensions" value="properties"/>
+                                    <property name="severity" value="info" />
+                                </module>
+
+                            </module>
+                        </checkstyleRules>
+                    </configuration>
+                    <executions>
+                        <execution>
+                            <goals>
+                                <goal>check</goal>
+                            </goals>
+                        </execution>
+                    </executions>
+                </plugin>
+            </plugins>
+        </pluginManagement>
+        <plugins>
+            <plugin>
+                <artifactId>maven-checkstyle-plugin</artifactId>
+            </plugin>
+        </plugins>
+    </build>
+
+    <reporting>
+        <plugins>
+            <plugin>
+                <artifactId>maven-checkstyle-plugin</artifactId>
+                <reportSets>
+                     <reportSet>
+                         <reports>
+                             <report>checkstyle</report>
+                         </reports>
+                     </reportSet>
+                </reportSets>
+            </plugin>
+        </plugins>
+    </reporting>
+</project>
+
diff --git a/src/it/MCHECKSTYLE-365/src/main/java/cat/App.java b/src/it/MCHECKSTYLE-365/src/main/java/cat/App.java
@@ -0,0 +1,37 @@
+package cat;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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.
+ */
+
+/**
+ * Hello world!
+ *
+ */
+public class App
+{
+    public static void main( String[] args )
+    {
+        Object[] bucket = new Object[12345];
+        String y = args[0];
+        Object x = bucket[ Math.abs(y.hashCode()) % bucket.length];
+        System.out.println( "Hello World!"  + x);
+        x = bucket[ y.hashCode() % bucket.length];
+        System.out.println( "Hello World!"  + x);
+    }
+}
diff --git a/src/it/MCHECKSTYLE-365/src/main/resources/test.properties b/src/it/MCHECKSTYLE-365/src/main/resources/test.properties
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you 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.
+spaces_property=text with trailing spaces after it  
+tabs_property=text with a	tab in the middle of it
diff --git a/src/it/MCHECKSTYLE-365/verify.groovy b/src/it/MCHECKSTYLE-365/verify.groovy
@@ -0,0 +1,45 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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.
+ */
+def htmlReportFile = new File(basedir, 'target/site/checkstyle.html');
+assert htmlReportFile.exists();
+
+/*
+ * These two test cases will both return 3 if the bug isn't fixed
+ * (1 for the violation, and 1 each for the 2 rules).
+ *
+ * If fixed, the count should be 2 (1 for the violation, and 1 for
+ * the Rule summary)
+ *
+ * Note that the angle-brackets at either end are to count only
+ * the user visible text as opposed to the occurances in the URLs.
+ *
+ * A more robust solution would be to parse the HTML, but that's
+ * a lot more effort than required to actually verify the behaviour.
+ */
+
+// Case with no custom messages
+def numberOfOccurancesOfFileTabCharacter = htmlReportFile.text.count(">FileTabCharacter<")
+assert 2 == numberOfOccurancesOfFileTabCharacter;
+
+// Case with custom messages
+def numberOfOccurancesOfRegexpSingleline = htmlReportFile.text.count(">RegexpSingleline<");
+assert 2 == numberOfOccurancesOfRegexpSingleline;
+
+return true;
diff --git a/src/main/java/org/apache/maven/plugins/checkstyle/CheckstyleReportGenerator.java b/src/main/java/org/apache/maven/plugins/checkstyle/CheckstyleReportGenerator.java
@@ -445,17 +445,23 @@ public boolean matchRule( AuditEvent event, String ruleName, String expectedMess
             // http://java.sun.com/j2se/1.4.2/docs/api/java/text/MessageFormat.html
             String msgWithoutSingleQuote = StringUtils.replace( expectedMessage, "'", "" );
 
-            return expectedMessage.equals( event.getMessage() ) || msgWithoutSingleQuote.equals( event.getMessage() );
+            if ( ! ( expectedMessage.equals( event.getMessage() )
+                || msgWithoutSingleQuote.equals( event.getMessage() ) ) )
+            {
+                return false;
+            }
         }
         // Check the severity. This helps to distinguish between
         // different configurations for the same rule, where each
-        // configuration has a different severity, like JavadocMetod.
+        // configuration has a different severity, like JavadocMethod.
         // See also https://issues.apache.org/jira/browse/MCHECKSTYLE-41
         if ( expectedSeverity != null )
         {
-            return expectedSeverity.equals( event.getSeverityLevel().getName() );
+            if ( !expectedSeverity.equals( event.getSeverityLevel().getName() ) )
+            {
+                return false;
+            }
         }
-
         return true;
     }
 
@@ -801,8 +807,9 @@ private void sortConfiguration( List<ConfReference> result, Configuration config
             else
             {
                 String fixedmessage = getConfigAttribute( childConfig, null, "message", null );
-                // Grab the severity from the rule configuration, use null as default value
-                String configSeverity = getConfigAttribute( childConfig, null, "severity", null );
+                // Grab the severity from the rule configuration, use "error" as default value (as is 
+                // done where the rule severity is displayed otherwise we miscount error violations)
+                String configSeverity = getConfigAttribute( childConfig, null, "severity", "error" );
 
                 // count rule violations
                 long violations = 0;