From 8979c381c5ba57f34190a778733b9170cafe82d1 Mon Sep 17 00:00:00 2001 From: Filipe Esperandio Date: Mon, 22 Jan 2018 10:21:54 -0200 Subject: [PATCH 1/6] Upgrade PMD to version 6 --- bin/install-pmd.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/install-pmd.sh b/bin/install-pmd.sh index ba14f96..95cf33a 100755 --- a/bin/install-pmd.sh +++ b/bin/install-pmd.sh @@ -4,7 +4,7 @@ set -euo pipefail LIB_DIR=/usr/src/app/lib download_pmd() { - URL="https://github.com/pmd/pmd/releases/download/pmd_releases/5.8.1/pmd-bin-5.8.1.zip" + URL="https://github.com/pmd/pmd/releases/download/pmd_releases/6.0.1/pmd-bin-6.0.1.zip" wget -O pmd.zip $URL } From 3e5a8f68d21b0c604419aa7853c1c6461a7fc7f6 Mon Sep 17 00:00:00 2001 From: Filipe Esperandio Date: Tue, 23 Jan 2018 14:58:13 -0200 Subject: [PATCH 2/6] New version has different output values --- test/SanityCheckTest.groovy | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/SanityCheckTest.groovy b/test/SanityCheckTest.groovy index 19bafd0..1c4e5f9 100644 --- a/test/SanityCheckTest.groovy +++ b/test/SanityCheckTest.groovy @@ -20,7 +20,6 @@ class SanityCheckTest { def (proc, out, err) = execute("/usr/src/app/pmd --codeFolder=/usr/src/app/fixtures/default --configFile=/usr/src/app/fixtures/default/config.json") assert !out.toString().isEmpty() - assert err.toString().isEmpty() assert proc.exitValue() == 0 } @@ -29,9 +28,12 @@ class SanityCheckTest { def (proc, out, _err) = execute("/usr/src/app/pmd --codeFolder=/usr/src/app/fixtures/specified_file --configFile=/usr/src/app/fixtures/specified_file/config.new.json") def (procOld, outOld, _errOld) = execute("/usr/src/app/pmd --codeFolder=/usr/src/app/fixtures/specified_file --configFile=/usr/src/app/fixtures/specified_file/config.old.json") - assert proc.exitValue() == procOld.exitValue() - assert out.toString().equals(outOld.toString()) + def expectedIssue = "Avoid modifying an outer loop incrementer in an inner loop for update expression" + assert proc.exitValue() == 0 + assert proc.exitValue() == procOld.exitValue() + assert out.toString().contains(expectedIssue) + assert outOld.toString().contains(expectedIssue) } @Test From b66cefa263c3fcbbb016b67ab74bfca17a0aed2b Mon Sep 17 00:00:00 2001 From: Filipe Esperandio Date: Tue, 23 Jan 2018 14:54:15 -0200 Subject: [PATCH 3/6] Use new rules xml instead of group name, as they became incompatible --- fixtures/specified_file/rules.xml | 870 ++---------------------------- java-basic-default-ruleset.xml | 42 ++ src/Config.groovy | 2 +- test/ConfigTest.groovy | 2 +- 4 files changed, 74 insertions(+), 842 deletions(-) create mode 100644 java-basic-default-ruleset.xml diff --git a/fixtures/specified_file/rules.xml b/fixtures/specified_file/rules.xml index feff72a..006ec43 100644 --- a/fixtures/specified_file/rules.xml +++ b/fixtures/specified_file/rules.xml @@ -8,845 +8,35 @@ The Basic ruleset contains a collection of good practices which should be followed. - - -Avoid jumbled loop incrementers - its usually a mistake, and is confusing even if intentional. - - 3 - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + - - -Some for loops can be simplified to while loops, this makes them more concise. - - 3 - - - - 1] - [not(LocalVariableDeclaration)] - [not(ForInit)] - [not(ForUpdate)] - [not(Type and Expression and Statement)] - ]]> - - - - - - - - - - -Override both public boolean Object.equals(Object other), and public int Object.hashCode(), or override neither. Even if you are inheriting a hashCode() from a parent class, consider implementing hashCode and explicitly delegating to your superclass. - - 3 - - - - - - - -Partially created objects can be returned by the Double Checked Locking pattern when used in Java. -An optimizing JRE may assign a reference to the baz variable before it calls the constructor of the object the -reference points to. - -Note: With Java 5, you can make Double checked locking work, if you declare the variable to be `volatile`. - -For more details refer to: http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double.html -or http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html - - 1 - - - - - - - -Avoid returning from a finally block, this can discard exceptions. - - 3 - - - - - - - - - - - - - - -Do not use "if" statements whose conditionals are always true or always false. - - 3 - - - - - - - - - - - - - - -Avoid instantiating Boolean objects; you can reference Boolean.TRUE, Boolean.FALSE, or call Boolean.valueOf() instead. - - 2 - - - - - - - -Sometimes two consecutive 'if' statements can be consolidated by separating their conditions with a boolean short-circuit operator. - - 3 - - - - - - - - - - - - - - -When deriving an array of a specific class from your Collection, one should provide an array of -the same class as the parameter of the toArray() method. Doing otherwise you will will result -in a ClassCastException. - - 3 - - - - - - - - - - - - - - - -One might assume that the result of "new BigDecimal(0.1)" is exactly equal to 0.1, but it is actually -equal to .1000000000000000055511151231257827021181583404541015625. -This is because 0.1 cannot be represented exactly as a double (or as a binary fraction of any finite -length). Thus, the long value that is being passed in to the constructor is not exactly equal to 0.1, -appearances notwithstanding. - -The (String) constructor, on the other hand, is perfectly predictable: 'new BigDecimal("0.1")' is -exactly equal to 0.1, as one would expect. Therefore, it is generally recommended that the -(String) constructor be used in preference to this one. - - 3 - - - - - - - - - - - - - - - -The null check here is misplaced. If the variable is null a NullPointerException will be thrown. -Either the check is useless (the variable will never be "null") or it is incorrect. - - 3 - - - - - - - - - - - - - - - - -Avoid using java.lang.ThreadGroup; although it is intended to be used in a threaded environment -it contains methods that are not thread-safe. - - 3 - - - - - - - - - - - - - - -The null check is broken since it will throw a NullPointerException itself. -It is likely that you used || instead of && or vice versa. - - 2 - - - - - - - -Don't create instances of already existing BigInteger (BigInteger.ZERO, BigInteger.ONE) and -for Java 1.5 onwards, BigInteger.TEN and BigDecimal (BigDecimal.ZERO, BigDecimal.ONE, BigDecimal.TEN) - - 3 - - - - - - - - - - 3 - - - - - - - - - - 3 - - - - - - - - - - - - - 3 - - - - - - - - - - 2 - - - - - - - No need to explicitly extend Object. - 4 - - - - - - - - - - - - - - The skip() method may skip a smaller number of bytes than requested. Check the returned value to find out if it was the case or not. - 3 - - - - - - - - - - 2 - - 25) { - break; - } -} - ]]> - - - - - -Explicitly calling Thread.run() method will execute in the caller's thread of control. Instead, call Thread.start() for the intended behavior. - - 4 - - - - - - - - - - - - - - -Don't use floating point for loop indices. If you must use floating point, use double -unless you're certain that float provides enough precision and you have a compelling -performance need (space or time). - - 3 - - - - - - - - - - - - - - - - - 3 - - - - - - - - - - diff --git a/java-basic-default-ruleset.xml b/java-basic-default-ruleset.xml new file mode 100644 index 0000000..006ec43 --- /dev/null +++ b/java-basic-default-ruleset.xml @@ -0,0 +1,42 @@ + + + + +The Basic ruleset contains a collection of good practices which should be followed. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/Config.groovy b/src/Config.groovy index a7dbdba..d233253 100644 --- a/src/Config.groovy +++ b/src/Config.groovy @@ -2,7 +2,7 @@ import groovy.json.JsonSlurper import groovy.util.FileNameFinder class Config { - static String DEFAULT_RULES = "java-basic" + static String DEFAULT_RULES = new File("java-basic-default-ruleset.xml").absolutePath def args def appContext def parsedConfig diff --git a/test/ConfigTest.groovy b/test/ConfigTest.groovy index 54c461f..88de746 100644 --- a/test/ConfigTest.groovy +++ b/test/ConfigTest.groovy @@ -5,7 +5,7 @@ class ConfigTest { @Test public void defaultRuleSet() { def config = new Config([configFile: "/usr/src/app/fixtures/default/config.json", codeFolder: "/usr/src/app/fixtures/default"]) - assertEquals "java-basic", config.ruleSet() + assertEquals "/usr/src/app/java-basic-default-ruleset.xml", config.ruleSet() } @Test From a97d44dffee8428a9d548a5c2f2ffd3669866217 Mon Sep 17 00:00:00 2001 From: Filipe Esperandio Date: Tue, 23 Jan 2018 14:55:19 -0200 Subject: [PATCH 4/6] Speedup analysis with builtin cache --- src/main.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.groovy b/src/main.groovy index bb5536b..019e0f1 100644 --- a/src/main.groovy +++ b/src/main.groovy @@ -24,6 +24,6 @@ class Main { System.exit(0) } - execute("/usr/src/app/lib/pmd/bin/run.sh pmd -filelist ${config.filesListPath()} -f codeclimate -R ${config.ruleSet()} -failOnViolation false") + execute("/usr/src/app/lib/pmd/bin/run.sh pmd -cache /tmp/pmd-cache -filelist ${config.filesListPath()} -f codeclimate -R ${config.ruleSet()} -failOnViolation false") } } From 3ae7e7549efdf9d35cc5711f1ceecfe409b7babb Mon Sep 17 00:00:00 2001 From: Filipe Esperandio Date: Tue, 23 Jan 2018 14:56:40 -0200 Subject: [PATCH 5/6] Redirect warnings to stderr --- src/OutputParser.groovy | 28 ++++++++++++++++++++++++++++ src/main.groovy | 3 ++- test/OutputParserTest.groovy | 34 ++++++++++++++++++++++++++++++++++ test/SanityCheckTest.groovy | 22 ++++++++++++++++++---- 4 files changed, 82 insertions(+), 5 deletions(-) create mode 100644 src/OutputParser.groovy create mode 100644 test/OutputParserTest.groovy diff --git a/src/OutputParser.groovy b/src/OutputParser.groovy new file mode 100644 index 0000000..8795f49 --- /dev/null +++ b/src/OutputParser.groovy @@ -0,0 +1,28 @@ + +class OutputParser { + public def out + public def err + + OutputParser(out, err) { + this.out = new OutStreamProcessor(out); + this.err = err + } + + class OutStreamProcessor extends PrintStream { + public OutStreamProcessor(out) { + super(out); + } + + public void print(String line) { + if(isJson(line)) { + super.print(line) + } else { + this.err.print(line) + } + } + + boolean isJson(txt) { + return txt.startsWith("{"); + } + } +} diff --git a/src/main.groovy b/src/main.groovy index 019e0f1..d91d2f2 100644 --- a/src/main.groovy +++ b/src/main.groovy @@ -2,11 +2,12 @@ import groovy.util.* class Main { static def execute(command) { + OutputParser parser = new OutputParser(System.out, System.err) ProcessBuilder builder = new ProcessBuilder(command.split(' ')) def env = builder.environment() env.put("JAVA_OPTS", "-XX:MinHeapFreeRatio=10 -XX:MaxHeapFreeRatio=30") Process process = builder.start() - process.consumeProcessOutput(System.out, System.err) + process.consumeProcessOutput(parser.out, parser.err) process.waitFor() System.exit(process.exitValue()) } diff --git a/test/OutputParserTest.groovy b/test/OutputParserTest.groovy new file mode 100644 index 0000000..c7e261b --- /dev/null +++ b/test/OutputParserTest.groovy @@ -0,0 +1,34 @@ +import static org.junit.Assert.* +import org.junit.* + +class OutputParserTest { + def out + def err + def parser + + @Before + public void setup() { + out = new ByteArrayOutputStream() + err = new ByteArrayOutputStream() + + def outStream = new PrintStream(out) + def errStream = new PrintStream(err) + + parser = new OutputParser(outStream, errStream) + } + + @Test + public void redirectNonJsonInput() { + parser.out.print("This is a warning") + assert out.toString("UTF-8").isEmpty() + assert "This is a warning", err.toString("UTF-8") + } + + @Test + public void printJsonLines() { + parser.out.print("{}") + assert err.toString("UTF-8").isEmpty() + assert "{}", out.toString("UTF-8") + } +} + diff --git a/test/SanityCheckTest.groovy b/test/SanityCheckTest.groovy index 1c4e5f9..9d25377 100644 --- a/test/SanityCheckTest.groovy +++ b/test/SanityCheckTest.groovy @@ -4,15 +4,29 @@ import groovy.util.FileNameFinder import static org.junit.Assert.* import org.junit.* +class CustomIO { + def byteStream + def printStream + + CustomIO() { + byteStream = new ByteArrayOutputStream() + printStream = new PrintStream(byteStream) + } + + public String toString() { + return byteStream.toString("UTF-8") + } +} + class SanityCheckTest { def execute(command) { def proc = command.execute() - def out = new StringBuffer() - def err = new StringBuffer() + def outIO = new CustomIO() + def errIO = new CustomIO() - proc.waitForProcessOutput(out, err) + proc.waitForProcessOutput(outIO.printStream, errIO.printStream) - return [proc, out, err] + return [proc, outIO, errIO] } @Test From 77a353d7987d6c6601959e4c8c58c28ad3c508c2 Mon Sep 17 00:00:00 2001 From: Filipe Esperandio Date: Wed, 24 Jan 2018 15:07:39 -0200 Subject: [PATCH 6/6] Hardcode ruleset as runtime directory changes --- src/Config.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Config.groovy b/src/Config.groovy index d233253..554c5a2 100644 --- a/src/Config.groovy +++ b/src/Config.groovy @@ -2,7 +2,7 @@ import groovy.json.JsonSlurper import groovy.util.FileNameFinder class Config { - static String DEFAULT_RULES = new File("java-basic-default-ruleset.xml").absolutePath + static String DEFAULT_RULES = "/usr/src/app/java-basic-default-ruleset.xml" def args def appContext def parsedConfig