From dc44ea19eb9c326cfeba4f13b298b83016520547 Mon Sep 17 00:00:00 2001 From: Paul O'Neill Date: Tue, 28 Jun 2016 22:35:11 +0100 Subject: [PATCH] Issue35 (#39) Fix #35 with batching of files issued to TsLint --- .gitignore | 3 +- pom.xml | 4 +- .../com/pablissimo/sonar/TsLintExecutor.java | 4 +- .../pablissimo/sonar/TsLintExecutorImpl.java | 90 ++++++++++++++---- .../com/pablissimo/sonar/TsLintParser.java | 2 +- .../pablissimo/sonar/TsLintParserImpl.java | 4 +- .../com/pablissimo/sonar/TsLintSensor.java | 74 ++++++++++----- .../sonar/TsLintExecutorImplTest.java | 91 ++++++++++++++++++- .../pablissimo/sonar/TsLintParserTest.java | 10 +- .../pablissimo/sonar/TsLintSensorTest.java | 10 +- 10 files changed, 232 insertions(+), 60 deletions(-) diff --git a/.gitignore b/.gitignore index 69cdaf6..254aea7 100644 --- a/.gitignore +++ b/.gitignore @@ -47,4 +47,5 @@ Desktop.ini hs_err_pid* #Custom stuff -sample.tslint.json \ No newline at end of file +sample.tslint.json +.idea diff --git a/pom.xml b/pom.xml index f2af457..043460d 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ com.pablissimo.sonar sonar-typescript-plugin sonar-plugin - 0.4-SNAPSHOT + 0.6-SNAPSHOT TypeScript Analyse TypeScript projects @@ -14,7 +14,7 @@ scm:git:git@github.com:Pablissimo/SonarTsPlugin.git scm:git:git@github.com:Pablissimo/SonarTsPlugin.git https://github.com/Pablissimo/SonarTsPlugin - 0.4 + 0.6 diff --git a/src/main/java/com/pablissimo/sonar/TsLintExecutor.java b/src/main/java/com/pablissimo/sonar/TsLintExecutor.java index 7d72b70..8ce8519 100644 --- a/src/main/java/com/pablissimo/sonar/TsLintExecutor.java +++ b/src/main/java/com/pablissimo/sonar/TsLintExecutor.java @@ -1,5 +1,7 @@ package com.pablissimo.sonar; +import java.util.List; + public interface TsLintExecutor { - String execute(String pathToTsLint, String configFile, String rulesDir, String file, Integer timeoutMs); + String execute(String pathToTsLint, String configFile, String rulesDir, List files, Integer timeoutMs); } diff --git a/src/main/java/com/pablissimo/sonar/TsLintExecutorImpl.java b/src/main/java/com/pablissimo/sonar/TsLintExecutorImpl.java index 7790dce..6797cf5 100644 --- a/src/main/java/com/pablissimo/sonar/TsLintExecutorImpl.java +++ b/src/main/java/com/pablissimo/sonar/TsLintExecutorImpl.java @@ -1,5 +1,8 @@ package com.pablissimo.sonar; +import java.util.ArrayList; +import java.util.List; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.sonar.api.utils.command.Command; @@ -7,27 +10,61 @@ import org.sonar.api.utils.command.StreamConsumer; public class TsLintExecutorImpl implements TsLintExecutor { + public static final int MAX_COMMAND_LENGTH = 4096; private static final Logger LOG = LoggerFactory.getLogger(TsLintExecutorImpl.class); private StringBuilder stdOut; private StringBuilder stdErr; - public String execute(String pathToTsLint, String configFile, String rulesDir, String file, Integer timeoutMs) { - LOG.info("TsLint executing for " + file); - Command command = Command.create("node"); - command.addArgument(pathToTsLint); - command.addArgument("--format"); - command.addArgument("json"); - + private static Command getBaseCommand(String pathToTsLint, String configFile, String rulesDir) { + Command command = + Command + .create("node") + .addArgument(pathToTsLint) + .addArgument("--format") + .addArgument("json"); + if (rulesDir != null && rulesDir.length() > 0) { - command.addArgument("--rules-dir"); - command.addArgument(rulesDir); + command + .addArgument("--rules-dir") + .addArgument(rulesDir); + } + + command + .addArgument("--config") + .addArgument(configFile) + .setNewShell(false); + + return command; + } + + public String execute(String pathToTsLint, String configFile, String rulesDir, List files, Integer timeoutMs) { + // New up a command that's everything we need except the files to process + // We'll use this as our reference for chunking up files + int baseCommandLength = getBaseCommand(pathToTsLint, configFile, rulesDir).toCommandLine().length(); + int availableForBatching = MAX_COMMAND_LENGTH - baseCommandLength; + + List> batches = new ArrayList>(); + List currentBatch = new ArrayList(); + batches.add(currentBatch); + + int currentBatchLength = 0; + for (int i = 0; i < files.size(); i++) { + String nextPath = files.get(i).trim(); + + // +1 for the space we'll be adding between filenames + if (currentBatchLength + nextPath.length() + 1 > availableForBatching) { + // Too long to add to this batch, create new + currentBatch = new ArrayList(); + currentBatchLength = 0; + batches.add(currentBatch); + } + + currentBatch.add(nextPath); + currentBatchLength += nextPath.length() + 1; } - - command.addArgument("--config"); - command.addArgument(configFile); - command.addArgument(file.trim()); - command.setNewShell(false); + + LOG.debug("Split " + files.size() + " files into " + batches.size() + " batches for processing"); this.stdOut = new StringBuilder(); this.stdErr = new StringBuilder(); @@ -35,7 +72,7 @@ public String execute(String pathToTsLint, String configFile, String rulesDir, S StreamConsumer stdOutConsumer = new StreamConsumer() { public void consumeLine(String line) { LOG.trace("TsLint Out: " + line); - stdOut.append(line + "\n"); + stdOut.append(line); } }; @@ -46,11 +83,28 @@ public void consumeLine(String line) { } }; - this.createExecutor().execute(command, stdOutConsumer, stdErrConsumer, timeoutMs); + for (int i = 0; i < batches.size(); i++) { + List thisBatch = batches.get(i); + Command thisCommand = getBaseCommand(pathToTsLint, configFile, rulesDir); + + for (int fileIndex = 0; fileIndex < thisBatch.size(); fileIndex++) { + thisCommand.addArgument(thisBatch.get(fileIndex)); + } + + LOG.debug("Executing TsLint with command: " + thisCommand.toCommandLine()); + + // Timeout is specified per file, not per batch (which can vary a lot) + // so multiply it up + this.createExecutor().execute(thisCommand, stdOutConsumer, stdErrConsumer, timeoutMs * thisBatch.size()); + } - return stdOut.toString(); + String rawOutput = stdOut.toString(); + + // TsLint returns nonsense for its JSON output when faced with multiple files + // so we need to fix it up before we do anything else + return "[" + rawOutput.replaceAll("\\]\\[", "],[") + "]"; } - + protected CommandExecutor createExecutor() { return CommandExecutor.create(); } diff --git a/src/main/java/com/pablissimo/sonar/TsLintParser.java b/src/main/java/com/pablissimo/sonar/TsLintParser.java index e5d7b07..54f7f42 100644 --- a/src/main/java/com/pablissimo/sonar/TsLintParser.java +++ b/src/main/java/com/pablissimo/sonar/TsLintParser.java @@ -3,5 +3,5 @@ import com.pablissimo.sonar.model.TsLintIssue; public interface TsLintParser { - TsLintIssue[] parse(String toParse); + TsLintIssue[][] parse(String toParse); } diff --git a/src/main/java/com/pablissimo/sonar/TsLintParserImpl.java b/src/main/java/com/pablissimo/sonar/TsLintParserImpl.java index 2bfe063..320be0b 100644 --- a/src/main/java/com/pablissimo/sonar/TsLintParserImpl.java +++ b/src/main/java/com/pablissimo/sonar/TsLintParserImpl.java @@ -5,10 +5,10 @@ import com.pablissimo.sonar.model.TsLintIssue; public class TsLintParserImpl implements TsLintParser { - public TsLintIssue[] parse(String toParse) { + public TsLintIssue[][] parse(String toParse) { GsonBuilder builder = new GsonBuilder(); Gson gson = builder.create(); - return gson.fromJson(toParse, TsLintIssue[].class); + return gson.fromJson(toParse, TsLintIssue[][].class); } } \ No newline at end of file diff --git a/src/main/java/com/pablissimo/sonar/TsLintSensor.java b/src/main/java/com/pablissimo/sonar/TsLintSensor.java index c699aff..a10e8bb 100644 --- a/src/main/java/com/pablissimo/sonar/TsLintSensor.java +++ b/src/main/java/com/pablissimo/sonar/TsLintSensor.java @@ -5,6 +5,7 @@ import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -88,37 +89,62 @@ else if (pathToTsLintConfig == null) { ruleNames.add(rule.getKey()); } + List paths = new ArrayList(); + HashMap fileMap = new HashMap(); + for (File file : fileSystem.files(this.filePredicates.hasLanguage(TypeScriptLanguage.LANGUAGE_KEY))) { if (skipTypeDefFiles && file.getName().toLowerCase().endsWith("." + TypeScriptLanguage.LANGUAGE_DEFINITION_EXTENSION)) { continue; } - + + String pathAdjusted = file.getAbsolutePath().replace('\\', '/'); + paths.add(pathAdjusted); + fileMap.put(pathAdjusted, file); + } + + String jsonResult = executor.execute(pathToTsLint, pathToTsLintConfig, rulesDir, paths, tsLintTimeoutMs); + + TsLintIssue[][] issues = parser.parse(jsonResult); + + if (issues == null) { + LOG.warn("TsLint returned no result at all"); + return; + } + + // Each issue bucket will contain info about a single file + for (TsLintIssue[] batchIssues : issues) { + if (batchIssues == null || batchIssues.length == 0) { + continue; + } + + String filePath = batchIssues[0].getName(); + + if (!fileMap.containsKey(filePath)) { + LOG.warn("TsLint reported issues against a file that wasn't sent to it - will be ignored: " + filePath); + continue; + } + + File file = fileMap.get(filePath); Resource resource = this.getFileFromIOFile(file, project); Issuable issuable = perspectives.as(Issuable.class, resource); - - String jsonResult = executor.execute(pathToTsLint, pathToTsLintConfig, rulesDir, file.getAbsolutePath(), tsLintTimeoutMs); - - TsLintIssue[] issues = parser.parse(jsonResult); - - if (issues != null) { - for (TsLintIssue issue : issues) { - // Make sure the rule we're violating is one we recognise - if not, we'll - // fall back to the generic 'tslint-issue' rule - String ruleName = issue.getRuleName(); - if (!ruleNames.contains(ruleName)) { - ruleName = TsRulesDefinition.RULE_TSLINT_ISSUE; - } - - issuable.addIssue - ( - issuable - .newIssueBuilder() - .line(issue.getStartPosition().getLine() + 1) - .message(issue.getFailure()) - .ruleKey(RuleKey.of(TsRulesDefinition.REPOSITORY_NAME, ruleName)) - .build() - ); + + for (TsLintIssue issue : batchIssues) { + // Make sure the rule we're violating is one we recognise - if not, we'll + // fall back to the generic 'tslint-issue' rule + String ruleName = issue.getRuleName(); + if (!ruleNames.contains(ruleName)) { + ruleName = TsRulesDefinition.RULE_TSLINT_ISSUE; } + + issuable.addIssue + ( + issuable + .newIssueBuilder() + .line(issue.getStartPosition().getLine() + 1) + .message(issue.getFailure()) + .ruleKey(RuleKey.of(TsRulesDefinition.REPOSITORY_NAME, ruleName)) + .build() + ); } } } diff --git a/src/test/java/com/pablissimo/sonar/TsLintExecutorImplTest.java b/src/test/java/com/pablissimo/sonar/TsLintExecutorImplTest.java index 42e5a0a..c097508 100644 --- a/src/test/java/com/pablissimo/sonar/TsLintExecutorImplTest.java +++ b/src/test/java/com/pablissimo/sonar/TsLintExecutorImplTest.java @@ -4,6 +4,8 @@ import static org.mockito.Mockito.*; import java.util.ArrayList; +import java.util.List; + import org.junit.Before; import org.junit.Test; import org.mockito.invocation.InvocationOnMock; @@ -12,6 +14,8 @@ import org.sonar.api.utils.command.CommandExecutor; import org.sonar.api.utils.command.StreamConsumer; +import edu.emory.mathcs.backport.java.util.Arrays; + public class TsLintExecutorImplTest { TsLintExecutorImpl executorImpl; CommandExecutor commandExecutor; @@ -24,7 +28,7 @@ public void setUp() throws Exception { } @Test - public void executesCommandWithCorrectArguments() { + public void executesCommandWithCorrectArgumentsAndTimeouts() { final ArrayList capturedCommands = new ArrayList(); final ArrayList capturedTimeouts = new ArrayList(); @@ -38,14 +42,93 @@ public Integer answer(InvocationOnMock invocation) throws Throwable { }; when(this.commandExecutor.execute(any(Command.class), any(StreamConsumer.class), any(StreamConsumer.class), any(long.class))).then(captureCommand); - this.executorImpl.execute("path/to/tslint", "path/to/config", "path/to/rules", "path/to/file", 40000); + this.executorImpl.execute("path/to/tslint", "path/to/config", "path/to/rules", Arrays.asList(new String[] { "path/to/file", "path/to/another" }), 40000); assertEquals(1, capturedCommands.size()); Command theCommand = capturedCommands.get(0); long theTimeout = capturedTimeouts.get(0); - assertEquals("node path/to/tslint --format json --rules-dir path/to/rules --config path/to/config path/to/file", theCommand.toCommandLine()); - assertEquals(40000, theTimeout); + assertEquals("node path/to/tslint --format json --rules-dir path/to/rules --config path/to/config path/to/file path/to/another", theCommand.toCommandLine()); + // Expect one timeout period per file processed + assertEquals(2 * 40000, theTimeout); + } + + @Test + public void DoesNotAddRulesDirParameter_IfNull() { + final ArrayList capturedCommands = new ArrayList(); + final ArrayList capturedTimeouts = new ArrayList(); + + Answer captureCommand = new Answer() { + @Override + public Integer answer(InvocationOnMock invocation) throws Throwable { + capturedCommands.add((Command) invocation.getArguments()[0]); + capturedTimeouts.add((long) invocation.getArguments()[3]); + return 0; + } + }; + + when(this.commandExecutor.execute(any(Command.class), any(StreamConsumer.class), any(StreamConsumer.class), any(long.class))).then(captureCommand); + this.executorImpl.execute("path/to/tslint", "path/to/config", null, Arrays.asList(new String[] { "path/to/file" }), 40000); + + Command theCommand = capturedCommands.get(0); + assertFalse(theCommand.toCommandLine().contains("--rules-dir")); + } + + @Test + public void DoesNotAddRulesDirParameter_IfEmptyString() { + final ArrayList capturedCommands = new ArrayList(); + final ArrayList capturedTimeouts = new ArrayList(); + + Answer captureCommand = new Answer() { + @Override + public Integer answer(InvocationOnMock invocation) throws Throwable { + capturedCommands.add((Command) invocation.getArguments()[0]); + capturedTimeouts.add((long) invocation.getArguments()[3]); + return 0; + } + }; + + when(this.commandExecutor.execute(any(Command.class), any(StreamConsumer.class), any(StreamConsumer.class), any(long.class))).then(captureCommand); + this.executorImpl.execute("path/to/tslint", "path/to/config", "", Arrays.asList(new String[] { "path/to/file" }), 40000); + + Command theCommand = capturedCommands.get(0); + assertFalse(theCommand.toCommandLine().contains("--rules-dir")); + } + + @Test + public void BatchesExecutions_IfTooManyFilesForCommandLine() { + List filenames = new ArrayList(); + int currentLength = 0; + int standardCmdLength = "node path/to/tslint --format json --rules-dir path/to/rules --config path/to/config".length(); + + String firstBatch = "first batch"; + while (currentLength + 12 < TsLintExecutorImpl.MAX_COMMAND_LENGTH - standardCmdLength) { + filenames.add(firstBatch); + currentLength += firstBatch.length() + 1; + } + filenames.add("second batch"); + + final ArrayList capturedCommands = new ArrayList(); + final ArrayList capturedTimeouts = new ArrayList(); + + Answer captureCommand = new Answer() { + @Override + public Integer answer(InvocationOnMock invocation) throws Throwable { + capturedCommands.add((Command) invocation.getArguments()[0]); + capturedTimeouts.add((long) invocation.getArguments()[3]); + return 0; + } + }; + + when(this.commandExecutor.execute(any(Command.class), any(StreamConsumer.class), any(StreamConsumer.class), any(long.class))).then(captureCommand); + this.executorImpl.execute("path/to/tslint", "path/to/config", "path/to/rules", filenames, 40000); + + assertEquals(2, capturedCommands.size()); + + Command theSecondCommand = capturedCommands.get(1); + + assertFalse(theSecondCommand.toCommandLine().contains("first batch")); + assertTrue(theSecondCommand.toCommandLine().contains("second batch")); } } diff --git a/src/test/java/com/pablissimo/sonar/TsLintParserTest.java b/src/test/java/com/pablissimo/sonar/TsLintParserTest.java index 5b0e39d..82a2a99 100644 --- a/src/test/java/com/pablissimo/sonar/TsLintParserTest.java +++ b/src/test/java/com/pablissimo/sonar/TsLintParserTest.java @@ -16,12 +16,16 @@ public class TsLintParserTest { @Test public void parsesValidTsLintRecordIntoObject() { - String toParse = "[{\"endPosition\":{\"character\":44,\"line\":23,\"position\":658},\"failure\":\"for statements must be braced\",\"name\":\"Tools.ts\",\"ruleName\":\"curly\",\"startPosition\":{\"character\":6,\"line\":22,\"position\":587}}]"; - TsLintIssue[] issues = new TsLintParserImpl().parse(toParse); + String toParse = "[[{\"endPosition\":{\"character\":44,\"line\":23,\"position\":658},\"failure\":\"for statements must be braced\",\"name\":\"Tools.ts\",\"ruleName\":\"curly\",\"startPosition\":{\"character\":6,\"line\":22,\"position\":587}}]]"; + TsLintIssue[][] issues = new TsLintParserImpl().parse(toParse); assertEquals(1, issues.length); - TsLintIssue issue = issues[0]; + TsLintIssue[] fileIssues = issues[0]; + + assertEquals(1, fileIssues.length); + + TsLintIssue issue = fileIssues[0]; assertEquals(44, issue.getEndPosition().getCharacter()); assertEquals(23, issue.getEndPosition().getLine()); assertEquals(658, issue.getEndPosition().getPosition()); diff --git a/src/test/java/com/pablissimo/sonar/TsLintSensorTest.java b/src/test/java/com/pablissimo/sonar/TsLintSensorTest.java index 973004f..dd644ee 100644 --- a/src/test/java/com/pablissimo/sonar/TsLintSensorTest.java +++ b/src/test/java/com/pablissimo/sonar/TsLintSensorTest.java @@ -63,6 +63,7 @@ public void setUp() throws Exception { this.file = mock(File.class); doReturn(true).when(this.file).isFile(); + doReturn("/path/to/file").when(this.file).getAbsolutePath(); this.files = new ArrayList(Arrays.asList(new File[] { this.file @@ -106,14 +107,15 @@ public void analyse_addsIssues() { TsLintIssue issue = new TsLintIssue(); issue.setFailure("failure"); issue.setRuleName("rule name"); + issue.setName("/path/to/file"); TsLintPosition startPosition = new TsLintPosition(); startPosition.setLine(1); issue.setStartPosition(startPosition); - TsLintIssue[] issues = new TsLintIssue[] { - issue + TsLintIssue[][] issues = new TsLintIssue[][] { + new TsLintIssue[] { issue } }; final List capturedIssues = new ArrayList(); @@ -155,7 +157,7 @@ public void analyse_doesNothingWhenNoConfigPathset() throws IOException { public void analyse_callsExecutorWithSuppliedTimeout() throws IOException { this.sensor.analyse(mock(Project.class), mock(SensorContext.class)); - verify(this.executor, times(1)).execute(any(String.class), any(String.class), any(String.class), any(String.class), eq(45000)); + verify(this.executor, times(1)).execute(any(String.class), any(String.class), any(String.class), any(List.class), eq(45000)); } @Test @@ -164,6 +166,6 @@ public void analyze_callsExecutorWithAtLeast5000msTimeout() throws IOException { this.sensor.analyse(mock(Project.class), mock(SensorContext.class)); - verify(this.executor, times(1)).execute(any(String.class), any(String.class), any(String.class), any(String.class), eq(5000)); + verify(this.executor, times(1)).execute(any(String.class), any(String.class), any(String.class), any(List.class), eq(5000)); } }