From 0e8fd7b4e5e1e8b0930ed9f48105ea201ef33f90 Mon Sep 17 00:00:00 2001 From: Chris Mumford Date: Mon, 24 Apr 2023 14:04:54 -0700 Subject: [PATCH] Add unit tests for PRESUBMIT.py Add a unit test program, and mocks, for PRESUBMIT.py. Additionally add unit tests for two newly added presubmit checks which landed in http://review.skia.org/668416. These tests also identified a bug in _CheckReleaseNotesForPublicAPI. That fix is also in this change. Bug: skia:14276 Change-Id: I90d86e4d5216067484340e85e31f5a33a3eeba3a Reviewed-on: https://skia-review.googlesource.com/c/skia/+/680017 Reviewed-by: Ravi Mistry Commit-Queue: Chris Mumford Reviewed-by: Kevin Lubick --- PRESUBMIT.py | 2 +- PRESUBMIT_test.py | 89 +++++++++++++ PRESUBMIT_test_mocks.py | 285 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 375 insertions(+), 1 deletion(-) create mode 100755 PRESUBMIT_test.py create mode 100644 PRESUBMIT_test_mocks.py diff --git a/PRESUBMIT.py b/PRESUBMIT.py index d7d0b012735d..8d8300337402 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -613,7 +613,7 @@ def _CheckReleaseNotesForPublicAPI(input_api, output_api): # We only care about files that end in .h and are under the top-level # include dir, but not include/private. if (file_ext == '.h' and - os.path.dirname(file_path) == 'include' and + file_path.split(os.path.sep)[0] == 'include' and 'private' not in file_path): public_api_changed = True elif os.path.dirname(file_path) == RELEASE_NOTES_DIR: diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py new file mode 100755 index 000000000000..fc75617080fd --- /dev/null +++ b/PRESUBMIT_test.py @@ -0,0 +1,89 @@ +#!/usr/bin/env python3 +# Copyright 2023 The Chromium Authors +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import io +import os.path +import subprocess +import textwrap +import unittest + +import PRESUBMIT + +from PRESUBMIT_test_mocks import MockFile, MockAffectedFile +from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi + + +class ReleaseNotesTest(unittest.TestCase): + def testNoEditTopReleaseNotesNoWarning(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockFile('README.chromium', ''), + ] + + mock_output_api = MockOutputApi() + results = PRESUBMIT._CheckTopReleaseNotesChanged( + mock_input_api, mock_output_api) + + self.assertEqual(0, len(results)) + + def testUpdateTopReleaseNotesIssuesWarning(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockFile('RELEASE_NOTES.md', ''), + ] + + mock_output_api = MockOutputApi() + results = PRESUBMIT._CheckTopReleaseNotesChanged( + mock_input_api, mock_output_api) + + self.assertEqual(1, len(results)) + self.assertIsInstance( + results[0], mock_output_api.PresubmitPromptWarning, 'Not a warning') + self.assertTrue(results[0].message.startswith( + 'Do not edit RELEASE_NOTES.md')) + + def testUpdateTopReleaseNotesNoWarning(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockFile('RELEASE_NOTES.md', ''), + MockFile('relnotes/deleted_note.md', ''), + ] + + mock_output_api = MockOutputApi() + results = PRESUBMIT._CheckTopReleaseNotesChanged( + mock_input_api, mock_output_api) + + self.assertEqual(0, len(results)) + + def testUpdatePublicHeaderAndNoReleaseNoteGeneratesWarning(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockFile('include/core/SkDrawable.h', ''), + ] + + mock_output_api = MockOutputApi() + results = PRESUBMIT._CheckReleaseNotesForPublicAPI( + mock_input_api, mock_output_api) + + self.assertEqual(1, len(results)) + self.assertIsInstance( + results[0], mock_output_api.PresubmitPromptWarning, 'Not a warning') + + def testUpdatePublicHeaderAndReleaseNoteGeneratesNoWarning(self): + mock_input_api = MockInputApi() + mock_input_api.files = [ + MockFile('include/core/SkDrawable.h', ''), + MockFile('relnotes/new_note.md', ''), + ] + + mock_output_api = MockOutputApi() + results = PRESUBMIT._CheckReleaseNotesForPublicAPI( + mock_input_api, mock_output_api) + + self.assertEqual(0, len(results)) + + +if __name__ == '__main__': + unittest.main() diff --git a/PRESUBMIT_test_mocks.py b/PRESUBMIT_test_mocks.py new file mode 100644 index 000000000000..67504c16b1a9 --- /dev/null +++ b/PRESUBMIT_test_mocks.py @@ -0,0 +1,285 @@ +# Copyright 2023 The Chromium Authors +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +# This is a copy of PRESUBMIT_test_mocks.py from the Chromium project. + +from collections import defaultdict +import fnmatch +import json +import os +import re +import subprocess +import sys + + +def _ReportErrorFileAndLine(filename, line_num, dummy_line): + """Default error formatter for _FindNewViolationsOfRule.""" + return '%s:%s' % (filename, line_num) + + +class MockCannedChecks(object): + def _FindNewViolationsOfRule(self, callable_rule, input_api, + source_file_filter=None, + error_formatter=_ReportErrorFileAndLine): + """Find all newly introduced violations of a per-line rule (a callable). + + Arguments: + callable_rule: a callable taking a file extension and line of input and + returning True if the rule is satisfied and False if there was a + problem. + input_api: object to enumerate the affected files. + source_file_filter: a filter to be passed to the input api. + error_formatter: a callable taking (filename, line_number, line) and + returning a formatted error string. + + Returns: + A list of the newly-introduced violations reported by the rule. + """ + errors = [] + for f in input_api.AffectedFiles(include_deletes=False, + file_filter=source_file_filter): + # For speed, we do two passes, checking first the full file. Shelling out + # to the SCM to determine the changed region can be quite expensive on + # Win32. Assuming that most files will be kept problem-free, we can + # skip the SCM operations most of the time. + extension = str(f.LocalPath()).rsplit('.', 1)[-1] + if all(callable_rule(extension, line) for line in f.NewContents()): + # No violation found in full text: can skip considering diff. + continue + + for line_num, line in f.ChangedContents(): + if not callable_rule(extension, line): + errors.append(error_formatter( + f.LocalPath(), line_num, line)) + + return errors + + +class MockInputApi(object): + """Mock class for the InputApi class. + + This class can be used for unittests for presubmit by initializing the files + attribute as the list of changed files. + """ + + DEFAULT_FILES_TO_SKIP = () + + def __init__(self): + self.canned_checks = MockCannedChecks() + self.fnmatch = fnmatch + self.json = json + self.re = re + self.os_path = os.path + self.platform = sys.platform + self.python_executable = sys.executable + self.python3_executable = sys.executable + self.platform = sys.platform + self.subprocess = subprocess + self.sys = sys + self.files = [] + self.is_committing = False + self.change = MockChange([]) + self.presubmit_local_path = os.path.dirname(__file__) + self.is_windows = sys.platform == 'win32' + self.no_diffs = False + # Although this makes assumptions about command line arguments used by test + # scripts that create mocks, it is a convenient way to set up the verbosity + # via the input api. + self.verbose = '--verbose' in sys.argv + + def CreateMockFileInPath(self, f_list): + self.os_path.exists = lambda x: x in f_list + + def AffectedFiles(self, file_filter=None, include_deletes=True): + for file in self.files: + if file_filter and not file_filter(file): + continue + if not include_deletes and file.Action() == 'D': + continue + yield file + + def RightHandSideLines(self, source_file_filter=None): + affected_files = self.AffectedSourceFiles(source_file_filter) + for af in affected_files: + lines = af.ChangedContents() + for line in lines: + yield (af, line[0], line[1]) + + def AffectedSourceFiles(self, file_filter=None): + return self.AffectedFiles(file_filter=file_filter) + + def FilterSourceFile(self, file, + files_to_check=(), files_to_skip=()): + local_path = file.LocalPath() + found_in_files_to_check = not files_to_check + if files_to_check: + if type(files_to_check) is str: + raise TypeError( + 'files_to_check should be an iterable of strings') + for pattern in files_to_check: + compiled_pattern = re.compile(pattern) + if compiled_pattern.match(local_path): + found_in_files_to_check = True + break + if files_to_skip: + if type(files_to_skip) is str: + raise TypeError( + 'files_to_skip should be an iterable of strings') + for pattern in files_to_skip: + compiled_pattern = re.compile(pattern) + if compiled_pattern.match(local_path): + return False + return found_in_files_to_check + + def LocalPaths(self): + return [file.LocalPath() for file in self.files] + + def PresubmitLocalPath(self): + return self.presubmit_local_path + + def ReadFile(self, filename, mode='r'): + if hasattr(filename, 'AbsoluteLocalPath'): + filename = filename.AbsoluteLocalPath() + for file_ in self.files: + if file_.LocalPath() == filename: + return '\n'.join(file_.NewContents()) + # Otherwise, file is not in our mock API. + raise IOError("No such file or directory: '%s'" % filename) + + +class MockOutputApi(object): + """Mock class for the OutputApi class. + + An instance of this class can be passed to presubmit unittests for outputting + various types of results. + """ + + class PresubmitResult(object): + def __init__(self, message, items=None, long_text=''): + self.message = message + self.items = items + self.long_text = long_text + + def __repr__(self): + return self.message + + class PresubmitError(PresubmitResult): + def __init__(self, message, items=None, long_text=''): + MockOutputApi.PresubmitResult.__init__( + self, message, items, long_text) + self.type = 'error' + + class PresubmitPromptWarning(PresubmitResult): + def __init__(self, message, items=None, long_text=''): + MockOutputApi.PresubmitResult.__init__( + self, message, items, long_text) + self.type = 'warning' + + class PresubmitNotifyResult(PresubmitResult): + def __init__(self, message, items=None, long_text=''): + MockOutputApi.PresubmitResult.__init__( + self, message, items, long_text) + self.type = 'notify' + + class PresubmitPromptOrNotify(PresubmitResult): + def __init__(self, message, items=None, long_text=''): + MockOutputApi.PresubmitResult.__init__( + self, message, items, long_text) + self.type = 'promptOrNotify' + + def __init__(self): + self.more_cc = [] + + def AppendCC(self, more_cc): + self.more_cc.append(more_cc) + + +class MockFile(object): + """Mock class for the File class. + + This class can be used to form the mock list of changed files in + MockInputApi for presubmit unittests. + """ + + def __init__(self, local_path, new_contents, old_contents=None, action='A', + scm_diff=None): + self._local_path = local_path + self._new_contents = new_contents + self._changed_contents = [(i + 1, l) + for i, l in enumerate(new_contents)] + self._action = action + if scm_diff: + self._scm_diff = scm_diff + else: + self._scm_diff = ( + "--- /dev/null\n+++ %s\n@@ -0,0 +1,%d @@\n" % + (local_path, len(new_contents))) + for l in new_contents: + self._scm_diff += "+%s\n" % l + self._old_contents = old_contents + + def Action(self): + return self._action + + def ChangedContents(self): + return self._changed_contents + + def NewContents(self): + return self._new_contents + + def LocalPath(self): + return self._local_path + + def AbsoluteLocalPath(self): + return self._local_path + + def GenerateScmDiff(self): + return self._scm_diff + + def OldContents(self): + return self._old_contents + + def rfind(self, p): + """os.path.basename is called on MockFile so we need an rfind method.""" + return self._local_path.rfind(p) + + def __getitem__(self, i): + """os.path.basename is called on MockFile so we need a get method.""" + return self._local_path[i] + + def __len__(self): + """os.path.basename is called on MockFile so we need a len method.""" + return len(self._local_path) + + def replace(self, altsep, sep): + """os.path.basename is called on MockFile so we need a replace method.""" + return self._local_path.replace(altsep, sep) + + +class MockAffectedFile(MockFile): + def AbsoluteLocalPath(self): + return self._local_path + + +class MockChange(object): + """Mock class for Change class. + + This class can be used in presubmit unittests to mock the query of the + current change. + """ + + def __init__(self, changed_files): + self._changed_files = changed_files + self.author_email = None + self.footers = defaultdict(list) + + def LocalPaths(self): + return self._changed_files + + def AffectedFiles(self, include_dirs=False, include_deletes=True, + file_filter=None): + return self._changed_files + + def GitFootersFromDescription(self): + return self.footers