presubmit: add location data to presubmit result

This will help us to display preusbmit result as findings both in
Gerrit and in Cider workspaces.

Change-Id: I0f5a9c503c1b14dc8dcbf9794cc556ef215a46a2
Bug: 404837554
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6469939
Commit-Queue: Yiwei Zhang <yiwzhang@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>

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

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()

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'],
         }