From fce72b9c89d7fa64ddeb3f9da7fc96ff07322abf Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 27 May 2020 12:07:49 -0700 Subject: [PATCH] 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 | 3 +- .../__tests__/ArcanistLinterTestCase.php | 154 ++++++++++++------ .../chmod/empty_executable.lint-test | 2 +- .../__tests__/cpplint/googlestyle.lint-test | 17 +- .../__tests__/filename/bad_filename.lint-test | 2 +- src/lint/linter/__tests__/php/fatal.lint-test | 2 +- .../linter/__tests__/php/syntax.lint-test | 2 +- .../linter/__tests__/ruby/hello.lint-test | 2 +- .../linter/__tests__/text/empty.lint-test | 2 +- .../linter/__tests__/xhpast/empty.lint-test | 2 +- 10 files changed, 122 insertions(+), 66 deletions(-) diff --git a/src/lint/linter/ArcanistTextLinter.php b/src/lint/linter/ArcanistTextLinter.php index c7c4af52..b10c5c46 100644 --- a/src/lint/linter/ArcanistTextLinter.php +++ b/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, '. - 'you will save time.'), + 'up your editor to automatically remove trailing whitespace.'), $string, ''); } diff --git a/src/lint/linter/__tests__/ArcanistLinterTestCase.php b/src/lint/linter/__tests__/ArcanistLinterTestCase.php index a6dc69b5..85144bc2 100644 --- a/src/lint/linter/__tests__/ArcanistLinterTestCase.php +++ b/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($path_name)); + $engine->setPaths(array($basename)); $linter->setEngine($engine); - $linter->addPath($path_name); - $linter->addData($path_name, $data); + $linter->addPath($basename); + $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); - $char = idx($parts, 2); - $code = idx($parts, 3); + $line = idx($parts, 1); + if ($line === '') { + $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", - pht("Lint failed for '%s'.", $file), - $expected, - $actual)); + "%s\n%s%s", + pht( + 'Lint emitted an unexpected set of messages for file "%s".', + $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(''); + } else { + $display_line = $line; + } + + $char = $message->getChar(); + if ($char === null) { + $display_char = pht(''); + } 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); } } diff --git a/src/lint/linter/__tests__/chmod/empty_executable.lint-test b/src/lint/linter/__tests__/chmod/empty_executable.lint-test index cf8980b6..695d3b8e 100644 --- a/src/lint/linter/__tests__/chmod/empty_executable.lint-test +++ b/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"} diff --git a/src/lint/linter/__tests__/cpplint/googlestyle.lint-test b/src/lint/linter/__tests__/cpplint/googlestyle.lint-test index 5417f073..0c5b1b79 100644 --- a/src/lint/linter/__tests__/cpplint/googlestyle.lint-test +++ b/src/lint/linter/__tests__/cpplint/googlestyle.lint-test @@ -1,10 +1,21 @@ #include "library.cpp" #include +void main() +{ +} +~~~~~~~~~ +warning +warning:2 +warning:5 +~~~~~~~~~~ +#include "library.cpp" +#include + void main() { } ~~~~~~~~~~ -warning:0: -warning:2: -warning:5: +{ + "path": "example.cpp" +} diff --git a/src/lint/linter/__tests__/filename/bad_filename.lint-test b/src/lint/linter/__tests__/filename/bad_filename.lint-test index 12ec5757..28b7e560 100644 --- a/src/lint/linter/__tests__/filename/bad_filename.lint-test +++ b/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"} diff --git a/src/lint/linter/__tests__/php/fatal.lint-test b/src/lint/linter/__tests__/php/fatal.lint-test index 6d3c7b82..04bd5c86 100644 --- a/src/lint/linter/__tests__/php/fatal.lint-test +++ b/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 diff --git a/src/lint/linter/__tests__/php/syntax.lint-test b/src/lint/linter/__tests__/php/syntax.lint-test index 279fb7af..098a9f68 100644 --- a/src/lint/linter/__tests__/php/syntax.lint-test +++ b/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 diff --git a/src/lint/linter/__tests__/ruby/hello.lint-test b/src/lint/linter/__tests__/ruby/hello.lint-test index f6693b6c..a9bf9016 100644 --- a/src/lint/linter/__tests__/ruby/hello.lint-test +++ b/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 diff --git a/src/lint/linter/__tests__/text/empty.lint-test b/src/lint/linter/__tests__/text/empty.lint-test index 72fb27ca..d4bf5989 100644 --- a/src/lint/linter/__tests__/text/empty.lint-test +++ b/src/lint/linter/__tests__/text/empty.lint-test @@ -1,4 +1,4 @@ ~~~~~~~~~~ -error:0:0:TXT10:Empty File +error:::TXT10:Empty File diff --git a/src/lint/linter/__tests__/xhpast/empty.lint-test b/src/lint/linter/__tests__/xhpast/empty.lint-test index 22cca1cd..c0b9b003 100644 --- a/src/lint/linter/__tests__/xhpast/empty.lint-test +++ b/src/lint/linter/__tests__/xhpast/empty.lint-test @@ -1,4 +1,4 @@