Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Add Static code analysis tool to ESH build #3995

Merged
merged 5 commits into from
Sep 5, 2017
Merged

Add Static code analysis tool to ESH build #3995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f2a7179
Static analysis added to ESH build
Sep 4, 2017
5638566
Remove unneeded repositories
Sep 4, 2017
b2a8293
Remove unused profile
Sep 4, 2017
8636a8e
Refine the documentation about the tool
Sep 4, 2017
12c5234
Change to JCenter repository
Sep 5, 2017
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 .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ before_install: echo "MAVEN_OPTS='-Xms1g -Xmx2g -XX:PermSize=512m -XX:MaxPermSiz
#$ mvn test -B

install:
- echo 'mvn clean install -B -V 1> .build.stdout 2> .build.stderr' > .build.sh
- echo 'mvn clean install -B -V -Dfindbugs.skip=true 1> .build.stdout 2> .build.stderr' > .build.sh
- chmod 0755 .build.sh
script:
- travis_wait 60 ./.build.sh
Expand Down
2 changes: 1 addition & 1 deletion ...e/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/items/ItemUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public static void assertValidItemName(String itemName) throws IllegalArgumentEx

public static State convertToAcceptedState(State state, Item item) {
if (state == null) {
LOGGER.error("A conversion of null was requested", new NullPointerException("state should not be null"));
LOGGER.error("A conversion of null was requested", new NullPointerException("state should not be null")); // NOPMD
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the exact PMD warning that is suppressed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #3888

return UnDefType.NULL;
}

Expand Down
8 changes: 8 additions & 0 deletions docs/documentation/development/guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,11 @@ Note that this list also serves as a checklist for code reviews on pull requests
- ```error``` logging should only be used to inform the user that something is tremendously wrong in his setup, the system cannot function normally anymore, and there is a need for immediate action. It should also be used if some code fails irrecoverably and the user should report it as a severe bug.
1. For bindings, you should NOT log errors, if e.g. connections are dropped - this is considered to be an external problem and from a system perspective to be a normal and expected situation. The correct way to inform users about such events is to update the Thing status accordingly. Note that all events (including Thing status events) are anyhow already logged.
1. Likewise, bundles that accept external requests (such as servlets) must not log errors or warnings if incoming requests are incorrect. Instead, appropriate error responses should be returned to the caller.

## Static code analysis

The Eclipse SmartHome Maven build includes [tooling for static code analysis](https://github.com/openhab/static-code-analysis) that will validate your code against the Coding Guidelines and some additional best practices. Information about the checks can be found [here](https://github.com/openhab/static-code-analysis/blob/master/docs/included-checks.md).

The tool will generate an individual report for each bundle that you can find in `path/to/bundle/target/code-analysis/report.html` file and a report for the whole build that contains links to the individual reports in the `target/summary_report.html`. The tool categorizes the found issues by priority: 1(error),2(warning) or 3(info). If any error is found within your code the Maven build will end with failure. You will receive detailed information (path to the file, line and message) listing all problems with Priority 1 on the console.

Please fix all the Priority 1 issues and all issues with Priority 2 and 3 that are relevant (if you have any doubt don't hesitate to ask). Re-run the build to confirm that the checks are passing.
76 changes: 76 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
<jdt-annotations.version>2.1.0</jdt-annotations.version>
<build.helper.maven.plugin.version>1.8</build.helper.maven.plugin.version>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<sat.version>0.3.0</sat.version>
</properties>

<packaging>pom</packaging>
Expand Down Expand Up @@ -469,6 +470,81 @@
</plugins>
</build>
</profile>
<!-- static code analysis profiles -->
<profile>
<id>check</id>
<activation>
<property>
<name>!skipChecks</name>
</property>
</activation>
<build>
<pluginManagement>
<plugins>
<plugin>
<groupId>org.openhab.tools</groupId>
<artifactId>static-code-analysis</artifactId>
<version>${sat.version}</version>
<executions>
<execution>
<phase>verify</phase>
<goals>
<goal>checkstyle</goal>
<goal>pmd</goal>
<goal>findbugs</goal>
<goal>report</goal>
</goals>
</execution>
</executions>
<configuration>
<checkstyleProperties>tools/static-code-analysis/checkstyle/ruleset.properties</checkstyleProperties>
<checkstyleFilter>tools/static-code-analysis/checkstyle/suppressions.xml</checkstyleFilter>
<findbugsExclude>tools/static-code-analysis/findbugs/suppressions.xml</findbugsExclude>
</configuration>
</plugin>
</plugins>
</pluginManagement>
</build>
</profile>
<profile>
<id>check-bundles</id>
<activation>
<file>
<exists>META-INF/MANIFEST.MF</exists>
</file>
</activation>
<build>
<plugins>
<plugin>
<groupId>org.openhab.tools</groupId>
<artifactId>static-code-analysis</artifactId>
<version>${sat.version}</version>
</plugin>
</plugins>
</build>
</profile>
</profiles>

<pluginRepositories>
<pluginRepository>
<id>jcenter</id>
<url>https://jcenter.bintray.com</url>
</pluginRepository>
</pluginRepositories>

<repositories>
<repository>
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the pluginRepository enough? Why do we need it as a dependency repository as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I haven't taught about that much until you asked.
The tool(as a Maven plugin) internally starts the Maven plugins for Checkstyle, PMD and FindBugs and adds itself as a dependency to these plugins, so the included custom checks that are contained there are available in the classpath.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test if it also works without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tested it, it is not working without this repository.

<id>jcenter</id>
<name>JCenter Repository</name>
<url>https://jcenter.bintray.com</url>
<releases>
<enabled>true</enabled>
<updatePolicy>never</updatePolicy>
</releases>
<snapshots>
<enabled>false</enabled>
</snapshots>
</repository>
</repositories>

</project>
3 changes: 3 additions & 0 deletions tools/static-code-analysis/checkstyle/ruleset.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
checkstyle.aboutHtmlCheck.url=https://eclipse.org/legal/epl/about.html
checkstyle.headerCheck.content=^/\\*\\*$\\n^ \\* Copyright \\(c\\) \\d\\d\\d\\d-\\d\\d\\d\\d by the respective copyright holders\\.$\\n^ \\* All rights reserved\\. This program and the accompanying materials$\\n^ \\* are made available under the terms of the Eclipse Public License v1\\.0$\\n^ \\* which accompanies this distribution, and is available at$\\n^ \\* http://www.eclipse.org/legal/epl\\-v10\\.html$
checkstyle.bundleVendorCheck.allowedValues=Eclipse.org/SmartHome
22 changes: 22 additions & 0 deletions tools/static-code-analysis/checkstyle/suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE suppressions PUBLIC
"-//Puppy Crawl//DTD Suppressions 1.1//EN"
"http://www.puppycrawl.com/dtds/suppressions_1_1.dtd">

<suppressions>
<!-- These suppressions define which files to be suppressed for which checks. -->
<suppress files=".+[\\/]internal[\\/].+\.java" checks="JavadocType|JavadocVariable|JavadocMethod|JavadocFilterCheck"/>
<suppress files=".+DTO\.java" checks="JavadocType|JavadocVariable|JavadocMethod|JavadocFilterCheck" />
<suppress files=".+Impl\.java" checks="JavadocType|JavadocVariable|JavadocMethod|JavadocFilterCheck"/>
<!-- All generated files will skip the author tag check -->
<suppress files=".+[\\/]gen[\\/].+\.java" checks="AuthorTagCheck"/>
<!-- Some checks will be supressed for test bundles -->
<suppress files=".+.test[\\/].+" checks="RequireBundleCheck|OutsideOfLibExternalLibrariesCheck|ManifestExternalLibrariesCheck|BuildPropertiesExternalLibrariesCheck"/>

<!-- Eclipse SmartHome specific suppressions-->
<!-- These bundles are generated trough XText -->
<suppress files=".+org.eclipse.smarthome.model.+|.+org.eclipse.smarthome.designer.+" checks="RequireBundleCheck|ExportInternalPackageCheck|ManifestPackageVersionCheck|ImportExportedPackagesCheck"/>
<!-- Some source files have different headers -->
<suppress files=".+org.eclipse.smarthome.automation.+" checks="HeaderCheck"/>
<suppress files=".+org.eclipse.smarthome.config.dispatch.test.+" checks="ServiceComponentManifestCheck"/>
</suppressions>
32 changes: 32 additions & 0 deletions tools/static-code-analysis/findbugs/suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?xml version="1.0" encoding="UTF-8"?>
<FindBugsFilter>
<!-- Groovy files produce a lot of warnings and will be ignored -->
<Match>
<Source name="~.*\.groovy" />
</Match>
<!-- Excludes all bugs with priority higher than 4 -->
<Match>
<Rank value="4"/>
<Not>
<Bug pattern="SLF4J_LOGGER_SHOULD_BE_NON_STATIC"/>
</Not>
</Match>
<!-- This pattern is not wanted as it reports usage of Throwable.getMessage() as argument to SLF4G logger -->
<Match>
<Bug pattern="SLF4J_MANUALLY_PROVIDED_MESSAGE"/>
</Match>
<!-- Allow util classes to have static loggers -->
<Match>
<Class name="~.*Utils"/>
<Bug pattern="SLF4J_LOGGER_SHOULD_BE_NON_STATIC"/>
</Match>
<Match>
<Class name="~.*Util"/>
<Bug pattern="SLF4J_LOGGER_SHOULD_BE_NON_STATIC"/>
</Match>
<!-- The format string is parameter, it can't be constant -->
<Match>
<Class name="org.eclipse.smarthome.model.script.actions.LogAction"/>
<Bug pattern="SLF4J_FORMAT_SHOULD_BE_CONST"/>
</Match>
</FindBugsFilter>