Add '--message' and '--bug' command line args.

These will automatically insert comments above the disabled test
indicating why it's disabled. --bug is a shorthand form which generates
a message with a TODO referencing the given bug.

Bug: 1281261
Change-Id: I97c3238aa748227f34155e6378c38bc2059c225e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3426750
Reviewed-by: Patrick Meiring <meiring@google.com>
Commit-Queue: Owen Rodley <orodley@chromium.org>
Auto-Submit: Owen Rodley <orodley@chromium.org>
Cr-Commit-Position: refs/heads/main@{#967095}

tools/disable_tests/disable.py

@@ -11,7 +11,7 @@
 import sys
 import subprocess
 import traceback
-from typing import List, Optional
+from typing import List, Optional, Tuple
 import urllib.parse
 
 import conditions
@@ -48,16 +48,40 @@ def main(argv: List[str]) -> int:
   parser.add_argument('-c',
                       '--cache',
                       action='store_true',
-                      help='cache ResultDB rpc results, useful for testing')
+                      help='cache ResultDB rpc results, useful for testing.')
+
+  group = parser.add_mutually_exclusive_group()
+  group.add_argument('-b',
+                     '--bug',
+                     help="write a TODO referencing this bug in a comment " +
+                     "next to the disabled test. Bug can be given as just the" +
+                     " ID or a URL (e.g. 123456, crbug.com/v8/654321).")
+  group.add_argument('-m',
+                     '--message',
+                     help="write a comment containing this message next to " +
+                     "the disabled test.")
 
   args = parser.parse_args(argv[1:])
 
   if args.cache:
     resultdb.CANNED_RESPONSE_FILE = os.path.join(os.path.dirname(__file__),
                                                  '.canned_responses.json')
 
+  message = args.message
+  if args.bug is not None:
+    try:
+      message = make_bug_message(args.bug)
+    except Exception:
+      print(
+          'Invalid value for --bug. Should have one of the following forms:\n' +
+          '\t1234\n' + '\tcrbug/1234\n' + '\tcrbug/project/1234\n' +
+          '\tcrbug.com/1234\n' + '\tcrbug.com/project/1234\n' +
+          '\tbugs.chromium.org/p/project/issues/detail?id=1234\n',
+          file=sys.stderr)
+      return 1
+
   try:
-    disable_test(args.test_id, args.conditions)
+    disable_test(args.test_id, args.conditions, message)
     return 0
   except errors.UserError as e:
     print(e, file=sys.stderr)
@@ -76,14 +100,60 @@ def main(argv: List[str]) -> int:
     return 1
 
 
+def make_bug_message(bug: str) -> str:
+  bug_id, project = parse_bug(bug)
+  project_component = '' if project == 'chromium' else f'{project}/'
+  bug_url = f"crbug.com/{project_component}{bug_id}"
+  return f"TODO({bug_url}): Re-enable this test"
+
+
+def parse_bug(bug: str) -> Tuple[int, str]:
+  # bug can be in a few different forms:
+  # * Just the ID, e.g. "1281261"
+  # * Monorail URL, e.g.
+  #     "https://bugs.chromium.org/p/chromium/issues/detail?id=1281261"
+  # * Monorail short URL, e.g.
+  #     "https://crbug.com/1281261"
+  #     or "crbug/1281261"
+  try:
+    bug_id = int(bug)
+    # Assume chromium host if only the ID is specified
+    return bug_id, 'chromium'
+  except ValueError:
+    pass
+
+  # Otherwise it should be a URL.
+  # Slight hack to ensure the domain is always in 'netloc'
+  if '//' not in bug:
+    bug = f"https://{bug}"
+  url = urllib.parse.urlparse(bug)
+
+  # Match crbug.com/ and crbug/
+  if url.netloc in {'crbug', 'crbug.com'}:
+    parts = url.path.split('/')[1:]
+    if len(parts) == 1:
+      return int(parts[0]), 'chromium'
+
+    return int(parts[1]), parts[0]
+
+  # Match full Monorail URLs.
+  if url.netloc == 'bugs.chromium.org':
+    parts = url.path.split('/')[1:]
+    project = parts[1]
+
+    bug_id = int(urllib.parse.parse_qs(url.query)['id'][0])
+    return bug_id, project
+
+  raise ValueError()
+
+
 # TODO: Extra command line flags for:
 #   * Opening the right file at the right line, for when you want to do
 #     something manually. Use $EDITOR.
-#   * Adding a comment / message accompanying the disablement.
 #   * Printing out all valid configs.
 #   * Overwrite the existing state rather than adding to it. Probably leave this
 #     until it's requested.
-def disable_test(test_id: str, cond_strs: List[str]):
+def disable_test(test_id: str, cond_strs: List[str], message: Optional[str]):
   conds = conditions.parse(cond_strs)
 
   #  If the given ID starts with "ninja:", then it's a full test ID. If not,
@@ -120,7 +190,7 @@ def disable_test(test_id: str, cond_strs: List[str]):
     raise errors.UserError(
         f"Don't know how to disable tests for this file format ({extension})")
 
-  new_content = disabler(test_name, source_file, conds)
+  new_content = disabler(test_name, source_file, conds, message)
   with open(full_path, 'w') as f:
     f.write(new_content)
 

tools/disable_tests/disable_test.py

@@ -0,0 +1,43 @@
+# Copyright 2022 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+"""Tests for disable.py"""
+
+import unittest
+import disable
+
+
+class DisableTest(unittest.TestCase):
+  def test_parse_bug(self):
+    self.assertEqual(disable.parse_bug('1234'), (1234, 'chromium'))
+    self.assertEqual(disable.parse_bug('crbug/9871'), (9871, 'chromium'))
+    self.assertEqual(disable.parse_bug('https://crbug/9871'),
+                     (9871, 'chromium'))
+    self.assertEqual(disable.parse_bug('crbug/v8/111111'), (111111, 'v8'))
+    self.assertEqual(disable.parse_bug('https://crbug/v8/111111'),
+                     (111111, 'v8'))
+    self.assertEqual(disable.parse_bug('crbug.com/8'), (8, 'chromium'))
+    self.assertEqual(disable.parse_bug('https://crbug.com/8'), (8, 'chromium'))
+    self.assertEqual(disable.parse_bug('crbug.com/monorail/19782757'),
+                     (19782757, 'monorail'))
+    self.assertEqual(disable.parse_bug('https://crbug.com/monorail/19782757'),
+                     (19782757, 'monorail'))
+    self.assertEqual(
+        disable.parse_bug('bugs.chromium.org/p/foo/issues/detail?id=6060842'),
+        (6060842, 'foo'))
+    self.assertEqual(
+        disable.parse_bug(
+            'http://bugs.chromium.org/p/foo/issues/detail?id=6060842'),
+        (6060842, 'foo'))
+    self.assertEqual(
+        disable.parse_bug(
+            'https://bugs.chromium.org/p/foo/issues/detail?id=6060842'),
+        (6060842, 'foo'))
+    self.assertEqual(
+        disable.parse_bug('https://bugs.chromium.org/p/foo/issues/detail' +
+                          '?id=191972&q=owner%3Ame%20link&can=2'),
+        (191972, 'foo'))
+
+
+if __name__ == '__main__':
+  unittest.main()

tools/disable_tests/expectations.py

@@ -46,7 +46,12 @@ def search_for_expectations(filename: str, test_name: str) -> str:
                              f"(expected to find it at {expectations_path})")
 
 
-def disabler(full_test_name: str, source_file: str, new_cond: Condition) -> str:
+def disabler(full_test_name: str, source_file: str, new_cond: Condition,
+             message: Optional[str]) -> str:
+  comment = None
+  if message:
+    comment = f"# {message}"
+
   existing_expectation: Optional[Expectation] = None
   condition = conditions.NEVER
   for expectation in TaggedTestListParser(source_file).expectations:
@@ -68,6 +73,10 @@ def disabler(full_test_name: str, source_file: str, new_cond: Condition) -> str:
 
     while not source_file.endswith('\n\n'):
       source_file += '\n'
+
+    if comment:
+      source_file += f"{comment}\n"
+
     source_file += ex.to_string()
     return source_file
 
@@ -82,6 +91,10 @@ def disabler(full_test_name: str, source_file: str, new_cond: Condition) -> str:
   lines = source_file.split('\n')
   # Minus 1 as 'lineno' is 1-based.
   lines[existing_expectation.lineno - 1] = new_expectation.to_string()
+
+  if comment:
+    lines.insert(existing_expectation.lineno - 1, comment)
+
   return '\n'.join(lines)
 
 

tools/disable_tests/gtest.py

@@ -27,7 +27,8 @@
 }
 
 
-def disabler(full_test_name: str, source_file: str, new_cond: Condition) -> str:
+def disabler(full_test_name: str, source_file: str, new_cond: Condition,
+             message: Optional[str]) -> str:
   """Disable a GTest test within the given file.
 
   Args:
@@ -82,32 +83,14 @@ def disabler(full_test_name: str, source_file: str, new_cond: Condition) -> str:
 
   merged = conditions.merge(existing_cond, new_cond)
 
-  # If it's not conditionally disabled, we don't need a pre-processor block to
-  # conditionally define the name. We just change the name within the test macro
-  # to its appropriate value, and delete any existing preprocessor block.
-  if isinstance(merged, conditions.BaseCondition):
-    if merged == conditions.ALWAYS:
-      replacement_name = disabled
-    elif merged == conditions.NEVER:
-      replacement_name = test_name
-
-    lines[test_name_index] = \
-        lines[test_name_index].replace(current_name, replacement_name)
-    modified_line = test_name_index
-
-    if src_range:
-      del lines[src_range[0]:src_range[1] + 1]
-      modified_line -= src_range[1] - src_range[0] + 1
-
-    return clang_format('\n'.join(lines), [modified_line])
-
-  # => now conditionally disabled
-  lines[test_name_index] = lines[test_name_index].replace(current_name, maybe)
+  comment = None
+  if message:
+    comment = f'// {message}'
 
   # Keep track of the line numbers of the lines which have been modified. These
   # line numbers will be fed to clang-format to ensure any modified lines are
   # correctly formatted.
-  modified_lines = [test_name_index]
+  modified_lines = []
 
   # Ensure that we update modified_lines upon inserting new lines into the file,
   # as any lines after the insertion point will be shifted over.
@@ -137,6 +120,36 @@ def insert_lines(start_index, end_index, new_lines):
   def insert_line(index, new_line):
     insert_lines(index, index, [new_line])
 
+  def replace_line(index, new_line):
+    insert_lines(index, index + 1, [new_line])
+
+  def delete_lines(start_index, end_index):
+    insert_lines(start_index, end_index, [])
+
+  # If it's not conditionally disabled, we don't need a pre-processor block to
+  # conditionally define the name. We just change the name within the test macro
+  # to its appropriate value, and delete any existing preprocessor block.
+  if isinstance(merged, conditions.BaseCondition):
+    if merged == conditions.ALWAYS:
+      replacement_name = disabled
+    elif merged == conditions.NEVER:
+      replacement_name = test_name
+
+    replace_line(test_name_index,
+                 lines[test_name_index].replace(current_name, replacement_name))
+
+    if src_range:
+      delete_lines(src_range[0], src_range[1] + 1)
+
+    if comment:
+      insert_line(test_name_index, comment)
+
+    return clang_format('\n'.join(lines), modified_lines)
+
+  # => now conditionally disabled
+  replace_line(test_name_index,
+               lines[test_name_index].replace(current_name, maybe))
+
   condition_impl = cc_format_condition(merged)
 
   condition_block = [
@@ -150,6 +163,7 @@ def insert_line(index, new_line):
   if src_range:
     # Replace the existing condition.
     insert_lines(src_range[0], src_range[1] + 1, condition_block)
+    comment_index = src_range[0]
   else:
     # No existing condition, so find where to add a new one.
     for i in range(test_name_index, -1, -1):
@@ -159,6 +173,10 @@ def insert_line(index, new_line):
       raise Exception("Couldn't find where to insert test conditions")
 
     insert_lines(i, i, condition_block)
+    comment_index = i
+
+  if comment:
+    insert_line(comment_index, comment)
 
   # Insert includes.
   # First find the set of headers we need for the given condition.

tools/disable_tests/gtest_test.py

@@ -4,15 +4,20 @@
 """Tests for gtest.py"""
 
 import unittest
+from typing import Optional
 
 import conditions
 from conditions import Condition
 import gtest
 
 
 class GtestTest(unittest.TestCase):
-  def disabler_test(self, input_file: str, test_name: str, new_cond,
-                    expected_result: str):
+  def disabler_test(self,
+                    input_file: str,
+                    test_name: str,
+                    new_cond,
+                    expected_result: str,
+                    message: Optional[str] = None):
     """Helper function for testing gtest.disabler."""
 
     self.maxDiff = None
@@ -22,7 +27,7 @@ def disabler_test(self, input_file: str, test_name: str, new_cond,
     else:
       assert isinstance(new_cond, conditions.BaseCondition)
 
-    resulting_file = gtest.disabler(test_name, input_file, new_cond)
+    resulting_file = gtest.disabler(test_name, input_file, new_cond, message)
 
     self.assertEqual(expected_result.strip(), resulting_file.strip())
 
@@ -192,6 +197,39 @@ def test_ignore_comments_in_preprocessor_directive(self):
 TEST(Suite, MAYBE_Test) {}
 ''', 'Suite.Test', conditions.NEVER, 'TEST(Suite, Test) {}')
 
+  def test_disable_unconditionally_with_message(self):
+    # Also include some formatting that should be fixed up, to test that the
+    # correct line number is passed to clang-format.
+    self.disabler_test('''
+// existing comment
+TEST(Suite,   Test) {}
+// another comment''',
+                       'Suite.Test',
+                       conditions.ALWAYS,
+                       '''
+// existing comment
+// here is the message
+TEST(Suite, DISABLED_Test) {}
+// another comment''',
+                       message='here is the message')
+
+  def test_conditionally_disable_test_with_message(self):
+    # Also include some formatting that should be fixed up, to test that the
+    # correct line number is passed to clang-format.
+    self.disabler_test('TEST(Suite,     Test) {}',
+                       'Suite.Test', ['linux', 'mac'],
+                       '''
+#include "build/build_config.h"
+// we should really fix this
+#if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_MAC)
+#define MAYBE_Test DISABLED_Test
+#else
+#define MAYBE_Test Test
+#endif
+TEST(Suite, MAYBE_Test) {}
+''',
+                       message='we should really fix this')
+
 
 if __name__ == '__main__':
   unittest.main()