Skip to content

Commit

Permalink
tools: fix check-imports.py to match on word boundaries
Browse files Browse the repository at this point in the history
`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: #33268
Refs: #29226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
  • Loading branch information
richardlau committed May 28, 2020
1 parent 4a20cc9 commit 05db682
Show file tree
Hide file tree
Showing 8 changed files with 114 additions and 4 deletions.
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions test/fixtures/tools/checkimports/invalid.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
using v8::MaybeLocal;
using v8::Array;
using v8::Local;

MaybeLocal<Array> CreateObject() const {
return MaybeLocal<Array>();
}
7 changes: 7 additions & 0 deletions test/fixtures/tools/checkimports/maybe.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
using v8::Array;
using v8::Local;
using v8::MaybeLocal;

MaybeLocal<Array> CreateObject() const {
return MaybeLocal<Array>();
}
6 changes: 6 additions & 0 deletions test/fixtures/tools/checkimports/unsorted.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
using v8::MaybeLocal;
using v8::Array;

MaybeLocal<Array> CreateObject() const {
return MaybeLocal<Array>();
}
5 changes: 5 additions & 0 deletions test/fixtures/tools/checkimports/unused.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
using v8::Context;

static void MyMethod(void) {
return;
}
6 changes: 6 additions & 0 deletions test/fixtures/tools/checkimports/valid.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
using v8::Array;
using v8::MaybeLocal;

MaybeLocal<Array> CreateObject() const {
return MaybeLocal<Array>();
}
77 changes: 77 additions & 0 deletions test/tools/test_checkimports.py
Original file line number Diff line number Diff line change
@@ -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()
6 changes: 4 additions & 2 deletions tools/check-imports.py → tools/checkimports.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

0 comments on commit 05db682

Please sign in to comment.