From 6d938a0d85d3a617dc5f0b88b3fc8ad3993f390f Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Wed, 15 Jul 2020 18:56:47 +0000 Subject: [PATCH] presubmit_canned_checks: Add DIR_METADATA presubmit checks. - Add a check to validate DIR_METADATA files. - Add a check to enforce that OWNERS files contain no metadata if a DIR_METADATA file is present in the same directory. Bug: 1102997 Change-Id: Iedb09941001577dc81b1dda2241481ff667b2568 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2298330 Commit-Queue: Edward Lesmes Reviewed-by: Nodir Turakulov --- presubmit_canned_checks.py | 56 +++++++++++++++++++++++++++++ tests/presubmit_unittest.py | 71 +++++++++++++++++++++++++++++++++---- 2 files changed, 120 insertions(+), 7 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 10d172951..ff49088bb 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -1014,6 +1014,62 @@ def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings, return [] +def CheckDirMetadataFormat(input_api, output_api): + file_filter = lambda f: input_api.basename(f.LocalPath()) == 'DIR_METADATA' + affected_files = set([ + f.LocalPath() + for f in input_api.change.AffectedFiles( + include_deletes=False, file_filter=file_filter) + ]) + if not affected_files: + return [] + + name = 'Validate DIR_METADATA files' + dirmd_bin = 'dirmd.bat' if input_api.is_windows else 'dirmd' + kwargs = {} + if input_api.is_windows: + # Needed to be able to resolve 'dirmd.bat'. + kwargs['shell'] = True + + cmd = [dirmd_bin, 'validate'] + sorted(affected_files) + return [input_api.Command( + name, cmd, kwargs, output_api.PresubmitError)] + + +def CheckOwnersDirMetadataExclusive(input_api, output_api): + """Check that metadata in OWNERS files and DIR_METADATA files are mutually + exclusive. + """ + _METADATA_LINE_RE = input_api.re.compile( + r'^#\s*(TEAM|COMPONENT|OS|WPT-NOTIFY)+\s*:\s*\S+$', + input_api.re.MULTILINE) + file_filter = ( + lambda f: input_api.basename(f.LocalPath()) in ('OWNERS', 'DIR_METADATA')) + affected_dirs = set([ + input_api.os_path.dirname(f.AbsoluteLocalPath()) + for f in input_api.change.AffectedFiles( + include_deletes=False, file_filter=file_filter) + ]) + + errors = [] + for path in affected_dirs: + owners_path = input_api.os_path.join(path, 'OWNERS') + dir_metadata_path = input_api.os_path.join(path, 'DIR_METADATA') + if (not input_api.os_path.isfile(dir_metadata_path) + or not input_api.os_path.isfile(owners_path)): + continue + if _METADATA_LINE_RE.search(input_api.ReadFile(owners_path)): + errors.append(owners_path) + + if not errors: + return [] + + return [output_api.PresubmitError( + 'The following OWNERS files should contain no metadata, as there is a ' + 'DIR_METADATA file present in the same directory:\n' + + '\n'.join(errors))] + + def CheckOwnersFormat(input_api, output_api): affected_files = set([ f.LocalPath() diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 4575b9c90..cfb953618 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -2515,18 +2515,75 @@ the current line as well! self.assertEqual(results[0].__class__, presubmit.OutputApi.PresubmitNotifyResult) - def GetInputApiWithOWNERS(self, owners_content): - affected_file = mock.MagicMock(presubmit.GitAffectedFile) - affected_file.LocalPath = lambda: 'OWNERS' - affected_file.Action = lambda: 'M' - + def GetInputApiWithFiles(self, files): change = mock.MagicMock(presubmit.Change) - change.AffectedFiles = lambda **_: [affected_file] + change.AffectedFiles = lambda *a, **kw: ( + presubmit.Change.AffectedFiles(change, *a, **kw)) + change._affected_files = [] + for path, (action, _) in files.items(): + affected_file = mock.MagicMock(presubmit.GitAffectedFile) + affected_file.AbsoluteLocalPath.return_value = path + affected_file.LocalPath.return_value = path + affected_file.Action.return_value = action + change._affected_files.append(affected_file) input_api = self.MockInputApi(None, False) input_api.change = change + input_api.ReadFile = lambda path: files[path][1] + input_api.basename = os.path.basename + input_api.is_windows = sys.platform.startswith('win') + + os.path.exists = lambda path: path in files and files[path][0] != 'D' + os.path.isfile = os.path.exists + + return input_api + + def testCheckDirMetadataFormat(self): + input_api = self.GetInputApiWithFiles({ + 'DIR_METADATA': ('M', ''), + 'a/DIR_METADATA': ('M', ''), + 'a/b/OWNERS': ('M', ''), + 'c/DIR_METADATA': ('D', ''), + }) + + dirmd_bin = 'dirmd.bat' if input_api.is_windows else 'dirmd' + expected_cmd = [dirmd_bin, 'validate', 'DIR_METADATA', 'a/DIR_METADATA'] + + commands = presubmit_canned_checks.CheckDirMetadataFormat( + input_api, presubmit.OutputApi) + self.assertEqual(1, len(commands)) + self.assertEqual(expected_cmd, commands[0].cmd) + + def testCheckOwnersDirMetadataExclusiveWorks(self): + input_api = self.GetInputApiWithFiles({ + 'only-owners/OWNERS': ('M', '# COMPONENT: Monorail>Component'), + 'only-dir-metadata/DIR_METADATA': ('M', ''), + 'owners-has-no-metadata/DIR_METADATA': ('M', ''), + 'owners-has-no-metadata/OWNERS': ('M', 'no-metadata'), + 'deleted-owners/OWNERS': ('D', None), + 'deleted-owners/DIR_METADATA': ('M', ''), + 'deleted-dir-metadata/OWNERS': ('M', '# COMPONENT: Monorail>Component'), + 'deleted-dir-metadata/DIR_METADATA': ('D', None), + 'non-metadata-comment/OWNERS': ('M', '# WARNING: something.'), + 'non-metadata-comment/DIR_METADATA': ('M', ''), + }) + self.assertEqual( + [], + presubmit_canned_checks.CheckOwnersDirMetadataExclusive( + input_api, presubmit.OutputApi)) + + def testCheckOwnersDirMetadataExclusiveFails(self): + input_api = self.GetInputApiWithFiles({ + 'DIR_METADATA': ('M', ''), + 'OWNERS': ('M', '# COMPONENT: Monorail>Component'), + }) + results = presubmit_canned_checks.CheckOwnersDirMetadataExclusive( + input_api, presubmit.OutputApi) + self.assertEqual(1, len(results)) + self.assertIsInstance(results[0], presubmit.OutputApi.PresubmitError) - os.path.exists = lambda _: True + def GetInputApiWithOWNERS(self, owners_content): + input_api = self.GetInputApiWithFiles({'OWNERS': ('M', owners_content)}) owners_file = StringIO(owners_content) fopen = lambda *args: owners_file