diff --git a/presubmit_support.py b/presubmit_support.py index 0d19a1cbd..ec246ed26 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -520,8 +520,7 @@ class OutputApi(object): def AppendCC(self, cc): """Appends a user to cc for this change.""" - if cc not in self.more_cc: - self.more_cc.append(cc) + self.more_cc.append(cc) def PresubmitPromptOrNotify(self, *args, **kwargs): """Warn the user when uploading, but only notify if committing.""" @@ -1602,6 +1601,9 @@ class PresubmitExecuter(object): presubmit_path)) logging.debug('Running %s done.', function_name) self.more_cc.extend(output_api.more_cc) + # Clear the CC list between running each presubmit check to prevent + # CCs from being repeatedly appended. + output_api.more_cc = [] else: # Old format if self.committing: @@ -1615,11 +1617,16 @@ class PresubmitExecuter(object): presubmit_path)) logging.debug('Running %s done.', function_name) self.more_cc.extend(output_api.more_cc) + # Clear the CC list between running each presubmit check to prevent + # CCs from being repeatedly appended. + output_api.more_cc = [] finally: for f in input_api._named_temporary_files: os.remove(f) + self.more_cc = sorted(set(self.more_cc)) + return results def _run_check_function(self, function_name, context, sink, presubmit_path): diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index d7b708039..8615bc8ef 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1596,6 +1596,65 @@ class OutputApiUnittest(PresubmitTestsBase): self.assertEqual(['chromium-reviews@chromium.org'], output_api.more_cc) + def testAppendCCAndMultipleChecks(self): + description_lines = ('Hello there', 'this is a change', 'BUG=123') + files = [ + ['A', 'foo\\blat.cc'], + ] + fake_presubmit = os.path.join(self.fake_root_dir, 'PRESUBMIT.py') + + change = presubmit.Change('mychange', '\n'.join(description_lines), + self.fake_root_dir, files, 0, 0, None) + + # Base case: AppendCC from multiple different checks should be reflected in + # the final result. + executer = presubmit.PresubmitExecuter(change, True, None, + presubmit.GerritAccessor()) + self.assertFalse( + executer.ExecPresubmitScript( + "PRESUBMIT_VERSION = '2.0.0'\n" + 'def CheckChangeAddCC1(input_api, output_api):\n' + " output_api.AppendCC('chromium-reviews@chromium.org')\n" + ' return []\n' + '\n' + 'def CheckChangeAppendCC2(input_api, output_api):\n' + " output_api.AppendCC('ipc-security-reviews@chromium.org')\n" + ' return []\n', fake_presubmit)) + self.assertEqual( + ['chromium-reviews@chromium.org', 'ipc-security-reviews@chromium.org'], + executer.more_cc) + + # Check that if one presubmit check appends a CC, it does not get duplicated + # into the more CC list by subsequent checks. + executer = presubmit.PresubmitExecuter(change, True, None, + presubmit.GerritAccessor()) + self.assertFalse( + executer.ExecPresubmitScript( + "PRESUBMIT_VERSION = '2.0.0'\n" + 'def CheckChangeAddCC(input_api, output_api):\n' + " output_api.AppendCC('chromium-reviews@chromium.org')\n" + ' return []\n' + '\n' + 'def CheckChangeDoNothing(input_api, output_api):\n' + ' return []\n', fake_presubmit)) + self.assertEqual(['chromium-reviews@chromium.org'], executer.more_cc) + + # Check that if multiple presubmit checks append the same CC, it gets + # deduplicated. + executer = presubmit.PresubmitExecuter(change, True, None, + presubmit.GerritAccessor()) + self.assertFalse( + executer.ExecPresubmitScript( + "PRESUBMIT_VERSION = '2.0.0'\n" + 'def CheckChangeAddCC1(input_api, output_api):\n' + " output_api.AppendCC('chromium-reviews@chromium.org')\n" + ' return []\n' + '\n' + 'def CheckChangeAppendCC2(input_api, output_api):\n' + " output_api.AppendCC('chromium-reviews@chromium.org')\n" + ' return []\n', fake_presubmit)) + self.assertEqual(['chromium-reviews@chromium.org'], executer.more_cc) + def testOutputApiHandling(self): presubmit.OutputApi.PresubmitError('!!!').handle() self.assertIsNotNone(sys.stdout.getvalue().count('!!!'))