From e7917f3580182bd0b4a1828a44ee9b89814ecf89 Mon Sep 17 00:00:00 2001 From: Tibor Blenessy Date: Wed, 12 Sep 2018 19:52:50 +0200 Subject: Log error output from external process and check for return value --- .../java/org/sonar/css/plugin/CssRuleSensor.java | 58 ++++++++++++----- .../css/plugin/ExternalProcessStreamConsumer.java | 73 ++++++++++++++++++++++ .../org/sonar/css/plugin/CssRuleSensorTest.java | 29 ++++++++- .../src/test/resources/executables/mockExit.js | 5 ++ .../src/test/resources/executables/mockThrow.js | 3 + 5 files changed, 148 insertions(+), 20 deletions(-) create mode 100644 sonar-css-plugin/src/main/java/org/sonar/css/plugin/ExternalProcessStreamConsumer.java create mode 100644 sonar-css-plugin/src/test/resources/executables/mockExit.js create mode 100644 sonar-css-plugin/src/test/resources/executables/mockThrow.js (limited to 'sonar-css-plugin') 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 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 @@ -145,6 +148,26 @@ public class CssRuleSensorTest { assertThat(logTester.logs(LoggerLevel.WARN)).contains("No rules are activated in CSS Quality Profile"); } + @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); 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!'); -- cgit v1.2.3