From 05db68245ba63bc908d09c1cc64098ce97fa9d1f Mon Sep 17 00:00:00 2001 From: Richard Lau Date: Wed, 6 May 2020 11:20:14 -0400 Subject: [PATCH] tools: fix check-imports.py to match on word boundaries `check-imports.py` was missing some unused `using` statements as it was not matching on word boundaries (e.g. `MaybeLocal` was considered a use of `Local`). Fix that and add some unit tests (which required the script to be renamed to drop the `-` so it could be imported into the test script). PR-URL: https://github.com/nodejs/node/pull/33268 Refs: https://github.com/nodejs/node/issues/29226 Reviewed-By: Ben Noordhuis Reviewed-By: Ruben Bridgewater Reviewed-By: Christian Clauss Reviewed-By: Daniel Bevenius Reviewed-By: Beth Griggs --- Makefile | 4 +- test/fixtures/tools/checkimports/invalid.cc | 7 ++ test/fixtures/tools/checkimports/maybe.cc | 7 ++ test/fixtures/tools/checkimports/unsorted.cc | 6 ++ test/fixtures/tools/checkimports/unused.cc | 5 ++ test/fixtures/tools/checkimports/valid.cc | 6 ++ test/tools/test_checkimports.py | 77 ++++++++++++++++++++ tools/{check-imports.py => checkimports.py} | 6 +- 8 files changed, 114 insertions(+), 4 deletions(-) create mode 100644 test/fixtures/tools/checkimports/invalid.cc create mode 100644 test/fixtures/tools/checkimports/maybe.cc create mode 100644 test/fixtures/tools/checkimports/unsorted.cc create mode 100644 test/fixtures/tools/checkimports/unused.cc create mode 100644 test/fixtures/tools/checkimports/valid.cc create mode 100644 test/tools/test_checkimports.py rename tools/{check-imports.py => checkimports.py} (84%) diff --git a/Makefile b/Makefile index 7a6e3d5c32d161..519e920685177f 100644 --- a/Makefile +++ b/Makefile @@ -1328,13 +1328,13 @@ else CPPLINT_QUIET = --quiet endif .PHONY: lint-cpp -# Lints the C++ code with cpplint.py and check-imports.py. +# Lints the C++ code with cpplint.py and checkimports.py. lint-cpp: tools/.cpplintstamp tools/.cpplintstamp: $(LINT_CPP_FILES) @echo "Running C++ linter..." @$(PYTHON) tools/cpplint.py $(CPPLINT_QUIET) $? - @$(PYTHON) tools/check-imports.py + @$(PYTHON) tools/checkimports.py @touch $@ .PHONY: lint-addon-docs diff --git a/test/fixtures/tools/checkimports/invalid.cc b/test/fixtures/tools/checkimports/invalid.cc new file mode 100644 index 00000000000000..deace1287f11bb --- /dev/null +++ b/test/fixtures/tools/checkimports/invalid.cc @@ -0,0 +1,7 @@ +using v8::MaybeLocal; +using v8::Array; +using v8::Local; + +MaybeLocal CreateObject() const { + return MaybeLocal(); +} diff --git a/test/fixtures/tools/checkimports/maybe.cc b/test/fixtures/tools/checkimports/maybe.cc new file mode 100644 index 00000000000000..e96899be775ae7 --- /dev/null +++ b/test/fixtures/tools/checkimports/maybe.cc @@ -0,0 +1,7 @@ +using v8::Array; +using v8::Local; +using v8::MaybeLocal; + +MaybeLocal CreateObject() const { + return MaybeLocal(); +} diff --git a/test/fixtures/tools/checkimports/unsorted.cc b/test/fixtures/tools/checkimports/unsorted.cc new file mode 100644 index 00000000000000..0e6b540dccc466 --- /dev/null +++ b/test/fixtures/tools/checkimports/unsorted.cc @@ -0,0 +1,6 @@ +using v8::MaybeLocal; +using v8::Array; + +MaybeLocal CreateObject() const { + return MaybeLocal(); +} diff --git a/test/fixtures/tools/checkimports/unused.cc b/test/fixtures/tools/checkimports/unused.cc new file mode 100644 index 00000000000000..231f5549e2900c --- /dev/null +++ b/test/fixtures/tools/checkimports/unused.cc @@ -0,0 +1,5 @@ +using v8::Context; + +static void MyMethod(void) { + return; +} diff --git a/test/fixtures/tools/checkimports/valid.cc b/test/fixtures/tools/checkimports/valid.cc new file mode 100644 index 00000000000000..7e52968883ad8b --- /dev/null +++ b/test/fixtures/tools/checkimports/valid.cc @@ -0,0 +1,6 @@ +using v8::Array; +using v8::MaybeLocal; + +MaybeLocal CreateObject() const { + return MaybeLocal(); +} diff --git a/test/tools/test_checkimports.py b/test/tools/test_checkimports.py new file mode 100644 index 00000000000000..6e8e17cc3f9c23 --- /dev/null +++ b/test/tools/test_checkimports.py @@ -0,0 +1,77 @@ +import unittest +import sys +from contextlib import contextmanager +from os import path +sys.path.append(path.abspath(path.join(path.dirname(__file__), + '..', '..', 'tools'))) +try: + from StringIO import StringIO +except ImportError: + from io import StringIO + +from checkimports import is_valid + +@contextmanager +def captured_output(): + tmp_out, tmp_err = StringIO(), StringIO() + old_out, old_err = sys.stdout, sys.stderr + try: + sys.stdout, sys.stderr = tmp_out, tmp_err + yield sys.stdout, sys.stderr + finally: + sys.stdout, sys.stderr = old_out, old_err + tmp_out.close() + tmp_err.close() + +class CheckImportsTest(unittest.TestCase): + fixturesDir = path.join(path.dirname(__file__), '..', '..', + 'test', 'fixtures', 'tools', 'checkimports') + + def test_unused_and_unsorted(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'invalid.cc')), + False) + output = out.getvalue() + self.assertIn('does not use "Local"', output); + self.assertIn('using statements aren\'t sorted in', output); + self.assertIn('Line 1: Actual: v8::MaybeLocal, Expected: v8::Array', + output); + self.assertIn('Line 2: Actual: v8::Array, Expected: v8::Local', + output); + self.assertIn('Line 3: Actual: v8::Local, Expected: v8::MaybeLocal', + output); + + def test_unused_complex(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'maybe.cc')), + False) + output = out.getvalue() + self.assertIn('does not use "Local"', output); + + def test_unused_simple(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'unused.cc')), + False) + output = out.getvalue() + self.assertIn('does not use "Context"', output); + + def test_unsorted(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'unsorted.cc')), + False) + output = out.getvalue() + self.assertIn('using statements aren\'t sorted in', output); + self.assertIn('Line 1: Actual: v8::MaybeLocal, Expected: v8::Array', + output); + self.assertIn('Line 2: Actual: v8::Array, Expected: v8::MaybeLocal', + output); + + def test_valid(self): + with captured_output() as (out, err): + self.assertEqual(is_valid(path.join(self.fixturesDir, 'valid.cc')), + True) + output = out.getvalue() + self.assertEqual(output, ''); + +if __name__ == '__main__': + unittest.main() diff --git a/tools/check-imports.py b/tools/checkimports.py similarity index 84% rename from tools/check-imports.py rename to tools/checkimports.py index 51b4e63aa03903..609a75f542748f 100755 --- a/tools/check-imports.py +++ b/tools/checkimports.py @@ -9,7 +9,7 @@ def do_exist(file_name, lines, imported): if not any(not re.match('using \w+::{0};'.format(imported), line) and - re.search(imported, line) for line in lines): + re.search('\\b{0}\\b'.format(imported), line) for line in lines): print('File "{0}" does not use "{1}"'.format(file_name, imported)) return False return True @@ -40,4 +40,6 @@ def is_valid(file_name): else: return valid -sys.exit(0 if all(map(is_valid, glob.iglob('src/*.cc'))) else 1) +if __name__ == '__main__': + files = glob.iglob(sys.argv[1] if len(sys.argv) > 1 else 'src/*.cc') + sys.exit(0 if all(map(is_valid, files)) else 1)