Android: Add lint regression test

New API related checks are some of our most valuable lint checks, as
triggering an error causes a crash directly.

This CL adds compile test targets to ensure that code with API level
errors trigger lint warnings. This prevents lint being accidentally
turned off.

Also delete now obsolete //build/android/lint directory and remove some
already-fixed suppressions from lint-suppressions.xml.

Bug: 1108309
Change-Id: I87e68c85ae6670b1c8a1f0691600e021477c2ff4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2412808
Commit-Queue: Peter Wen <wnwen@chromium.org>
Reviewed-by: Mohamed Heikal <mheikal@chromium.org>
Auto-Submit: Peter Wen <wnwen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807625}

build/android/BUILD.gn

@@ -16,6 +16,27 @@ if (enable_java_templates) {
     create_cache = true
   }
 
+  android_library("lint_test_java") {
+    sources = [ "java/test/LintTest.java" ]
+  }
+
+  android_apk("lint_test_apk") {
+    # This cannot be marked testonly since lint has special ignores for testonly
+    # targets. We need to test linting a normal apk target.
+    apk_name = "LintTest"
+    deps = [ ":lint_test_java" ]
+    android_manifest = "//build/android/AndroidManifest.xml"
+  }
+
+  android_lint("lint_test") {
+    lint_expected_warnings = "InlinedApi,NewApi"
+    _test_apk_target = ":lint_test_apk"
+    deps = [ "${_test_apk_target}__java" ]
+    build_config_dep = "$_test_apk_target$build_config_target_suffix"
+    build_config = get_label_info(_test_apk_target, "target_gen_dir") + "/" +
+                   get_label_info(_test_apk_target, "name") + ".build_config"
+  }
+
   if (enable_jdk_library_desugaring) {
     dex_jdk_libs("all_jdk_libs") {
       output = "$target_out_dir/$target_name.l8.dex"

build/android/gyp/lint.py

@@ -8,6 +8,7 @@
 from __future__ import print_function
 
 import argparse
+import functools
 import logging
 import os
 import re
@@ -141,6 +142,17 @@ def _WriteXmlFile(root, path):
             root, encoding='utf-8')).toprettyxml(indent='  '))
 
 
+def _CheckLintWarning(expected_warnings, lint_output):
+  for expected_warning in expected_warnings.split(','):
+    expected_str = '[{}]'.format(expected_warning)
+    if expected_str not in lint_output:
+      raise Exception('Expected {!r} warning in lint output:\n{}.'.format(
+          expected_str, lint_output))
+
+  # Do not print warning
+  return ''
+
+
 def _RunLint(lint_binary_path,
              config_path,
              manifest_path,
@@ -155,6 +167,7 @@ def _RunLint(lint_binary_path,
              android_sdk_root,
              lint_gen_dir,
              baseline,
+             expected_warnings,
              testonly_target=False,
              warnings_as_errors=False):
   logging.info('Lint starting')
@@ -239,6 +252,8 @@ def _RunLint(lint_binary_path,
   # This filter is necessary for JDK11.
   stderr_filter = build_utils.FilterReflectiveAccessJavaWarnings
   stdout_filter = lambda x: build_utils.FilterLines(x, 'No issues found')
+  if expected_warnings:
+    stdout_filter = functools.partial(_CheckLintWarning, expected_warnings)
 
   start = time.time()
   logging.debug('Lint command %s', ' '.join(cmd))
@@ -325,6 +340,9 @@ def _ParseArgs(argv):
   parser.add_argument('--baseline',
                       help='Baseline file to ignore existing errors and fail '
                       'on new errors.')
+  parser.add_argument('--expected-warnings',
+                      help='Comma separated list of warnings to test for in '
+                      'the output, failing if not found.')
 
   args = parser.parse_args(build_utils.ExpandFileArgs(argv))
   args.java_sources = build_utils.ParseGnList(args.java_sources)
@@ -367,6 +385,7 @@ def main():
            args.android_sdk_root,
            args.lint_gen_dir,
            args.baseline,
+           args.expected_warnings,
            testonly_target=args.testonly,
            warnings_as_errors=args.warnings_as_errors)
   logging.info('Creating stamp file')

build/android/lint/suppress.py

-#!/usr/bin/env python
-#
-# Copyright (c) 2013 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.
-
-"""Add all generated lint_result.xml files to suppressions.xml"""
-
-# pylint: disable=no-member
-
-from __future__ import print_function
-
-import argparse
-import os
-import re
-import sys
-from xml.dom import minidom
-
-_BUILD_ANDROID_DIR = os.path.join(os.path.dirname(__file__), '..')
-sys.path.append(_BUILD_ANDROID_DIR)
-
-_TMP_DIR_RE = re.compile(r'^/tmp/.*/(SRC_ROOT[0-9]+|PRODUCT_DIR)/')
-_THIS_FILE = os.path.abspath(__file__)
-_DEFAULT_CONFIG_PATH = os.path.join(os.path.dirname(_THIS_FILE),
-                                    'suppressions.xml')
-_INSERT_COMMENT = ('TODO: This line was added by suppress.py,'
-                   ' please add an explanation.')
-
-
-class _Issue(object):
-
-  def __init__(self, dom_element):
-    self.regexps = set()
-    self.dom_element = dom_element
-
-
-def _CollectIssuesFromDom(dom):
-  issues_dict = {}
-  for issue_element in dom.getElementsByTagName('issue'):
-    issue_id = issue_element.attributes['id'].value
-    issue = _Issue(issue_element)
-    issues_dict[issue_id] = issue
-    for child in issue_element.childNodes:
-      if child.nodeType != minidom.Node.ELEMENT_NODE:
-        continue
-      if child.tagName == 'ignore' and child.getAttribute('regexp'):
-        issue.regexps.add(child.getAttribute('regexp'))
-
-  return issues_dict
-
-
-def _TrimWhitespaceNodes(n):
-  """Remove all whitespace-only TEXT_NODEs."""
-  rm_children = []
-  for c in n.childNodes:
-    if c.nodeType == minidom.Node.TEXT_NODE and c.data.strip() == '':
-      rm_children.append(c)
-    else:
-      _TrimWhitespaceNodes(c)
-
-  for c in rm_children:
-    n.removeChild(c)
-
-
-def _ParseAndInsertNewSuppressions(result_path, config_path):
-  print('Parsing %s' % config_path)
-  config_dom = minidom.parse(config_path)
-  issues_dict = _CollectIssuesFromDom(config_dom)
-  print('Parsing and merging %s' % result_path)
-  dom = minidom.parse(result_path)
-  for issue_element in dom.getElementsByTagName('issue'):
-    issue_id = issue_element.attributes['id'].value
-    severity = issue_element.attributes['severity'].value
-    path = issue_element.getElementsByTagName(
-        'location')[0].attributes['file'].value
-    # Strip temporary file path.
-    path = re.sub(_TMP_DIR_RE, '', path)
-    # Escape Java inner class name separator and suppress with regex instead
-    # of path. Doesn't use re.escape() as it is a bit too aggressive and
-    # escapes '_', causing trouble with PRODUCT_DIR.
-    regexp = path.replace('$', r'\$')
-    if issue_id not in issues_dict:
-      element = config_dom.createElement('issue')
-      element.attributes['id'] = issue_id
-      element.attributes['severity'] = severity
-      config_dom.documentElement.appendChild(element)
-      issue = _Issue(element)
-      issues_dict[issue_id] = issue
-    else:
-      issue = issues_dict[issue_id]
-      if issue.dom_element.getAttribute('severity') == 'ignore':
-        continue
-
-    if regexp not in issue.regexps:
-      issue.regexps.add(regexp)
-      ignore_element = config_dom.createElement('ignore')
-      ignore_element.attributes['regexp'] = regexp
-      issue.dom_element.appendChild(config_dom.createComment(_INSERT_COMMENT))
-      issue.dom_element.appendChild(ignore_element)
-
-    for issue_id, issue in issues_dict.iteritems():
-      if issue.dom_element.getAttribute('severity') == 'ignore':
-        print('Warning: [%s] is suppressed globally.' % issue_id)
-
-  # toprettyxml inserts whitespace, so delete whitespace first.
-  _TrimWhitespaceNodes(config_dom.documentElement)
-
-  with open(config_path, 'w') as f:
-    f.write(config_dom.toprettyxml(indent='  ', encoding='utf-8'))
-  print('Updated %s' % config_path)
-
-
-def _Suppress(config_path, result_path):
-  _ParseAndInsertNewSuppressions(result_path, config_path)
-
-
-def main():
-  parser = argparse.ArgumentParser(description=__doc__)
-  parser.add_argument('--config',
-                      help='Path to suppression.xml config file',
-                      default=_DEFAULT_CONFIG_PATH)
-  parser.add_argument('result_path',
-                      help='Lint results xml file',
-                      metavar='RESULT_FILE')
-  args = parser.parse_args()
-
-  _Suppress(args.config, args.result_path)
-
-
-if __name__ == '__main__':
-  main()

build/config/android/internal_rules.gni

@@ -988,11 +988,9 @@ if (enable_java_templates) {
         pool = "//build/toolchain:link_pool($default_toolchain)"
       }
 
-      # Lint only requires generated sources and generated resources from the
-      # build. Since turbine __header targets already depend on and generate all
-      # the generated sources, and the __assetres targets include all generated
-      # resources, lint only needs to depend on the header and assetres targets
-      # rather than the top level java targets.
+      # Lint requires generated sources and generated resources from the build.
+      # Turbine __header targets depend on all generated sources, and the
+      # __assetres targets depend on all generated resources.
       if (defined(invoker.deps)) {
         foreach(_dep, invoker.deps) {
           _target_label = get_label_info(_dep, "label_no_toolchain")
@@ -1000,16 +998,12 @@ if (enable_java_templates) {
               filter_exclude([ _target_label ], _java_resource_patterns) !=
               []) {
             deps += [
-              # Strictly speaking the __header target is sufficient, since they
-              # already depend on resources (due to srcjar deps), but prefer to
-              # be more explicit here since if/when __header targets stop
-              # depending on resources (e.g. if R.java generation moves to java
-              # targets), lint will not be affected.
               "${_target_label}__assetres",
               "${_target_label}__header",
             ]
           } else {
-            # Keep non-java deps as they may generate files used by lint.
+            # Keep non-java deps as they may generate files used only by lint.
+            # e.g. generated suppressions.xml files.
             deps += [ _dep ]
           }
         }
@@ -1111,6 +1105,13 @@ if (enable_java_templates) {
           # The full classpath is required for annotation checks like @IntDef.
           "--classpath=@FileArg($_rebased_build_config:deps_info:javac_full_interface_classpath)",
         ]
+
+        if (defined(invoker.lint_expected_warnings)) {
+          args += [
+            "--expected-warnings",
+            invoker.lint_expected_warnings,
+          ]
+        }
       }
 
       outputs = [ _stamp_path ]

chrome/android/expectations/lint-suppressions.xml

@@ -176,12 +176,8 @@ Still reading?
     <ignore regexp="Static interface  method requires API level 24"/>
     <!-- 1: TaskInfo is refactored at API 29. -->
     <ignore regexp="Field requires API level .*`android.app.TaskInfo"/>
-    <!-- 1: TODO(crbug.com/1082222): Fix -->
-    <ignore regexp="chrome/android/java/src/org/chromium/chrome/browser/omnibox/suggestions/header/HeaderView.java"/>
     <!-- 1: TODO(crbug.com/1085410): Fix -->
     <ignore regexp="components/content_capture/android/java/src/org/chromium/components/content_capture"/>
-    <!-- 1: TODO(wnwen): File bugs to fix -->
-    <ignore regexp="android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/util/BoundaryInterfaceReflectionUtil.java"/>
     <!-- Endnote: Please specify number of suppressions when adding more -->
   </issue>
   <!-- This warning just adds a lot of false positives. -->