diff --git a/presubmit_support.py b/presubmit_support.py index 40eb7fbd9b..926f20275b 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -36,7 +36,8 @@ import unittest # Exposed through the API. import urllib.parse as urlparse import urllib.request as urllib_request import urllib.error as urllib_error -from typing import Mapping +from dataclasses import asdict, dataclass +from typing import ClassVar, Mapping from warnings import warn # Local imports. @@ -311,6 +312,67 @@ def prompt_should_continue(prompt_string): return response in ('y', 'yes') +# Top level object so multiprocessing can pickle +# Public access through OutputApi object. +@dataclass +class _PresubmitResultLocation: + COMMIT_MSG_PATH: ClassVar[str] = '/COMMIT_MSG' + # path to the file where errors/warnings are reported. + # + # path MUST either be COMMIT_MSG_PATH or relative to the repo root to + # indicate the errors/warnings are against the commit message + # (a.k.a cl description). + file_path: str + # The range in the file defined by (start_line, start_col) - + # (end_line, end_col) where errors/warnings are reported. + # The semantic are the same as Gerrit comment range: + # https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#comment-range + # + # To specify the entire line, make start_line == end_line and + # start_col == end_col == 0. + start_line: int = 0 # inclusive 1-based + start_col: int = 0 # inclusive 0-based + end_line: int = 0 # exclusive 1-based + end_col: int = 0 # exclusive 0-based + + def validate(self): + if not self.file_path: + raise ValueError('file path is required') + if self.file_path != self.COMMIT_MSG_PATH and os.path.isabs( + self.file_path): + raise ValueError( + f'file path must be relative path, got {self.file_path}') + if not self.start_line: + if self.end_line: + raise ValueError('end_line must be empty if start line is not ' + 'specified') + if self.start_col: + raise ValueError('start_col must be empty if start line is not ' + 'specified') + if self.end_col: + raise ValueError('end_col must be empty if start line is not ' + 'specified') + elif self.start_line < 0: + raise ValueError('start_line MUST not be negative, ' + f'got {self.start_line}') + elif self.end_line < 1: + raise ValueError('start_line is specified so end_line must be ' + f'positive, got {self.end_line}') + elif self.start_col < 0: + raise ValueError('start_col MUST not be negative, ' + f'got {self.start_col}') + elif self.end_col < 0: + raise ValueError('end_col MUST not be negative, ' + f'got {self.end_col}') + elif self.start_line > self.end_line or ( + self.start_line == self.end_line + and self.start_col > self.end_col and self.end_col > 0): + raise ValueError( + '(start_line, start_col) must not be after (end_line, end_col' + f'), got ({self.start_line}, {self.start_col}) .. ' + f'({self.end_line}, {self.end_col})') + + # Top level object so multiprocessing can pickle # Public access through OutputApi object. class _PresubmitResult(object): @@ -318,15 +380,28 @@ class _PresubmitResult(object): fatal = False should_prompt = False - def __init__(self, message, items=None, long_text='', show_callstack=None): - """ - message: A short one-line message to indicate errors. - items: A list of short strings to indicate where errors occurred. - long_text: multi-line text output, e.g. from another tool + def __init__(self, + message: str, + items: list[str] = None, + long_text: str = '', + locations: list[_PresubmitResultLocation] = None, + show_callstack: bool = None): + """Inits _PresubmitResult. + + Args: + message: A short one-line message to indicate errors. + items: A list of short strings to indicate where errors occurred. + Note that if you are using this parameter to print where errors + occurred, please use `locations` instead + long_text: multi-line text output, e.g. from another tool + locations: The locations indicate where the errors occurred. """ self._message = _PresubmitResult._ensure_str(message) self._items = items or [] self._long_text = _PresubmitResult._ensure_str(long_text.rstrip()) + self._locations = locations or [] + for loc in self._locations: + loc.validate() if show_callstack is None: show_callstack = _SHOW_CALLSTACKS if show_callstack: @@ -346,24 +421,45 @@ class _PresubmitResult(object): return val.decode() raise ValueError("Unknown string type %s" % type(val)) - def handle(self): - sys.stdout.write(self._message) - sys.stdout.write('\n') + def handle(self, out_file=None): + if not out_file: + out_file = sys.stdout + out_file.write(self._message) + out_file.write('\n') for item in self._items: - sys.stdout.write(' ') + out_file.write(' ') # Write separately in case it's unicode. - sys.stdout.write(str(item)) - sys.stdout.write('\n') + out_file.write(str(item)) + out_file.write('\n') + if self._locations: + out_file.write('Found in:\n') + for loc in self._locations: + if loc.file_path == _PresubmitResultLocation.COMMIT_MSG_PATH: + out_file.write(' - Commit Message') + else: + out_file.write(f' - {loc.file_path}') + if not loc.start_line: + pass + elif loc.start_line == loc.end_line and (loc.start_col == 0 + and loc.end_col == 0): + out_file.write(f' [Ln {loc.start_line}]') + elif loc.start_col == 0 and loc.end_col == 0: + out_file.write(f' [Ln {loc.start_line} - {loc.end_line}]') + else: + out_file.write(f' [Ln {loc.start_line}, Col {loc.start_col}' + f' - Ln {loc.end_line}, Col {loc.end_col}]') + out_file.write('\n') if self._long_text: - sys.stdout.write('\n***************\n') + out_file.write('\n***************\n') # Write separately in case it's unicode. - sys.stdout.write(self._long_text) - sys.stdout.write('\n***************\n') + out_file.write(self._long_text) + out_file.write('\n***************\n') def json_format(self): return { 'message': self._message, 'items': [str(item) for item in self._items], + 'locations': [asdict(loc) for loc in self._locations], 'long_text': self._long_text, 'fatal': self.fatal } @@ -515,6 +611,7 @@ class OutputApi(object): PresubmitPromptWarning = _PresubmitPromptWarning PresubmitNotifyResult = _PresubmitNotifyResult MailTextResult = _MailTextResult + PresubmitResultLocation = _PresubmitResultLocation def __init__(self, is_committing): self.is_committing = is_committing diff --git a/tests/presubmit_support_test.py b/tests/presubmit_support_test.py index 62a14827ca..259df28c64 100755 --- a/tests/presubmit_support_test.py +++ b/tests/presubmit_support_test.py @@ -3,6 +3,7 @@ # 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 sys import unittest @@ -324,5 +325,179 @@ diff --git a/ffoo b/foo self.assertEqual(res, [('M', 'file')]) +class PresubmitResultLocationTest(unittest.TestCase): + + def test_invalid_missing_filepath(self): + with self.assertRaisesRegex(ValueError, 'file path is required'): + presubmit_support._PresubmitResultLocation(file_path='').validate() + + def test_invalid_abs_filepath_except_for_commit_msg(self): + loc = presubmit_support._PresubmitResultLocation(file_path='/foo') + with self.assertRaisesRegex(ValueError, + 'file path must be relative path'): + loc.validate() + loc = presubmit_support._PresubmitResultLocation( + file_path='/COMMIT_MSG') + try: + loc.validate() + except ValueError: + self.fail("validate should not fail for /COMMIT_MSG path") + + def test_invalid_end_line_without_start_line(self): + loc = presubmit_support._PresubmitResultLocation(file_path='foo', + end_line=5) + with self.assertRaisesRegex(ValueError, 'end_line must be empty'): + loc.validate() + + def test_invalid_start_col_without_start_line(self): + loc = presubmit_support._PresubmitResultLocation(file_path='foo', + start_col=5) + with self.assertRaisesRegex(ValueError, 'start_col must be empty'): + loc.validate() + + def test_invalid_end_col_without_start_line(self): + loc = presubmit_support._PresubmitResultLocation(file_path='foo', + end_col=5) + with self.assertRaisesRegex(ValueError, 'end_col must be empty'): + loc.validate() + + def test_invalid_negative_start_line(self): + loc = presubmit_support._PresubmitResultLocation(file_path='foo', + start_line=-1) + with self.assertRaisesRegex(ValueError, + 'start_line MUST not be negative'): + loc.validate() + + def test_invalid_non_positive_end_line(self): + loc = presubmit_support._PresubmitResultLocation(file_path='foo', + start_line=1, + end_line=0) + with self.assertRaisesRegex(ValueError, 'end_line must be positive'): + loc.validate() + + def test_invalid_negative_start_col(self): + loc = presubmit_support._PresubmitResultLocation(file_path='foo', + start_line=1, + end_line=1, + start_col=-1) + with self.assertRaisesRegex(ValueError, + 'start_col MUST not be negative'): + loc.validate() + + def test_invalid_negative_end_col(self): + loc = presubmit_support._PresubmitResultLocation(file_path='foo', + start_line=1, + end_line=1, + end_col=-1) + with self.assertRaisesRegex(ValueError, 'end_col MUST not be negative'): + loc.validate() + + def test_invalid_start_after_end_line(self): + loc = presubmit_support._PresubmitResultLocation(file_path='foo', + start_line=6, + end_line=5) + with self.assertRaisesRegex(ValueError, 'must not be after'): + loc.validate() + + def test_invalid_start_after_end_col(self): + loc = presubmit_support._PresubmitResultLocation(file_path='foo', + start_line=5, + start_col=11, + end_line=5, + end_col=10) + with self.assertRaisesRegex(ValueError, 'must not be after'): + loc.validate() + + +class PresubmitResultTest(unittest.TestCase): + + def test_handle_message_only(self): + result = presubmit_support._PresubmitResult('Simple message') + out = io.StringIO() + result.handle(out) + self.assertEqual(out.getvalue(), 'Simple message\n') + + def test_handle_full_args(self): + result = presubmit_support._PresubmitResult( + 'This is a message', + items=['item1', 'item2'], + long_text='Long text here.', + locations=[ + presubmit_support._PresubmitResultLocation( + file_path=presubmit_support._PresubmitResultLocation. + COMMIT_MSG_PATH), + presubmit_support._PresubmitResultLocation( + file_path='file1', + start_line=10, + end_line=10, + ), + presubmit_support._PresubmitResultLocation( + file_path='file2', + start_line=11, + end_line=15, + ), + presubmit_support._PresubmitResultLocation( + file_path='file3', + start_line=5, + start_col=0, + end_line=8, + end_col=5, + ) + ]) + out = io.StringIO() + result.handle(out) + expected = ('This is a message\n' + ' item1\n' + ' item2\n' + 'Found in:\n' + ' - Commit Message\n' + ' - file1 [Ln 10]\n' + ' - file2 [Ln 11 - 15]\n' + ' - file3 [Ln 5, Col 0 - Ln 8, Col 5]\n' + '\n***************\n' + 'Long text here.\n' + '***************\n') + self.assertEqual(out.getvalue(), expected) + + def test_json_format(self): + loc1 = presubmit_support._PresubmitResultLocation(file_path='file1', + start_line=1, + end_line=1) + loc2 = presubmit_support._PresubmitResultLocation(file_path='file2', + start_line=5, + start_col=2, + end_line=6, + end_col=10) + result = presubmit_support._PresubmitResult('This is a message', + items=['item1', 'item2'], + long_text='Long text here.', + locations=[loc1, loc2]) + expected = { + 'message': + 'This is a message', + 'items': ['item1', 'item2'], + 'locations': [ + { + 'file_path': 'file1', + 'start_line': 1, + 'start_col': 0, + 'end_line': 1, + 'end_col': 0 + }, + { + 'file_path': 'file2', + 'start_line': 5, + 'start_col': 2, + 'end_line': 6, + 'end_col': 10 + }, + ], + 'long_text': + 'Long text here.', + 'fatal': + False, + } + self.assertEqual(result.json_format(), expected) + if __name__ == "__main__": unittest.main() diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 4fdd519735..dd1ad62adc 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -694,30 +694,33 @@ class PresubmitUnittest(PresubmitTestsBase): fake_error = 'Missing LGTM' fake_error_items = '["!", "!!", "!!!"]' fake_error_long_text = "Error long text..." + fake_error_locations = '[output_api.PresubmitResultLocation(file_path="path/to/file")]' fake_error2 = 'This failed was found in file fake.py' fake_error2_items = '["!!!", "!!", "!"]' fake_error2_long_text = " Error long text" * 3 fake_warning = 'Line 88 is more than 80 characters.' fake_warning_items = '["W", "w"]' fake_warning_long_text = 'Warning long text...' + fake_warning_locations = ( + '[' + 'output_api.PresubmitResultLocation(file_path="path/to/foo", start_line=1, end_line=1), ' + 'output_api.PresubmitResultLocation(file_path="path/to/bar", start_line=4, start_col=5, end_line=6, end_col=7)' + ']') fake_notify = 'This is a dry run' fake_notify_items = '["N"]' fake_notify_long_text = 'Notification long text...' - always_fail_presubmit_script = ("""\n + always_fail_presubmit_script = (f"""\n def CheckChangeOnUpload(input_api, output_api): output_api.more_cc = ['me@example.com'] return [ - output_api.PresubmitError("%s",%s, "%s"), - output_api.PresubmitError("%s",%s, "%s"), - output_api.PresubmitPromptWarning("%s",%s, "%s"), - output_api.PresubmitNotifyResult("%s",%s, "%s") + output_api.PresubmitError("{fake_error}", {fake_error_items}, "{fake_error_long_text}", {fake_error_locations}), + output_api.PresubmitError("{fake_error2}", {fake_error2_items}, "{fake_error2_long_text}"), + output_api.PresubmitPromptWarning("{fake_warning}", {fake_warning_items}, "{fake_warning_long_text}", {fake_warning_locations}), + output_api.PresubmitNotifyResult("{fake_notify}", {fake_notify_items}, "{fake_notify_long_text}") ] def CheckChangeOnCommit(input_api, output_api): raise Exception("Test error") -""" % (fake_error, fake_error_items, fake_error_long_text, fake_error2, - fake_error2_items, fake_error2_long_text, fake_warning, - fake_warning_items, fake_warning_long_text, fake_notify, - fake_notify_items, fake_notify_long_text)) +""") os.path.isfile.return_value = False os.listdir.side_effect = [[], ['PRESUBMIT.py']] @@ -732,24 +735,54 @@ def CheckChangeOnCommit(input_api, output_api): 'message': fake_notify, 'items': json.loads(fake_notify_items), 'fatal': False, - 'long_text': fake_notify_long_text + 'long_text': fake_notify_long_text, + 'locations': [], }], 'errors': [{ - 'message': fake_error, - 'items': json.loads(fake_error_items), - 'fatal': True, - 'long_text': fake_error_long_text + 'message': + fake_error, + 'items': + json.loads(fake_error_items), + 'fatal': + True, + 'long_text': + fake_error_long_text, + 'locations': [{ + 'file_path': 'path/to/file', + 'start_line': 0, + 'start_col': 0, + 'end_line': 0, + 'end_col': 0, + }], }, { 'message': fake_error2, 'items': json.loads(fake_error2_items), 'fatal': True, - 'long_text': fake_error2_long_text + 'long_text': fake_error2_long_text, + 'locations': [], }], 'warnings': [{ - 'message': fake_warning, - 'items': json.loads(fake_warning_items), - 'fatal': False, - 'long_text': fake_warning_long_text + 'message': + fake_warning, + 'items': + json.loads(fake_warning_items), + 'fatal': + False, + 'long_text': + fake_warning_long_text, + 'locations': [{ + 'file_path': 'path/to/foo', + 'start_line': 1, + 'start_col': 0, + 'end_line': 1, + 'end_col': 0, + }, { + 'file_path': 'path/to/bar', + 'start_line': 4, + 'start_col': 5, + 'end_line': 6, + 'end_col': 7, + }], }], 'more_cc': ['me@example.com'], }