Enforce Python style on testing/tools
Opts PDFium testing/tools scripts into Pylint and auto-formatting,
following the directions at
https://chromium.googlesource.com/chromium/src/+/master/styleguide/python/python.md.
Cleaned up existing lint warnings:
1. Removed unused parameters and variables.
2. Renamed shadowing names.
3. Wrapped top-level statements to fix scoping issues.
4. Fixed a long line.
5. Suppressed warnings about (implicit) relative imports. Fixing this
properly will require separating "scripts" and "packages".
6. Suppressed warnings about defining attributes in Run() methods.
Miscellaneous changes:
7. Filtered non-C++ files from top-level CheckChangeLintsClean check.
(This check runs cpplint, and was triggering on PRESUBMIT.py.)
Change-Id: I8c25b3df433420bdfc9310e93f7e82d21396a458
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/63153
Commit-Queue: K Moon <kmoon@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index 0567def..9ad4abe 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -308,11 +308,14 @@
return results
def CheckChangeOnUpload(input_api, output_api):
+ cpp_source_filter = lambda x: input_api.FilterSourceFile(
+ x, white_list=(r'\.(?:c|cc|cpp|h)$',))
+
results = []
results += _CheckUnwantedDependencies(input_api, output_api)
results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api)
results += input_api.canned_checks.CheckChangeLintsClean(
- input_api, output_api, None, LINT_FILTERS)
+ input_api, output_api, cpp_source_filter, LINT_FILTERS)
results += _CheckIncludeOrder(input_api, output_api)
results += _CheckTestDuplicates(input_api, output_api)
results += _CheckPNGFormat(input_api, output_api)
diff --git a/testing/tools/.style.yapf b/testing/tools/.style.yapf
new file mode 100644
index 0000000..de0c6a7
--- /dev/null
+++ b/testing/tools/.style.yapf
@@ -0,0 +1,2 @@
+[style]
+based_on_style = chromium
diff --git a/testing/tools/PRESUBMIT.py b/testing/tools/PRESUBMIT.py
new file mode 100644
index 0000000..59a3858
--- /dev/null
+++ b/testing/tools/PRESUBMIT.py
@@ -0,0 +1,26 @@
+# Copyright 2019 The PDFium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+"""Presubmit script for PDFium testing tools.
+
+See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
+for more details on the presubmit API built into depot_tools.
+"""
+
+
+def _CommonChecks(input_api, output_api):
+ tests = []
+ tests.extend(input_api.canned_checks.GetPylint(input_api, output_api))
+ return tests
+
+
+def CheckChangeOnUpload(input_api, output_api):
+ tests = []
+ tests.extend(_CommonChecks(input_api, output_api))
+ return input_api.RunTests(tests)
+
+
+def CheckChangeOnCommit(input_api, output_api):
+ tests = []
+ tests.extend(_CommonChecks(input_api, output_api))
+ return input_api.RunTests(tests)
diff --git a/testing/tools/api_check.py b/testing/tools/api_check.py
index 0411432..66b3077 100755
--- a/testing/tools/api_check.py
+++ b/testing/tools/api_check.py
@@ -71,7 +71,7 @@
return functions
-def _CheckSorted(functions, api_test_path):
+def _CheckSorted(functions):
unsorted_functions = set()
for i in range(len(functions) - 1):
if functions[i] > functions[i + 1]:
@@ -123,7 +123,7 @@
result = True
unsorted_functions = set()
for functions in test_functions_per_section:
- unsorted_functions |= _CheckSorted(functions, api_test_path)
+ unsorted_functions |= _CheckSorted(functions)
check = _CheckAndPrintFailures(
unsorted_functions,
'Found CHKs that are not in alphabetical order within each section in %s'
diff --git a/testing/tools/coverage/coverage_report.py b/testing/tools/coverage/coverage_report.py
index d6b6f80..65b12fc 100755
--- a/testing/tools/coverage/coverage_report.py
+++ b/testing/tools/coverage/coverage_report.py
@@ -76,8 +76,8 @@
if not GetBooleanGnArg('use_clang_coverage', self.build_directory,
self.verbose):
parser.error(
- 'use_clang_coverage does not appear to be set to true for build, but is '
- 'needed')
+ 'use_clang_coverage does not appear to be set to true for build, but '
+ 'is needed')
self.use_goma = GetBooleanGnArg('use_goma', self.build_directory,
self.verbose)
diff --git a/testing/tools/githelper.py b/testing/tools/githelper.py
index 61d5954..91a6c71 100644
--- a/testing/tools/githelper.py
+++ b/testing/tools/githelper.py
@@ -5,6 +5,7 @@
import subprocess
+# pylint: disable=relative-import
from common import RunCommandPropagateErr
diff --git a/testing/tools/gold.py b/testing/tools/gold.py
index 3e552a3..c686bba 100644
--- a/testing/tools/gold.py
+++ b/testing/tools/gold.py
@@ -250,7 +250,7 @@
# Produce example output for manual testing.
-if __name__ == '__main__':
+def _Example():
# Create a test directory with three empty 'image' files.
test_dir = './testdirectory'
if not os.path.exists(test_dir):
@@ -278,3 +278,7 @@
gr.AddTestResult('test-3', 'hash-3', os.path.join(test_dir, 'image3.png'),
GoldBaseline.MISMATCH)
gr.WriteResults()
+
+
+if __name__ == '__main__':
+ _Example()
diff --git a/testing/tools/pngdiffer.py b/testing/tools/pngdiffer.py
index 3febf70..a469506 100755
--- a/testing/tools/pngdiffer.py
+++ b/testing/tools/pngdiffer.py
@@ -9,6 +9,7 @@
import shutil
import sys
+# pylint: disable=relative-import
import common
diff --git a/testing/tools/run_corpus_tests.py b/testing/tools/run_corpus_tests.py
index 5c1a207..c1bec3a 100755
--- a/testing/tools/run_corpus_tests.py
+++ b/testing/tools/run_corpus_tests.py
@@ -5,6 +5,7 @@
import sys
+# pylint: disable=relative-import
import test_runner
diff --git a/testing/tools/run_javascript_tests.py b/testing/tools/run_javascript_tests.py
index cb6e69a..948c1af 100755
--- a/testing/tools/run_javascript_tests.py
+++ b/testing/tools/run_javascript_tests.py
@@ -5,6 +5,7 @@
import sys
+# pylint: disable=relative-import
import test_runner
diff --git a/testing/tools/run_pixel_tests.py b/testing/tools/run_pixel_tests.py
index 92523d1..0992e0a 100755
--- a/testing/tools/run_pixel_tests.py
+++ b/testing/tools/run_pixel_tests.py
@@ -5,6 +5,7 @@
import sys
+# pylint: disable=relative-import
import test_runner
diff --git a/testing/tools/safetynet_compare.py b/testing/tools/safetynet_compare.py
index 00c67a8..c76ce44 100755
--- a/testing/tools/safetynet_compare.py
+++ b/testing/tools/safetynet_compare.py
@@ -16,6 +16,7 @@
import sys
import tempfile
+# pylint: disable=relative-import
from common import GetBooleanGnArg
from common import PrintErr
from common import RunCommandPropagateErr
@@ -122,8 +123,8 @@
self.__FreezeFile(os.path.join('testing', 'tools', 'safetynet_measure.py'))
self.__FreezeFile(os.path.join('testing', 'tools', 'common.py'))
- def __FreezeFile(self, file):
- RunCommandPropagateErr(['cp', file, self.safe_script_dir],
+ def __FreezeFile(self, filename):
+ RunCommandPropagateErr(['cp', filename, self.safe_script_dir],
exit_status_on_error=1)
def _ProfileTwoOtherBranchesInThisRepo(self, before_branch, after_branch):
diff --git a/testing/tools/safetynet_image.py b/testing/tools/safetynet_image.py
index 628ed04..f300615 100644
--- a/testing/tools/safetynet_image.py
+++ b/testing/tools/safetynet_image.py
@@ -13,6 +13,7 @@
import sys
import webbrowser
+# pylint: disable=relative-import
from common import DirectoryFinder
@@ -58,6 +59,9 @@
Exit status.
"""
+ # Running a test defines a number of attributes on the fly.
+ # pylint: disable=attribute-defined-outside-init
+
if len(self.two_labels) != 2:
print >> sys.stderr, 'two_labels must be a tuple of length 2'
return 1
diff --git a/testing/tools/safetynet_job.py b/testing/tools/safetynet_job.py
index dba524a..9b5cbfd 100755
--- a/testing/tools/safetynet_job.py
+++ b/testing/tools/safetynet_job.py
@@ -15,6 +15,7 @@
import os
import sys
+# pylint: disable=relative-import
from common import PrintWithTime
from common import RunCommandPropagateErr
from githelper import GitHelper
diff --git a/testing/tools/safetynet_measure.py b/testing/tools/safetynet_measure.py
index fcda406..3577189 100755
--- a/testing/tools/safetynet_measure.py
+++ b/testing/tools/safetynet_measure.py
@@ -13,6 +13,7 @@
import subprocess
import sys
+# pylint: disable=relative-import
from common import PrintErr
CALLGRIND_PROFILER = 'callgrind'
@@ -113,7 +114,7 @@
case, always return 1 since no profiler is being used.
"""
cmd_to_run = self._BuildTestHarnessCommand()
- output = subprocess.check_output(cmd_to_run, stderr=subprocess.STDOUT)
+ subprocess.check_output(cmd_to_run, stderr=subprocess.STDOUT)
# Return 1 for every run.
return 1
diff --git a/testing/tools/suppressor.py b/testing/tools/suppressor.py
index f3c00f4..a9f9508 100755
--- a/testing/tools/suppressor.py
+++ b/testing/tools/suppressor.py
@@ -5,6 +5,7 @@
import os
+# pylint: disable=relative-import
import common
@@ -32,16 +33,18 @@
for x in f.readlines()] if y
]
- def _FilterSuppressions(self, os, js, xfa, unfiltered_list):
+ def _FilterSuppressions(self, os_name, js, xfa, unfiltered_list):
return [
- x[0] for x in unfiltered_list if self._MatchSuppression(x, os, js, xfa)
+ x[0]
+ for x in unfiltered_list
+ if self._MatchSuppression(x, os_name, js, xfa)
]
- def _MatchSuppression(self, item, os, js, xfa):
+ def _MatchSuppression(self, item, os_name, js, xfa):
os_column = item[1].split(",")
js_column = item[2].split(",")
xfa_column = item[3].split(",")
- return (('*' in os_column or os in os_column) and
+ return (('*' in os_column or os_name in os_column) and
('*' in js_column or js in js_column) and
('*' in xfa_column or xfa in xfa_column))
diff --git a/testing/tools/test_runner.py b/testing/tools/test_runner.py
index 69938c9..714bbd8 100644
--- a/testing/tools/test_runner.py
+++ b/testing/tools/test_runner.py
@@ -12,6 +12,7 @@
import subprocess
import sys
+# pylint: disable=relative-import
import common
import gold
import pngdiffer
@@ -95,7 +96,7 @@
raised_exception = self.TestText(input_root, expected_txt_path, pdf_path)
else:
use_ahem = 'use_ahem' in source_dir
- raised_exception, results = self.TestPixel(input_root, pdf_path, use_ahem)
+ raised_exception, results = self.TestPixel(pdf_path, use_ahem)
if raised_exception is not None:
print 'FAILURE: %s; %s' % (input_filename, raised_exception)
@@ -177,7 +178,7 @@
except Exception as e:
return e
- def TestPixel(self, input_root, pdf_path, use_ahem):
+ def TestPixel(self, pdf_path, use_ahem):
cmd_to_run = [
self.pdfium_test_path, '--send-events', '--png', '--md5',
'--time=' + TEST_SEED_TIME
@@ -226,6 +227,9 @@
self.failures.append(input_path)
def Run(self):
+ # Running a test defines a number of attributes on the fly.
+ # pylint: disable=attribute-defined-outside-init
+
parser = optparse.OptionParser()
parser.add_option(
