Make lint tests handle paths better and distinguish between "0" and "null" more carefully

Summary:
Ref T13543. Currently, the `cpplint` tests do not function because `cpplint` is passed a path which does not end in a suffix it recognizes.

Change the tempfile / path code to pass `linter path/to/example.c`-style linters a path they expect.

Then, correct some older code which was playing it fast-and-loose with "null" vs "0".

Test Plan: Ran `arc unit --everything`, got a clean bill of health on all the linters I have installed. (This is probably not all tests, since I have only a subset of linters installed locally that we have code for.)

Maniphest Tasks: T13543

Differential Revision: https://secure.phabricator.com/D21291

src/lint/linter/ArcanistTextLinter.php

@@ -261,8 +261,7 @@ final class ArcanistTextLinter extends ArcanistLinter {
         self::LINT_TRAILING_WHITESPACE,
         pht(
           'This line contains trailing whitespace. Consider setting '.
-          'up your editor to automatically remove trailing whitespace, '.
+          'up your editor to automatically remove trailing whitespace.'),
-          'you will save time.'),
         $string,
         '');
     }

src/lint/linter/__tests__/ArcanistLinterTestCase.php

@@ -64,8 +64,6 @@ abstract class ArcanistLinterTestCase extends PhutilTestCase {
       $contents,
       array(null, null));
 
-    $basename = basename($file);
-
     if ($config) {
       $config = phutil_json_decode($config);
     } else {
@@ -87,6 +85,14 @@ abstract class ArcanistLinterTestCase extends PhutilTestCase {
     $caught_exception = false;
 
     try {
+      $path_name = idx($config, 'path');
+
+      if ($path_name !== null) {
+        $basename = basename($path_name);
+      } else {
+        $basename = basename($file);
+      }
+
       $tmp = new TempFile($basename);
       Filesystem::writeFile($tmp, $data);
       $full_path = (string)$tmp;
@@ -97,7 +103,6 @@ abstract class ArcanistLinterTestCase extends PhutilTestCase {
       }
 
       $dir = dirname($full_path);
-      $path = basename($full_path);
 
       $working_copy = ArcanistWorkingCopyIdentity::newFromRootAndConfigFile(
         $dir,
@@ -106,26 +111,25 @@ abstract class ArcanistLinterTestCase extends PhutilTestCase {
       $configuration_manager = new ArcanistConfigurationManager();
       $configuration_manager->setWorkingCopyIdentity($working_copy);
 
-
       $engine = new ArcanistUnitTestableLintEngine();
       $engine->setWorkingCopy($working_copy);
       $engine->setConfigurationManager($configuration_manager);
 
-      $path_name = idx($config, 'path', $path);
+      $engine->setPaths(array($basename));
-      $engine->setPaths(array($path_name));
 
       $linter->setEngine($engine);
-      $linter->addPath($path_name);
+      $linter->addPath($basename);
-      $linter->addData($path_name, $data);
+      $linter->addData($basename, $data);
 
       foreach (idx($config, 'config', array()) as $key => $value) {
         $linter->setLinterConfigurationValue($key, $value);
       }
 
       $engine->addLinter($linter);
-      $engine->addFileData($path_name, $data);
+      $engine->addFileData($basename, $data);
 
       $results = $engine->run();
+
       $this->assertEqual(
         1,
         count($results),
@@ -187,9 +191,20 @@ abstract class ArcanistLinterTestCase extends PhutilTestCase {
       $message = new ArcanistLintMessage();
 
       $severity = idx($parts, 0);
-      $line     = idx($parts, 1);
+      $line = idx($parts, 1);
-      $char     = idx($parts, 2);
+      if ($line === '') {
-      $code     = idx($parts, 3);
+        $line = null;
+      }
+
+      $char = idx($parts, 2);
+      if ($char === '') {
+        $char = null;
+      }
+
+      $code = idx($parts, 3);
+      if ($code === '') {
+        $code = null;
+      }
 
       if ($severity !== null) {
         $message->setSeverity($severity);
@@ -256,46 +271,14 @@ abstract class ArcanistLinterTestCase extends PhutilTestCase {
     }
 
     if ($missing || $surprising) {
-      $expected = pht('EXPECTED MESSAGES');
-      if ($missing) {
-        foreach ($missing as $message) {
-          $expected .= sprintf(
-            "\n  %s: %s %s",
-            pht(
-              '%s at line %d, char %d',
-              $message->getSeverity(),
-              $message->getLine(),
-              $message->getChar()),
-            $message->getCode(),
-            $message->getName());
-        }
-      } else {
-        $expected .= "\n  ".pht('No messages');
-      }
-
-      $actual = pht('UNEXPECTED MESSAGES');
-      if ($surprising) {
-        foreach ($surprising as $message) {
-          $actual .= sprintf(
-            "\n  %s: %s %s",
-            pht(
-              '%s at line %d, char %d',
-              $message->getSeverity(),
-              $message->getLine(),
-              $message->getChar()),
-            $message->getCode(),
-            $message->getName());
-        }
-      } else {
-        $actual .= "\n  ".pht('No messages');
-      }
-
       $this->assertFailure(
         sprintf(
-          "%s\n\n%s\n\n%s",
+          "%s\n%s%s",
-          pht("Lint failed for '%s'.", $file),
+          pht(
-          $expected,
+            'Lint emitted an unexpected set of messages for file "%s".',
-          $actual));
+            $file),
+          $this->renderMessages(pht('MISSING MESSAGES'), $missing),
+          $this->renderMessages(pht('SURPLUS MESSAGES'), $surprising)));
     }
   }
 
@@ -312,15 +295,78 @@ abstract class ArcanistLinterTestCase extends PhutilTestCase {
   /**
    * Compare properties of @{class:ArcanistLintMessage} instances.
    *
-   * The expectation is that if one (or both) of the properties is null, then
-   * we don't care about its value.
-   *
    * @param  wild
    * @param  wild
    * @return bool
    */
   private static function compareLintMessageProperty($x, $y) {
-    return $x === null || $y === null || $x === $y;
+    if ($x === null) {
+      return true;
+    }
+
+    return ($x === $y);
+  }
+
+  private function renderMessages($header, array $messages) {
+    if (!$messages) {
+      $display = tsprintf(
+        "%s\n",
+        pht('(No messages.)'));
+    } else {
+      $lines = array();
+      foreach ($messages as $message) {
+        $line = $message->getLine();
+        if ($line === null) {
+          $display_line = pht('<null>');
+        } else {
+          $display_line = $line;
+        }
+
+        $char = $message->getChar();
+        if ($char === null) {
+          $display_char = pht('<null>');
+        } else {
+          $display_char = $char;
+        }
+
+        $code = $message->getCode();
+        $name = $message->getName();
+        if ($code !== null && $name !== null) {
+          $display_code = pht('%s: %s', $code, $name);
+        } else if ($code !== null) {
+          $display_code = pht('%s', $code);
+        } else {
+          $display_code = null;
+        }
+
+        $severity = $message->getSeverity();
+
+        if ($display_code === null) {
+          $display_message = pht(
+            'Message with severity "%s" at "%s:%s"',
+            $severity,
+            $display_line,
+            $display_char);
+        } else {
+          $display_message = pht(
+            'Message with severity "%s" at "%s:%s" (%s)',
+            $severity,
+            $display_line,
+            $display_char,
+            $display_code);
+        }
+
+        $lines[] = tsprintf(
+          "    %s\n",
+          $display_message);
+      }
+      $display = implode('', $lines);
+    }
+
+    return tsprintf(
+      "%s\n%B\n",
+      $header,
+      $display);
   }
 
 }

src/lint/linter/__tests__/chmod/empty_executable.lint-test

@@ -1,5 +1,5 @@
 ~~~~~~~~~~
-warning:0:0:CHMOD1:Invalid Executable
+warning:::CHMOD1:Invalid Executable
 ~~~~~~~~~~
 ~~~~~~~~~~
 {"mode": "0755"}

src/lint/linter/__tests__/cpplint/googlestyle.lint-test

@@ -1,10 +1,21 @@
 #include "library.cpp"
 #include <stdio>
 
+void main()
+{
+}
+~~~~~~~~~
+warning
+warning:2
+warning:5
+~~~~~~~~~~
+#include "library.cpp"
+#include <stdio>
+
 void main()
 {
 }
 ~~~~~~~~~~
-warning:0:
+{
-warning:2:
+  "path": "example.cpp"
-warning:5:
+}

src/lint/linter/__tests__/filename/bad_filename.lint-test

@@ -1,5 +1,5 @@
 ~~~~~~~~~~
-error:0:0:NAME1:Bad Filename
+error:::NAME1:Bad Filename
 ~~~~~~~~~~
 ~~~~~~~~~~
 {"path": "bad@filename"}

src/lint/linter/__tests__/php/fatal.lint-test

@@ -4,4 +4,4 @@ function f() {
   $this = "cannot be re-assigned";
 }
 ~~~~~~~~~
-error:4:0:PHP2:Fatal Error
+error:4::PHP2:Fatal Error

src/lint/linter/__tests__/php/syntax.lint-test

@@ -4,4 +4,4 @@ function f() {
   this is bad syntax;
 }
 ~~~~~~~~~
-error:4:0:PHP1:Parse Error
+error:4::PHP1:Parse Error

src/lint/linter/__tests__/ruby/hello.lint-test

@@ -1,4 +1,4 @@
 def hello()
   puts "hello world"
 ~~~~~~~~~~
-error:2:0:RUBY:Syntax Error
+error:2::RUBY:Syntax Error

src/lint/linter/__tests__/text/empty.lint-test

@@ -1,4 +1,4 @@
 
 
 ~~~~~~~~~~
-error:0:0:TXT10:Empty File
+error:::TXT10:Empty File

src/lint/linter/__tests__/xhpast/empty.lint-test

@@ -1,4 +1,4 @@
 <?php
 
 ~~~~~~~~~~
-warning:0:0:XHP82:Empty File
+warning:::XHP82:Empty File