From b3b2da46087966021cfc1b54a835820cae28cb5b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 13 Feb 2011 11:57:14 -0800 Subject: [PATCH] Fix greediness in Apache License linter. Summary: The multi-line comment regexp was potentially too greedy. See "greedy.lint-test". - Made it less greedy. - Added test coverage. - Fixed an issue with the Apache license getting applied with too much whitespace against C files. Test Plan: Ran unit tests. Reviewers: aran CC: Differential Revision: 36 --- src/__phutil_library_map__.php | 6 +- .../ArcanistApacheLicenseLinter.php | 9 +- .../ArcanistApacheLicenseLinterTestCase.php | 26 +++ .../apachelicense/__tests__/__init__.php | 13 ++ .../__tests__/data/c-basic.lint-test | 29 +++ .../__tests__/data/greedy.lint-test | 37 ++++ .../__tests__/data/php-basic.lint-test | 25 +++ .../__tests__/data/php-script.lint-test | 27 +++ .../__tests__/data/php-update-mess.lint-test | 29 +++ .../__tests__/data/php-update-multi.lint-test | 27 +++ .../data/php-update-single.lint-test | 27 +++ .../base/test/ArcanistLinterTestCase.php | 172 ++++++++++++++++++ src/lint/linter/base/test/__init__.php | 18 ++ .../linter/license/ArcanistLicenseLinter.php | 2 +- .../ArcanistXHPASTLinterTestCase.php | 152 +--------------- src/lint/linter/xhpast/__tests__/__init__.php | 8 +- 16 files changed, 448 insertions(+), 159 deletions(-) create mode 100644 src/lint/linter/apachelicense/__tests__/ArcanistApacheLicenseLinterTestCase.php create mode 100644 src/lint/linter/apachelicense/__tests__/__init__.php create mode 100644 src/lint/linter/apachelicense/__tests__/data/c-basic.lint-test create mode 100644 src/lint/linter/apachelicense/__tests__/data/greedy.lint-test create mode 100644 src/lint/linter/apachelicense/__tests__/data/php-basic.lint-test create mode 100644 src/lint/linter/apachelicense/__tests__/data/php-script.lint-test create mode 100644 src/lint/linter/apachelicense/__tests__/data/php-update-mess.lint-test create mode 100644 src/lint/linter/apachelicense/__tests__/data/php-update-multi.lint-test create mode 100644 src/lint/linter/apachelicense/__tests__/data/php-update-single.lint-test create mode 100644 src/lint/linter/base/test/ArcanistLinterTestCase.php create mode 100644 src/lint/linter/base/test/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0f12d494..2b53d7b8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -10,6 +10,7 @@ phutil_register_library_map(array( array( 'ArcanistAmendWorkflow' => 'workflow/amend', 'ArcanistApacheLicenseLinter' => 'lint/linter/apachelicense', + 'ArcanistApacheLicenseLinterTestCase' => 'lint/linter/apachelicense/__tests__', 'ArcanistBaseUnitTestEngine' => 'unit/engine/base', 'ArcanistBaseWorkflow' => 'workflow/base', 'ArcanistBundle' => 'parser/bundle', @@ -43,6 +44,7 @@ phutil_register_library_map(array( 'ArcanistLintSeverity' => 'lint/severity', 'ArcanistLintWorkflow' => 'workflow/lint', 'ArcanistLinter' => 'lint/linter/base', + 'ArcanistLinterTestCase' => 'lint/linter/base/test', 'ArcanistListWorkflow' => 'workflow/list', 'ArcanistMarkCommittedWorkflow' => 'workflow/mark-committed', 'ArcanistNoEffectException' => 'exception/usage/noeffect', @@ -77,6 +79,7 @@ phutil_register_library_map(array( array( 'ArcanistAmendWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistApacheLicenseLinter' => 'ArcanistLicenseLinter', + 'ArcanistApacheLicenseLinterTestCase' => 'ArcanistLinterTestCase', 'ArcanistCommitWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistCoverWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistDiffParserTestCase' => 'ArcanistPhutilTestCase', @@ -89,6 +92,7 @@ phutil_register_library_map(array( 'ArcanistHelpWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistLicenseLinter' => 'ArcanistLinter', 'ArcanistLintWorkflow' => 'ArcanistBaseWorkflow', + 'ArcanistLinterTestCase' => 'ArcanistPhutilTestCase', 'ArcanistListWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistMarkCommittedWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistNoEffectException' => 'ArcanistUsageException', @@ -103,7 +107,7 @@ phutil_register_library_map(array( 'ArcanistUnitWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistUserAbortException' => 'ArcanistUsageException', 'ArcanistXHPASTLinter' => 'ArcanistLinter', - 'ArcanistXHPASTLinterTestCase' => 'ArcanistPhutilTestCase', + 'ArcanistXHPASTLinterTestCase' => 'ArcanistLinterTestCase', 'PhutilLintEngine' => 'ArcanistLintEngine', 'PhutilUnitTestEngine' => 'ArcanistBaseUnitTestEngine', 'PhutilUnitTestEngineTestCase' => 'ArcanistPhutilTestCase', diff --git a/src/lint/linter/apachelicense/ArcanistApacheLicenseLinter.php b/src/lint/linter/apachelicense/ArcanistApacheLicenseLinter.php index d20f8575..3029b3b4 100644 --- a/src/lint/linter/apachelicense/ArcanistApacheLicenseLinter.php +++ b/src/lint/linter/apachelicense/ArcanistApacheLicenseLinter.php @@ -50,7 +50,14 @@ EOLICENSE; $maybe_php_or_script = '(#![^\n]+?[\n])?(<[?]php\s+?)?'; return array( "@^{$maybe_php_or_script}//[^\n]*Copyright[^\n]*[\n]\s*@i", - "@^{$maybe_php_or_script}/[*].*?Copyright.*?[*]/\s*@is", + + // We need to be careful about matching after "/*", since otherwise we'll + // end up in trouble on code like this, and consume the entire thing: + // + // /* a */ + // copyright(); + // /* b */ + "@^{$maybe_php_or_script}/[*](?:[^*]|[*][^/])*?Copyright.*?[*]/\s*@is", "@^{$maybe_php_or_script}\s*@", ); } diff --git a/src/lint/linter/apachelicense/__tests__/ArcanistApacheLicenseLinterTestCase.php b/src/lint/linter/apachelicense/__tests__/ArcanistApacheLicenseLinterTestCase.php new file mode 100644 index 00000000..b06c3ec1 --- /dev/null +++ b/src/lint/linter/apachelicense/__tests__/ArcanistApacheLicenseLinterTestCase.php @@ -0,0 +1,26 @@ +executeTestsInDirectory(dirname(__FILE__).'/data/', $linter); + } + +} diff --git a/src/lint/linter/apachelicense/__tests__/__init__.php b/src/lint/linter/apachelicense/__tests__/__init__.php new file mode 100644 index 00000000..d9c8c50f --- /dev/null +++ b/src/lint/linter/apachelicense/__tests__/__init__.php @@ -0,0 +1,13 @@ + + +int main(int argv, char **argv) { + return 0; +} +~~~~~~~~~~ +error:1:1 +~~~~~~~~~~ +/* + * Copyright 2011 Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +int main(int argv, char **argv) { + return 0; +} diff --git a/src/lint/linter/apachelicense/__tests__/data/greedy.lint-test b/src/lint/linter/apachelicense/__tests__/data/greedy.lint-test new file mode 100644 index 00000000..0f7879f0 --- /dev/null +++ b/src/lint/linter/apachelicense/__tests__/data/greedy.lint-test @@ -0,0 +1,37 @@ +lintFile($root.$file, $linter); + } + } + + private function lintFile($file, $linter) { + $linter = clone $linter; + + $working_copy = ArcanistWorkingCopyIdentity::newFromPath(__FILE__); + + $contents = Filesystem::readFile($file); + $contents = explode("~~~~~~~~~~\n", $contents); + if (count($contents) < 2) { + throw new Exception( + "Expected '~~~~~~~~~~' separating test case and results."); + } + + list ($data, $expect, $xform, $config) = array_merge( + $contents, + array(null, null)); + + $basename = basename($file); + + if ($config) { + $config = json_decode($config, true); + if (!is_array($config)) { + throw new Exception( + "Invalid configuration in test '{$basename}', not valid JSON."); + } + } else { + $config = array(); + } + + /* TODO: ? + validate_parameter_list( + $config, + array( + ), + array( + 'project' => true, + 'path' => true, + 'hook' => true, + )); + */ + + $exception = null; + $after_lint = null; + $messages = null; + $exception_message = false; + $caught_exception = false; + try { + + $path = idx($config, 'path', 'lint/'.$basename.'.php'); + + $engine = new UnitTestableArcanistLintEngine(); + $engine->setWorkingCopy($working_copy); + $engine->setPaths(array($path)); + +// TODO: restore this +// $engine->setCommitHookMode(idx($config, 'hook', false)); + + $linter->addPath($path); + $linter->addData($path, $data); + + $engine->addLinter($linter); + $engine->addFileData($path, $data); + + $results = $engine->run(); + $this->assertEqual( + 1, + count($results), + 'Expect one result returned by linter.'); + + $result = reset($results); + $patcher = ArcanistLintPatcher::newFromArcanistLintResult($result); + $after_lint = $patcher->getModifiedFileContent(); + + } catch (ArcanistPhutilTestTerminatedException $ex) { + throw $ex; + } catch (Exception $exception) { + $caught_exception = true; + $exception_message = $exception->getMessage()."\n\n". + $exception->getTraceAsString(); + } + + switch ($basename) { + default: + $this->assertEqual(false, $caught_exception, $exception_message); + $this->compareLint($basename, $expect, $result); + $this->compareTransform($xform, $after_lint); + break; + } + } + + private function compareLint($file, $expect, $result) { + $seen = array(); + $raised = array(); + foreach ($result->getMessages() as $message) { + $sev = $message->getSeverity(); + $line = $message->getLine(); + $char = $message->getChar(); + $code = $message->getCode(); + $name = $message->getName(); + $seen[] = $sev.":".$line.":".$char; + $raised[] = " {$sev} at line {$line}, char {$char}: {$code} {$name}"; + } + $expect = trim($expect); + if ($expect) { + $expect = explode("\n", $expect); + } else { + $expect = array(); + } + foreach ($expect as $key => $expected) { + $expect[$key] = reset(explode(' ', $expected)); + } + + $expect = array_fill_keys($expect, true); + $seen = array_fill_keys($seen, true); + + if (!$raised) { + $raised = array("No messages."); + } + $raised = "Actually raised:\n".implode("\n", $raised); + + foreach (array_diff_key($expect, $seen) as $missing => $ignored) { + list($sev, $line, $char) = explode(':', $missing); + $this->assertFailure( + "In '{$file}', ". + "expected lint to raise {$sev} on line {$line} at char {$char}, ". + "but no {$sev} was raised. {$raised}"); + } + + foreach (array_diff_key($seen, $expect) as $surprising => $ignored) { + list($sev, $line, $char) = explode(':', $surprising); + $this->assertFailure( + "In '{$file}', ". + "lint raised {$sev} on line {$line} at char {$char}, ". + "but nothing was expected. {$raised}"); + } + } + + private function compareTransform($expected, $actual) { + if (!strlen($expected)) { + return; + } + $this->assertEqual( + $expected, + $actual, + "File as patched by lint did not match the expected patched file."); + } +} diff --git a/src/lint/linter/base/test/__init__.php b/src/lint/linter/base/test/__init__.php new file mode 100644 index 00000000..c51dbca4 --- /dev/null +++ b/src/lint/linter/base/test/__init__.php @@ -0,0 +1,18 @@ +lintFile($root.$file); - } + $linter = new ArcanistXHPASTLinter(); + return $this->executeTestsInDirectory(dirname(__FILE__).'/data/', $linter); } - private function lintFile($file) { - $working_copy = ArcanistWorkingCopyIdentity::newFromPath(__FILE__); - - $contents = Filesystem::readFile($file); - $contents = explode("~~~~~~~~~~\n", $contents); - if (count($contents) < 2) { - throw new Exception( - "Expected '~~~~~~~~~~' separating test case and results."); - } - - list ($data, $expect, $xform, $config) = array_merge( - $contents, - array(null, null)); - - $basename = basename($file); - - if ($config) { - $config = json_decode($config, true); - if (!is_array($config)) { - throw new Exception( - "Invalid configuration in test '{$basename}', not valid JSON."); - } - } else { - $config = array(); - } - - /* TODO: ? - validate_parameter_list( - $config, - array( - ), - array( - 'project' => true, - 'path' => true, - 'hook' => true, - )); - */ - - $exception = null; - $after_lint = null; - $messages = null; - $exception_message = false; - $caught_exception = false; - try { - - $path = idx($config, 'path', 'lint/'.$basename.'.php'); - - $engine = new UnitTestableArcanistLintEngine(); - $engine->setWorkingCopy($working_copy); - $engine->setPaths(array($path)); - -// TODO: restore this -// $engine->setCommitHookMode(idx($config, 'hook', false)); - - $linter = new ArcanistXHPASTLinter(); - $linter->addPath($path); - $linter->addData($path, $data); - - $engine->addLinter($linter); - $engine->addFileData($path, $data); - - $results = $engine->run(); - $this->assertEqual( - 1, - count($results), - 'Expect one result returned by linter.'); - - $result = reset($results); - $patcher = ArcanistLintPatcher::newFromArcanistLintResult($result); - $after_lint = $patcher->getModifiedFileContent(); - - } catch (ArcanistPhutilTestTerminatedException $ex) { - throw $ex; - } catch (Exception $exception) { - $caught_exception = true; - $exception_message = $exception->getMessage()."\n\n". - $exception->getTraceAsString(); - } - - switch ($basename) { - default: - $this->assertEqual(false, $caught_exception, $exception_message); - $this->compareLint($basename, $expect, $result); - $this->compareTransform($xform, $after_lint); - break; - } - } - - private function compareLint($file, $expect, $result) { - $seen = array(); - $raised = array(); - foreach ($result->getMessages() as $message) { - $sev = $message->getSeverity(); - $line = $message->getLine(); - $char = $message->getChar(); - $code = $message->getCode(); - $name = $message->getName(); - $seen[] = $sev.":".$line.":".$char; - $raised[] = " {$sev} at line {$line}, char {$char}: {$code} {$name}"; - } - $expect = trim($expect); - if ($expect) { - $expect = explode("\n", $expect); - } else { - $expect = array(); - } - foreach ($expect as $key => $expected) { - $expect[$key] = reset(explode(' ', $expected)); - } - - $expect = array_fill_keys($expect, true); - $seen = array_fill_keys($seen, true); - - if (!$raised) { - $raised = array("No messages."); - } - $raised = "Actually raised:\n".implode("\n", $raised); - - foreach (array_diff_key($expect, $seen) as $missing => $ignored) { - list($sev, $line, $char) = explode(':', $missing); - $this->assertFailure( - "In '{$file}', ". - "expected lint to raise {$sev} on line {$line} at char {$char}, ". - "but no {$sev} was raised. {$raised}"); - } - - foreach (array_diff_key($seen, $expect) as $surprising => $ignored) { - list($sev, $line, $char) = explode(':', $surprising); - $this->assertFailure( - "In '{$file}', ". - "lint raised {$sev} on line {$line} at char {$char}, ". - "but nothing was expected. {$raised}"); - } - } - - private function compareTransform($expected, $actual) { - if (!strlen($expected)) { - return; - } - $this->assertEqual( - $expected, - $actual, - "File as patched by lint did not match the expected patched file."); - } } diff --git a/src/lint/linter/xhpast/__tests__/__init__.php b/src/lint/linter/xhpast/__tests__/__init__.php index b4fa3967..bed07e76 100644 --- a/src/lint/linter/xhpast/__tests__/__init__.php +++ b/src/lint/linter/xhpast/__tests__/__init__.php @@ -6,14 +6,8 @@ -phutil_require_module('arcanist', 'lint/engine/test'); +phutil_require_module('arcanist', 'lint/linter/base/test'); phutil_require_module('arcanist', 'lint/linter/xhpast'); -phutil_require_module('arcanist', 'lint/patcher'); -phutil_require_module('arcanist', 'unit/engine/phutil/testcase'); -phutil_require_module('arcanist', 'workingcopyidentity'); - -phutil_require_module('phutil', 'filesystem'); -phutil_require_module('phutil', 'utils'); phutil_require_source('ArcanistXHPASTLinterTestCase.php');