Log error output from external process and check for return value

5 files changed, 148 insertions, 20 deletions

diff --git a/sonar-css-plugin/src/main/java/org/sonar/css/plugin/CssRuleSensor.java b/sonar-css-plugin/src/main/java/org/sonar/css/plugin/CssRuleSensor.java
index 57b6d0d..a34b433 100644
--- a/sonar-css-plugin/src/main/java/org/sonar/css/plugin/CssRuleSensor.java
+++ b/sonar-css-plugin/src/main/java/org/sonar/css/plugin/CssRuleSensor.java
@@ -24,7 +24,6 @@ import com.google.gson.GsonBuilder;
 import com.google.gson.JsonSyntaxException;
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStreamReader;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Paths;
@@ -56,8 +55,11 @@ public class CssRuleSensor implements Sensor {
   private final BundleHandler bundleHandler;
   private final CssRules cssRules;
   private final LinterCommandProvider linterCommandProvider;
+  private final ExternalProcessStreamConsumer externalProcessStreamConsumer = new ExternalProcessStreamConsumer();
 
-  public CssRuleSensor(BundleHandler bundleHandler, CheckFactory checkFactory, LinterCommandProvider linterCommandProvider) {
+  public CssRuleSensor(BundleHandler bundleHandler,
+                       CheckFactory checkFactory,
+                       LinterCommandProvider linterCommandProvider) {
     this.bundleHandler = bundleHandler;
     this.linterCommandProvider = linterCommandProvider;
     this.cssRules = new CssRules(checkFactory);
@@ -83,25 +85,40 @@ public class CssRuleSensor implements Sensor {
 
     File deployDestination = context.fileSystem().workDir();
     bundleHandler.deployBundle(deployDestination);
-
     String[] commandParts = linterCommandProvider.commandParts(deployDestination, context);
-    ProcessBuilder processBuilder = new ProcessBuilder(commandParts);
-    String command = String.join(" ", commandParts);
 
     try {
-      createConfig(deployDestination);
+      ProcessBuilder processBuilder = new ProcessBuilder(commandParts);
+      createLinterConfig(deployDestination);
       Process process = processBuilder.start();
-
-      try (InputStreamReader inputStreamReader = new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8)) {
-        IssuesPerFile[] issues = new Gson().fromJson(inputStreamReader, IssuesPerFile[].class);
-        saveIssues(context, issues);
+      StringBuilder output = new StringBuilder();
+      externalProcessStreamConsumer.consumeStream(process.getInputStream(), output::append);
+      externalProcessStreamConsumer.consumeStream(process.getErrorStream(), LOG::error);
+      if (isSuccessful(process)) {
+        saveIssues(context, output.toString());
       }
+    } catch (Exception e) {
+      LOG.error("Failed to run external linting process " + String.join(" ", commandParts), e);
+    } finally {
+      externalProcessStreamConsumer.shutdownNow();
+    }
+  }
 
-    } catch (IOException e) {
-      throw new IllegalStateException(String.format("Failed to run external process '%s'", command), e);
-
-    } catch (JsonSyntaxException e) {
-      throw new IllegalStateException(String.format("Failed to parse json result of external process execution '%s'. To diagnose, try to run it manually.", command), e);
+  private boolean isSuccessful(Process process) {
+    try {
+      int exitValue = process.waitFor();
+      externalProcessStreamConsumer.await();
+      // exit codes 0 and 2 are expected. 0 - means no issues were found, 2 - means that at least one "error-level" rule found issue
+      // see https://github.com/stylelint/stylelint/blob/master/docs/user-guide/cli.md#exit-codes
+      boolean isSuccessful = exitValue == 0 || exitValue == 2;
+      if (!isSuccessful) {
+        LOG.error("Analysis didn't terminate normally, please verify ERROR and WARN logs above. Exit code {}", exitValue);
+      }
+      return isSuccessful;
+    } catch (InterruptedException e) {
+      LOG.warn("InterruptedException while waiting for external process to finish", e);
+      Thread.currentThread().interrupt();
+      return false;
     }
   }
 
@@ -138,7 +155,7 @@ public class CssRuleSensor implements Sensor {
     return true;
   }
 
-  private void createConfig(File deployDestination) throws IOException {
+  private void createLinterConfig(File deployDestination) throws IOException {
     String configPath = linterCommandProvider.configPath(deployDestination);
     StylelintConfig config = cssRules.getConfig();
     final GsonBuilder gsonBuilder = new GsonBuilder();
@@ -159,7 +176,14 @@ public class CssRuleSensor implements Sensor {
     }
   }
 
-  private void saveIssues(SensorContext context, IssuesPerFile[] issues) {
+  private void saveIssues(SensorContext context, String issuesAsJson) {
+    IssuesPerFile[] issues;
+    try {
+      issues = new Gson().fromJson(issuesAsJson, IssuesPerFile[].class);
+    } catch (JsonSyntaxException e) {
+      throw new IllegalStateException("Failed to parse JSON result of external linting process execution: \n-------\n" + issuesAsJson + "\n-------", e);
+    }
+
     FileSystem fileSystem = context.fileSystem();
 
     for (IssuesPerFile issuesPerFile : issues) {
diff --git a/sonar-css-plugin/src/main/java/org/sonar/css/plugin/ExternalProcessStreamConsumer.java b/sonar-css-plugin/src/main/java/org/sonar/css/plugin/ExternalProcessStreamConsumer.java
new file mode 100644
index 0000000..d6f176b
--- /dev/null
+++ b/sonar-css-plugin/src/main/java/org/sonar/css/plugin/ExternalProcessStreamConsumer.java
@@ -0,0 +1,73 @@
+/*
+ * SonarCSS
+ * Copyright (C) 2018-2018 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.css.plugin;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Consumer;
+import org.sonar.api.utils.log.Logger;
+import org.sonar.api.utils.log.Loggers;
+
+class ExternalProcessStreamConsumer {
+
+  private static final Logger LOG = Loggers.get(ExternalProcessStreamConsumer.class);
+  private ExecutorService executorService;
+
+  ExternalProcessStreamConsumer() {
+    executorService = Executors.newCachedThreadPool(r -> {
+      Thread thread = new Thread(r);
+      thread.setName("nodejs-stream-consumer");
+      thread.setDaemon(true);
+      return thread;
+    });
+  }
+
+  void consumeStream(InputStream inputStream, Consumer<String> consumer) {
+    executorService.submit(() -> {
+      try (BufferedReader errorReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8))) {
+        errorReader.lines().forEach(consumer);
+      } catch (IOException e) {
+        LOG.error("Error while reading stream", e);
+      }
+    });
+  }
+
+  void await() {
+    executorService.shutdown();
+    try {
+      if (!executorService.awaitTermination(5, TimeUnit.MINUTES)) {
+        LOG.error("External process stream consumer timed out");
+      }
+    } catch (InterruptedException e) {
+      LOG.warn("InterruptedException while waiting for external process stream consumer to finish", e);
+      Thread.currentThread().interrupt();
+    }
+  }
+
+  void shutdownNow() {
+    executorService.shutdownNow();
+  }
+}
diff --git a/sonar-css-plugin/src/test/java/org/sonar/css/plugin/CssRuleSensorTest.java b/sonar-css-plugin/src/test/java/org/sonar/css/plugin/CssRuleSensorTest.java
index f44781b..782b10f 100644
--- a/sonar-css-plugin/src/test/java/org/sonar/css/plugin/CssRuleSensorTest.java
+++ b/sonar-css-plugin/src/test/java/org/sonar/css/plugin/CssRuleSensorTest.java
@@ -26,6 +26,8 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.concurrent.TimeUnit;
+import org.awaitility.Awaitility;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -46,6 +48,7 @@ import org.sonar.css.plugin.bundle.BundleHandler;
 import org.sonar.css.plugin.bundle.CssBundleHandler;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
 
 public class CssRuleSensorTest {
 
@@ -68,6 +71,7 @@ public class CssRuleSensorTest {
   @Before
   public void setUp() {
     context.fileSystem().setWorkDir(tmpDir.getRoot().toPath());
+    Awaitility.setDefaultTimeout(5, TimeUnit.MINUTES);
   }
 
   @Test
@@ -128,12 +132,11 @@ public class CssRuleSensorTest {
 
   @Test
   public void test_error() {
-    thrown.expect(IllegalStateException.class);
-    thrown.expectMessage("Failed to parse json result of external process execution");
-
     TestLinterCommandProvider commandProvider = new TestLinterCommandProvider().nodeScript("/executables/mockError.js", inputFile.absolutePath());
     CssRuleSensor sensor = new CssRuleSensor(new TestBundleHandler(), checkFactory, commandProvider);
     sensor.execute(context);
+
+    assertThat(logTester.logs(LoggerLevel.ERROR)).anyMatch(s -> s.startsWith("Failed to run external linting process"));
   }
 
   @Test
@@ -146,6 +149,26 @@ public class CssRuleSensorTest {
   }
 
   @Test
+  public void test_stylelint_throws() {
+    TestLinterCommandProvider commandProvider = new TestLinterCommandProvider().nodeScript("/executables/mockThrow.js", inputFile.absolutePath());
+    CssRuleSensor sensor = new CssRuleSensor(new TestBundleHandler(), checkFactory, commandProvider);
+    sensor.execute(context);
+
+    await().until(() -> logTester.logs(LoggerLevel.ERROR)
+      .contains("throw new Error('houps!');"));
+  }
+
+  @Test
+  public void test_stylelint_exitvalue() {
+    TestLinterCommandProvider commandProvider = new TestLinterCommandProvider().nodeScript("/executables/mockExit.js", "1");
+    CssRuleSensor sensor = new CssRuleSensor(new TestBundleHandler(), checkFactory, commandProvider);
+    sensor.execute(context);
+
+    await().until(() -> logTester.logs(LoggerLevel.ERROR)
+      .contains("Analysis didn't terminate normally, please verify ERROR and WARN logs above. Exit code 1"));
+  }
+
+  @Test
   public void test_syntax_error() {
     SensorContextTester context = SensorContextTester.create(BASE_DIR);
     context.fileSystem().setWorkDir(tmpDir.getRoot().toPath());
diff --git a/sonar-css-plugin/src/test/resources/executables/mockExit.js b/sonar-css-plugin/src/test/resources/executables/mockExit.js
new file mode 100644
index 0000000..b18a959
--- /dev/null
+++ b/sonar-css-plugin/src/test/resources/executables/mockExit.js
@@ -0,0 +1,5 @@
+#!/usr/bin/env node
+
+console.log("[]");
+
+process.exit(process.argv[2]);
diff --git a/sonar-css-plugin/src/test/resources/executables/mockThrow.js b/sonar-css-plugin/src/test/resources/executables/mockThrow.js
new file mode 100644
index 0000000..ca88c27
--- /dev/null
+++ b/sonar-css-plugin/src/test/resources/executables/mockThrow.js
@@ -0,0 +1,3 @@
+#!/usr/bin/env node
+
+throw new Error('houps!');