Refactoring after review, new tests for pylint output parser.

src/main/java/pl/touk/sputnik/processor/pylint/PylintExecutor.java

@@ -12,9 +12,9 @@
 
 @Slf4j
 public class PylintExecutor {
-    private static final String pylintExecutable = "pylint";
-    private static final String pylintOutputFormat = "--output-format=json";
-    private static final String pylintRcfileName = "--rcfile=";
+    private static final String PYLINT_EXECUTABLE = "pylint";
+    private static final String PYLINT_OUTPUT_FORMAT = "--output-format=json";
+    private static final String PYLINT_RCFILE_NAME = "--rcfile=";
 
     private String rcfileName;
 
@@ -30,19 +30,18 @@ public String runOnFile(String filePath) {
     @NotNull
     private String[] buildParams(String filePath) {
         List<String> basicPylintArgs = ImmutableList.of(
-                pylintExecutable,
-                pylintOutputFormat);
+                PYLINT_EXECUTABLE,
+                PYLINT_OUTPUT_FORMAT);
         List<String> rcfileNameArg = getRcfileNameAsList();
         List<String> filePathArg = ImmutableList.of(filePath);
         List<String> allArgs = Lists.newArrayList(Iterables.concat(basicPylintArgs, rcfileNameArg, filePathArg));
         return allArgs.toArray(new String[allArgs.size()]);
     }
 
     private List<String> getRcfileNameAsList() {
-        if (rcfileName != null) {
-            return ImmutableList.of(pylintRcfileName + rcfileName);
-        } else {
+        if (rcfileName == null) {
             return ImmutableList.of();
         }
+        return ImmutableList.of(PYLINT_RCFILE_NAME + rcfileName);
     }
 }

src/main/java/pl/touk/sputnik/processor/pylint/PylintResultParser.java

@@ -17,6 +17,12 @@ public class PylintResultParser implements ExternalProcessResultParser {
 
     private final ObjectMapper objectMapper = new ObjectMapper();
 
+    private static final String PYLINT_ERROR = "error";
+    private static final String PYLINT_FATAL = "fatal";
+    private static final String PYLINT_WARNING = "warning";
+    private static final String PYLINT_CONVENTION = "convention";
+    private static final String PYLINT_REFACTOR = "refactor";
+
     @Override
     public List<Violation> parse(String pylintOutput) {
         if (StringUtils.isEmpty(pylintOutput)) {
@@ -29,7 +35,7 @@ public List<Violation> parse(String pylintOutput) {
                 Violation violation = new Violation(message.getPath(),
                         message.getLine(),
                         formatViolationMessageFromPylint(message),
-                        pylintMessageTypeToSeverity(message.getType()));
+                        pylintMessageTypeToSeverity(message.getMessage(), message.getType()));
                 violations.add(violation);
             }
             return violations;
@@ -46,15 +52,17 @@ private String formatViolationMessageFromPylint(PylintMessage message) {
         return message.getMessage() + " [" + message.getSymbol() + "]";
     }
 
-    private Severity pylintMessageTypeToSeverity(String messageType) {
-        if (messageType.equals("error") || messageType.equals("fatal")) {
+    private Severity pylintMessageTypeToSeverity(String message, String messageType) {
+        if (PYLINT_ERROR.equals(messageType)) {
             return Severity.ERROR;
-        } else if (messageType.equals("warning")) {
+        } else if (PYLINT_WARNING.equals(messageType)) {
             return Severity.WARNING;
-        } else if (messageType.equals("convention") || messageType.equals("refactor")) {
+        } else if (PYLINT_CONVENTION.equals(messageType) || PYLINT_REFACTOR.equals(messageType)) {
             return Severity.INFO;
+        } else if (PYLINT_FATAL.equals(messageType)) {
+            throw new PylintException("Fatal error from pylint (" + message + ")");
         } else {
-            throw new PylintException("Unknown message type returned by pylint in message (type = " + messageType + ")");
+            throw new PylintException("Unknown message type returned by pylint (type = " + messageType + ")");
         }
     }
 }

src/test/java/pl/touk/sputnik/processor/pylint/PylintResultParserTest.java

@@ -5,21 +5,28 @@
 import junitparams.Parameters;
 import org.apache.commons.io.IOUtils;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
+import pl.touk.sputnik.review.Severity;
 import pl.touk.sputnik.review.Violation;
 
 import java.io.IOException;
 import java.net.URISyntaxException;
 import java.util.List;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.hamcrest.core.StringStartsWith.startsWith;
 
 @RunWith(JUnitParamsRunner.class)
 public class PylintResultParserTest {
 
     private PylintResultParser pylintResultParser;
 
+    @Rule
+    public ExpectedException thrown = ExpectedException.none();
+
     @Before
     public void setUp() {
         pylintResultParser = new PylintResultParser();
@@ -46,4 +53,29 @@ public void shouldParseSampleViolations(String filePath) throws IOException, URI
                 .contains("Black listed name \"bar\" [blacklisted-name]")
                 .contains("Black listed name \"foo\" [blacklisted-name]");
     }
+
+    @Test
+    public void shouldParseMessageTypes() throws IOException, URISyntaxException {
+        // given
+        String response = IOUtils.toString(Resources.getResource("pylint/output-with-many-message-types.txt").toURI());
+
+        // when
+        List<Violation> violations = pylintResultParser.parse(response);
+
+        // then
+        assertThat(violations).hasSize(4);
+        assertThat(violations)
+                .extracting("severity")
+                .containsExactly(Severity.INFO, Severity.INFO,  Severity.WARNING, Severity.ERROR);
+    }
+
+    @Test
+    public void shouldThrowExceptionWhenFatalPylintErrorOccurs() throws IOException, URISyntaxException {
+        // given
+        String response = IOUtils.toString(Resources.getResource("pylint/output-with-fatal.txt").toURI());
+
+        thrown.expect(PylintException.class);
+        thrown.expectMessage(startsWith("Fatal error from pylint"));
+        pylintResultParser.parse(response);
+    }
 }

src/test/resources/pylint/output-with-fatal.txt

@@ -0,0 +1,42 @@
+[
+    {
+        "message": "Missing module docstring",
+        "obj": "",
+        "column": 0,
+        "path": "PythonTest.py",
+        "line": 1,
+        "type": "convention",
+        "symbol": "missing-docstring",
+        "module": "PythonTest"
+    },
+    {
+        "message": "Do refactor",
+        "obj": "foo",
+        "column": 0,
+        "path": "PythonTest.py",
+        "line": 1,
+        "type": "refactor",
+        "symbol": "blacklisted-name",
+        "module": "PythonTest"
+    },
+    {
+        "message": "Fatal error - total destruction",
+        "obj": "foo",
+        "column": 0,
+        "path": "PythonTest.py",
+        "line": 1,
+        "type": "fatal",
+        "symbol": "invalid-name",
+        "module": "PythonTest"
+    },
+    {
+        "message": "Something bad",
+        "obj": "foo",
+        "column": 0,
+        "path": "PythonTest.py",
+        "line": 1,
+        "type": "error",
+        "symbol": "missing-docstring",
+        "module": "PythonTest"
+    }
+]

src/test/resources/pylint/output-with-many-message-types.txt

@@ -0,0 +1,42 @@
+[
+    {
+        "message": "Missing module docstring",
+        "obj": "",
+        "column": 0,
+        "path": "PythonTest.py",
+        "line": 1,
+        "type": "convention",
+        "symbol": "missing-docstring",
+        "module": "PythonTest"
+    },
+    {
+        "message": "Do refactor",
+        "obj": "foo",
+        "column": 0,
+        "path": "PythonTest.py",
+        "line": 1,
+        "type": "refactor",
+        "symbol": "blacklisted-name",
+        "module": "PythonTest"
+    },
+    {
+        "message": "Some warning",
+        "obj": "foo",
+        "column": 0,
+        "path": "PythonTest.py",
+        "line": 1,
+        "type": "warning",
+        "symbol": "invalid-name",
+        "module": "PythonTest"
+    },
+    {
+        "message": "Something bad",
+        "obj": "foo",
+        "column": 0,
+        "path": "PythonTest.py",
+        "line": 1,
+        "type": "error",
+        "symbol": "missing-docstring",
+        "module": "PythonTest"
+    }
+]