android_test_mapping_format: updated the code to match the internal version.android-s-beta-5android-s-beta-5

Made changes to match the internal cl: cl/394099398

+ Used the imperative language in the docstring.
+ Added typing for functions
+ Fixed pylint errors.
+ Added "_" to all globals.
+ Used f' strings
+ Small optimizations to the code to be more efficient.

Bug: 196099793
Test: unittests pass

Tested:
$ pylint tools/android_test_mapping_format.py
$ pylint tools/android_test_mapping_format_unittest.py
$ pytest tools/android_test_mapping_format_unittest.py

Change-Id: I9e8b4be2a6729762001f2e5cd266dccbb0b6f9e9

2 files changed, 137 insertions, 132 deletions

diff --git a/tools/android_test_mapping_format.py b/tools/android_test_mapping_format.py
index 1e654e6..ef1a9b5 100755
--- a/tools/android_test_mapping_format.py
+++ b/tools/android_test_mapping_format.py
@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""Validate TEST_MAPPING files in Android source code.
+"""Validates TEST_MAPPING files in Android source code.
 
 The goal of this script is to validate the format of TEST_MAPPING files:
 1. It must be a valid json file.
@@ -26,6 +26,7 @@ import argparse
 import json
 import os
 import re
+from typing import Any, Dict
 import sys
 
 _path = os.path.realpath(__file__ + '/../..')
@@ -38,14 +39,16 @@ del _path
 # pylint: disable=wrong-import-position
 import rh.git
 
-IMPORTS = 'imports'
-NAME = 'name'
-OPTIONS = 'options'
-PATH = 'path'
-HOST = 'host'
-PREFERRED_TARGETS = 'preferred_targets'
-FILE_PATTERNS = 'file_patterns'
-TEST_MAPPING_URL = (
+_IMPORTS = 'imports'
+_NAME = 'name'
+_OPTIONS = 'options'
+_PATH = 'path'
+_HOST = 'host'
+_PREFERRED_TARGETS = 'preferred_targets'
+_FILE_PATTERNS = 'file_patterns'
+_INVALID_IMPORT_CONFIG = 'Invalid import config in TEST_MAPPING file'
+_INVALID_TEST_CONFIG = 'Invalid test config in TEST_MAPPING file'
+_TEST_MAPPING_URL = (
     'https://source.android.com/compatibility/tests/development/'
     'test-mapping')
 
@@ -61,8 +64,8 @@ class InvalidTestMappingError(Error):
     """Exception to raise when detecting an invalid TEST_MAPPING file."""
 
 
-def filter_comments(json_data):
-    """Remove '//'-format comments in TEST_MAPPING file to valid format.
+def _filter_comments(json_data: str) -> str:
+    """Removes '//'-format comments in TEST_MAPPING file to valid format.
 
     Args:
         json_data: TEST_MAPPING file content (as a string).
@@ -70,12 +73,12 @@ def filter_comments(json_data):
     Returns:
         Valid json string without comments.
     """
-    return ''.join('\n' if _COMMENTS_RE.match(x) else x for x in
-                   json_data.splitlines())
+    return ''.join(
+        '\n' if _COMMENTS_RE.match(x) else x for x in json_data.splitlines())
 
 
-def _validate_import(entry, test_mapping_file):
-    """Validate an import setting.
+def _validate_import(entry: Dict[str, Any], test_mapping_file: str) -> None:
+    """Validates an import setting.
 
     Args:
         entry: A dictionary of an import setting.
@@ -86,85 +89,84 @@ def _validate_import(entry, test_mapping_file):
     """
     if len(entry) != 1:
         raise InvalidTestMappingError(
-            'Invalid import config in test mapping file %s. each import can '
-            'only have one `path` setting. Failed entry: %s' %
-            (test_mapping_file, entry))
-    if list(entry.keys())[0] != PATH:
+            f'{_INVALID_IMPORT_CONFIG} {test_mapping_file}. Each import can '
+            f'only have one `path` setting. Failed entry: {entry}')
+    if _PATH not in entry:
         raise InvalidTestMappingError(
-            'Invalid import config in test mapping file %s. import can only '
-            'have one `path` setting. Failed entry: %s' %
-            (test_mapping_file, entry))
+            f'{_INVALID_IMPORT_CONFIG} {test_mapping_file}. Import can '
+            f'only have one `path` setting. Failed entry: {entry}')
 
 
-def _validate_test(test, test_mapping_file):
-    """Validate a test declaration.
+def _validate_test(test: Dict[str, Any], test_mapping_file: str) -> bool:
+    """Returns whether a test declaration is valid.
 
     Args:
-        entry: A dictionary of a test declaration.
+        test: A dictionary of a test declaration.
         test_mapping_file: Path to the TEST_MAPPING file to be validated.
 
     Raises:
         InvalidTestMappingError: if the a test declaration is invalid.
     """
-    if NAME not in test:
+    if _NAME not in test:
         raise InvalidTestMappingError(
-            'Invalid test config in test mapping file %s. test config must '
-            'a `name` setting. Failed test config: %s' %
-            (test_mapping_file, test))
-    if not isinstance(test.get(HOST, False), bool):
-        raise InvalidTestMappingError(
-            'Invalid test config in test mapping file %s. `host` setting in '
-            'test config can only have boolean value of `true` or `false`. '
-            'Failed test config: %s' % (test_mapping_file, test))
-    preferred_targets = test.get(PREFERRED_TARGETS, [])
-    if (not isinstance(preferred_targets, list) or
-            any(not isinstance(t, str) for t in preferred_targets)):
-        raise InvalidTestMappingError(
-            'Invalid test config in test mapping file %s. `preferred_targets` '
-            'setting in test config can only be a list of strings. Failed test '
-            'config: %s' % (test_mapping_file, test))
-    file_patterns = test.get(FILE_PATTERNS, [])
-    if (not isinstance(file_patterns, list) or
-            any(not isinstance(p, str) for p in file_patterns)):
+
+            f'{_INVALID_TEST_CONFIG} {test_mapping_file}. Test config must '
+            f'have a `name` setting. Failed test config: {test}')
+
+    if not isinstance(test.get(_HOST, False), bool):
         raise InvalidTestMappingError(
-            'Invalid test config in test mapping file %s. `file_patterns` '
-            'setting in test config can only be a list of strings. Failed test '
-            'config: %s' % (test_mapping_file, test))
-    for option in test.get(OPTIONS, []):
+            f'{_INVALID_TEST_CONFIG} {test_mapping_file}. `host` setting in '
+            f'test config can only have boolean value of `true` or `false`. '
+            f'Failed test config: {test}')
+
+    for key in (_PREFERRED_TARGETS, _FILE_PATTERNS):
+        value = test.get(key, [])
+        if (not isinstance(value, list) or
+            any(not isinstance(t, str) for t in value)):
+            raise InvalidTestMappingError(
+                f'{_INVALID_TEST_CONFIG} {test_mapping_file}. `{key}` setting '
+                f'in test config can only be a list of strings. '
+                f'Failed test config: {test}')
+
+    for option in test.get(_OPTIONS, []):
+        if not isinstance(option, dict):
+            raise InvalidTestMappingError(
+                f'{_INVALID_TEST_CONFIG} {test_mapping_file}. Option setting '
+                f'in test config can only be a dictionary of key-val setting. '
+                f'Failed entry: {option}')
         if len(option) != 1:
             raise InvalidTestMappingError(
-                'Invalid option setting in test mapping file %s. each option '
-                'setting can only have one key-val setting. Failed entry: %s' %
-                (test_mapping_file, option))
+                f'{_INVALID_TEST_CONFIG} {test_mapping_file}. Each option '
+                f'setting can only have one key-val setting. '
+                f'Failed entry: {option}')
 
 
-def _load_file(test_mapping_file):
-    """Load a TEST_MAPPING file as a json file."""
+def process_file(test_mapping_file: str) -> None:
+    """Validates a TEST_MAPPING file content."""
     try:
-        return json.loads(filter_comments(test_mapping_file))
-    except ValueError as e:
+        test_mapping_data = json.loads(_filter_comments(test_mapping_file))
+    except ValueError as exception:
         # The file is not a valid JSON file.
         print(
-            'Failed to parse JSON file %s, error: %s' % (test_mapping_file, e),
+            f'Invalid JSON data in TEST_MAPPING file '
+            f'Failed to parse JSON data: {test_mapping_file}, '
+            f'error: {exception}',
             file=sys.stderr)
         raise
 
-
-def process_file(test_mapping_file):
-    """Validate a TEST_MAPPING file."""
-    test_mapping = _load_file(test_mapping_file)
-    # Validate imports.
-    for import_entry in test_mapping.get(IMPORTS, []):
-        _validate_import(import_entry, test_mapping_file)
-    # Validate tests.
-    all_tests = [test for group, tests in test_mapping.items()
-                 if group != IMPORTS for test in tests]
-    for test in all_tests:
-        _validate_test(test, test_mapping_file)
+    for group, value in test_mapping_data.items():
+        if group == _IMPORTS:
+            # Validate imports.
+            for test in value:
+                _validate_import(test, test_mapping_file)
+        else:
+            # Validate tests.
+            for test in value:
+                _validate_test(test, test_mapping_file)
 
 
 def get_parser():
-    """Return a command line parser."""
+    """Returns a command line parser."""
     parser = argparse.ArgumentParser(description=__doc__)
     parser.add_argument('--commit', type=str,
                         help='Specify the commit to validate.')
@@ -174,6 +176,7 @@ def get_parser():
 
 
 def main(argv):
+    """Main function."""
     parser = get_parser()
     opts = parser.parse_args(argv)
     try:
@@ -181,12 +184,12 @@ def main(argv):
             if opts.commit:
                 json_data = rh.git.get_file_content(opts.commit, filename)
             else:
-                with open(os.path.join(opts.project_dir, filename)) as f:
-                    json_data = f.read()
+                with open(os.path.join(opts.project_dir, filename)) as file:
+                    json_data = file.read()
             process_file(json_data)
     except:
         print('Visit %s for details about the format of TEST_MAPPING '
-              'file.' % TEST_MAPPING_URL, file=sys.stderr)
+              'file.' % _TEST_MAPPING_URL, file=sys.stderr)
         raise
 
 
diff --git a/tools/android_test_mapping_format_unittest.py b/tools/android_test_mapping_format_unittest.py
index 9bef300..14bae32 100755
--- a/tools/android_test_mapping_format_unittest.py
+++ b/tools/android_test_mapping_format_unittest.py
@@ -13,6 +13,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""Unittests for android_test_mapping_format."""
+
 import os
 import shutil
 import tempfile
@@ -21,7 +23,7 @@ import unittest
 import android_test_mapping_format
 
 
-VALID_TEST_MAPPING = r"""
+_VALID_TEST_MAPPING = r"""
 {
   "presubmit": [
     {
@@ -52,11 +54,11 @@ VALID_TEST_MAPPING = r"""
 }
 """
 
-BAD_JSON = """
+_BAD_JSON = """
 {wrong format}
 """
 
-BAD_TEST_WRONG_KEY = """
+_BAD_TEST_WRONG_KEY = """
 {
   "presubmit": [
     {
@@ -66,7 +68,7 @@ BAD_TEST_WRONG_KEY = """
 }
 """
 
-BAD_TEST_WRONG_HOST_VALUE = """
+_BAD_TEST_WRONG_HOST_VALUE = """
 {
   "presubmit": [
     {
@@ -78,7 +80,7 @@ BAD_TEST_WRONG_HOST_VALUE = """
 """
 
 
-BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_NONE_LIST = """
+_BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_NONE_LIST = """
 {
   "presubmit": [
     {
@@ -89,7 +91,7 @@ BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_NONE_LIST = """
 }
 """
 
-BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_WRONG_TYPE = """
+_BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_WRONG_TYPE = """
 {
   "presubmit": [
     {
@@ -100,7 +102,7 @@ BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_WRONG_TYPE = """
 }
 """
 
-BAD_TEST_WRONG_OPTION = """
+_BAD_TEST_WRONG_OPTION = """
 {
   "presubmit": [
     {
@@ -116,7 +118,7 @@ BAD_TEST_WRONG_OPTION = """
 }
 """
 
-BAD_IMPORT_WRONG_KEY = """
+_BAD_IMPORT_WRONG_KEY = """
 {
   "imports": [
     {
@@ -126,7 +128,7 @@ BAD_IMPORT_WRONG_KEY = """
 }
 """
 
-BAD_IMPORT_WRONG_IMPORT_VALUE = """
+_BAD_IMPORT_WRONG_IMPORT_VALUE = """
 {
   "imports": [
     {
@@ -137,7 +139,7 @@ BAD_IMPORT_WRONG_IMPORT_VALUE = """
 }
 """
 
-BAD_FILE_PATTERNS = """
+_BAD_FILE_PATTERNS = """
 {
   "presubmit": [
     {
@@ -148,7 +150,7 @@ BAD_FILE_PATTERNS = """
 }
 """
 
-TEST_MAPPING_WITH_SUPPORTED_COMMENTS = r"""
+_TEST_MAPPING_WITH_SUPPORTED_COMMENTS = r"""
 // supported comment
 {
   // supported comment!@#$%^&*()_
@@ -171,7 +173,7 @@ TEST_MAPPING_WITH_SUPPORTED_COMMENTS = r"""
 }
 """
 
-TEST_MAPPING_WITH_NON_SUPPORTED_COMMENTS = """
+_TEST_MAPPING_WITH_NON_SUPPORTED_COMMENTS = """
 { #non-supported comments
   // supported comments
   "presubmit": [#non-supported comments
@@ -196,112 +198,112 @@ class AndroidTestMappingFormatTests(unittest.TestCase):
     def test_valid_test_mapping(self):
         """Verify that the check doesn't raise any error for valid test mapping.
         """
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(VALID_TEST_MAPPING)
-        with open(self.test_mapping_file, 'r') as f:
-            android_test_mapping_format.process_file(f.read())
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_VALID_TEST_MAPPING)
+        with open(self.test_mapping_file, 'r') as file:
+            android_test_mapping_format.process_file(file.read())
 
     def test_invalid_test_mapping_bad_json(self):
         """Verify that TEST_MAPPING file with bad json can be detected."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(BAD_JSON)
-        with open(self.test_mapping_file, 'r') as f:
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_BAD_JSON)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 ValueError, android_test_mapping_format.process_file,
-                f.read())
+                file.read())
 
     def test_invalid_test_mapping_wrong_test_key(self):
         """Verify that test config using wrong key can be detected."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(BAD_TEST_WRONG_KEY)
-        with open(self.test_mapping_file, 'r') as f:
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_BAD_TEST_WRONG_KEY)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 android_test_mapping_format.InvalidTestMappingError,
                 android_test_mapping_format.process_file,
-                f.read())
+                file.read())
 
     def test_invalid_test_mapping_wrong_test_value(self):
         """Verify that test config using wrong host value can be detected."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(BAD_TEST_WRONG_HOST_VALUE)
-        with open(self.test_mapping_file, 'r') as f:
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_BAD_TEST_WRONG_HOST_VALUE)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 android_test_mapping_format.InvalidTestMappingError,
                 android_test_mapping_format.process_file,
-                f.read())
+                file.read())
 
     def test_invalid_test_mapping_wrong_preferred_targets_value(self):
         """Verify invalid preferred_targets are rejected."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_NONE_LIST)
-        with open(self.test_mapping_file, 'r') as f:
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_NONE_LIST)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 android_test_mapping_format.InvalidTestMappingError,
                 android_test_mapping_format.process_file,
-                f.read())
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_WRONG_TYPE)
-        with open(self.test_mapping_file, 'r') as f:
+                file.read())
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_BAD_TEST_WRONG_PREFERRED_TARGETS_VALUE_WRONG_TYPE)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 android_test_mapping_format.InvalidTestMappingError,
                 android_test_mapping_format.process_file,
-                f.read())
+                file.read())
 
     def test_invalid_test_mapping_wrong_test_option(self):
         """Verify that test config using wrong option can be detected."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(BAD_TEST_WRONG_OPTION)
-        with open(self.test_mapping_file, 'r') as f:
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_BAD_TEST_WRONG_OPTION)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 android_test_mapping_format.InvalidTestMappingError,
                 android_test_mapping_format.process_file,
-                f.read())
+                file.read())
 
     def test_invalid_test_mapping_wrong_import_key(self):
         """Verify that import setting using wrong key can be detected."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(BAD_IMPORT_WRONG_KEY)
-        with open(self.test_mapping_file, 'r') as f:
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_BAD_IMPORT_WRONG_KEY)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 android_test_mapping_format.InvalidTestMappingError,
                 android_test_mapping_format.process_file,
-                f.read())
+                file.read())
 
     def test_invalid_test_mapping_wrong_import_value(self):
         """Verify that import setting using wrong value can be detected."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(BAD_IMPORT_WRONG_IMPORT_VALUE)
-        with open(self.test_mapping_file, 'r') as f:
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_BAD_IMPORT_WRONG_IMPORT_VALUE)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 android_test_mapping_format.InvalidTestMappingError,
                 android_test_mapping_format.process_file,
-                f.read())
+                file.read())
 
     def test_invalid_test_mapping_file_patterns_value(self):
         """Verify that file_patterns using wrong value can be detected."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(BAD_FILE_PATTERNS)
-        with open(self.test_mapping_file, 'r') as f:
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_BAD_FILE_PATTERNS)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 android_test_mapping_format.InvalidTestMappingError,
                 android_test_mapping_format.process_file,
-                f.read())
+                file.read())
 
     def test_valid_test_mapping_file_with_supported_comments(self):
         """Verify that '//'-format comment can be filtered."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(TEST_MAPPING_WITH_SUPPORTED_COMMENTS)
-        with open(self.test_mapping_file, 'r') as f:
-            android_test_mapping_format.process_file(f.read())
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_TEST_MAPPING_WITH_SUPPORTED_COMMENTS)
+        with open(self.test_mapping_file, 'r') as file:
+            android_test_mapping_format.process_file(file.read())
 
     def test_valid_test_mapping_file_with_non_supported_comments(self):
         """Verify that non-supported comment can be detected."""
-        with open(self.test_mapping_file, 'w') as f:
-            f.write(TEST_MAPPING_WITH_NON_SUPPORTED_COMMENTS)
-        with open(self.test_mapping_file, 'r') as f:
+        with open(self.test_mapping_file, 'w') as file:
+            file.write(_TEST_MAPPING_WITH_NON_SUPPORTED_COMMENTS)
+        with open(self.test_mapping_file, 'r') as file:
             self.assertRaises(
                 ValueError, android_test_mapping_format.process_file,
-                f.read())
+                file.read())
 
 
 if __name__ == '__main__':