diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 839fe1d7f..52081912b 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -12,7 +12,7 @@ def CheckChangeHasTestField(input_api, output_api): return [] else: return [output_api.PresubmitNotifyResult( - "Changelist should have a TEST= field. TEST=none is allowed.")] + 'Changelist should have a TEST= field. TEST=none is allowed.')] def CheckChangeHasBugField(input_api, output_api): @@ -21,7 +21,7 @@ def CheckChangeHasBugField(input_api, output_api): return [] else: return [output_api.PresubmitNotifyResult( - "Changelist should have a BUG= field. BUG=none is allowed.")] + 'Changelist should have a BUG= field. BUG=none is allowed.')] def CheckChangeHasTestedField(input_api, output_api): @@ -29,7 +29,7 @@ def CheckChangeHasTestedField(input_api, output_api): if input_api.change.TESTED: return [] else: - return [output_api.PresubmitError("Changelist must have a TESTED= field.")] + return [output_api.PresubmitError('Changelist must have a TESTED= field.')] def CheckChangeHasQaField(input_api, output_api): @@ -37,7 +37,7 @@ def CheckChangeHasQaField(input_api, output_api): if input_api.change.QA: return [] else: - return [output_api.PresubmitError("Changelist must have a QA= field.")] + return [output_api.PresubmitError('Changelist must have a QA= field.')] def CheckDoNotSubmitInDescription(input_api, output_api): @@ -46,7 +46,7 @@ def CheckDoNotSubmitInDescription(input_api, output_api): keyword = 'DO NOT ' + 'SUBMIT' if keyword in input_api.change.DescriptionText(): return [output_api.PresubmitError( - keyword + " is present in the changelist description.")] + keyword + ' is present in the changelist description.')] else: return [] @@ -56,9 +56,9 @@ def CheckChangeHasDescription(input_api, output_api): text = input_api.change.DescriptionText() if text.strip() == '': if input_api.is_committing: - return [output_api.PresubmitError("Add a description.")] + return [output_api.PresubmitError('Add a description.')] else: - return [output_api.PresubmitNotifyResult("Add a description.")] + return [output_api.PresubmitNotifyResult('Add a description.')] return [] ### Content checks @@ -75,7 +75,7 @@ def CheckDoNotSubmitInFiles(input_api, output_api): def CheckChangeLintsClean(input_api, output_api, source_file_filter=None): - """Checks that all ".cc" and ".h" files pass cpplint.py.""" + """Checks that all '.cc' and '.h' files pass cpplint.py.""" _RE_IS_TEST = input_api.re.compile(r'.*tests?.(cc|h)$') result = [] @@ -92,9 +92,9 @@ def CheckChangeLintsClean(input_api, output_api, source_file_filter=None): # - runtime/int : Can be fixed long term; volume of errors too high # - runtime/virtual : Broken now, but can be fixed in the future? # - whitespace/braces : We have a lot of explicit scoping in chrome code. - cpplint._SetFilters("-build/include,-build/include_order,-build/namespace," - "-readability/casting,-runtime/int,-runtime/virtual," - "-whitespace/braces") + cpplint._SetFilters('-build/include,-build/include_order,-build/namespace,' + '-readability/casting,-runtime/int,-runtime/virtual,' + '-whitespace/braces') # We currently are more strict with normal code than unit tests; 4 and 5 are # the verbosity level that would normally be passed to cpplint.py through @@ -114,7 +114,7 @@ def CheckChangeLintsClean(input_api, output_api, source_file_filter=None): res_type = output_api.PresubmitError else: res_type = output_api.PresubmitPromptWarning - result = [res_type("Changelist failed cpplint.py check.")] + result = [res_type('Changelist failed cpplint.py check.')] return result @@ -127,7 +127,7 @@ def CheckChangeHasNoCR(input_api, output_api, source_file_filter=None): cr_files.append(f.LocalPath()) if cr_files: return [output_api.PresubmitPromptWarning( - "Found a CR character in these files:", items=cr_files)] + 'Found a CR character in these files:', items=cr_files)] return [] @@ -162,7 +162,7 @@ def CheckSvnModifiedDirectories(input_api, output_api, source_file_filter=None): else: error_type = output_api.PresubmitNotifyResult errors.append(error_type( - "Potential accidental commits in changelist %s:" % f.LocalPath(), + 'Potential accidental commits in changelist %s:' % f.LocalPath(), items=bad_files)) return errors @@ -173,7 +173,7 @@ def CheckChangeHasOnlyOneEol(input_api, output_api, source_file_filter=None): for f in input_api.AffectedSourceFiles(source_file_filter): contents = input_api.ReadFile(f, 'rb') # Check that the file ends in one and only one newline character. - if len(contents) > 1 and (contents[-1:] != "\n" or contents[-2:-1] == "\n"): + if len(contents) > 1 and (contents[-1:] != '\n' or contents[-2:-1] == '\n'): eof_files.append(f.LocalPath()) if eof_files: @@ -196,12 +196,12 @@ def CheckChangeHasNoCrAndHasOnlyOneEol(input_api, output_api, if '\r' in contents: cr_files.append(f.LocalPath()) # Check that the file ends in one and only one newline character. - if len(contents) > 1 and (contents[-1:] != "\n" or contents[-2:-1] == "\n"): + if len(contents) > 1 and (contents[-1:] != '\n' or contents[-2:-1] == '\n'): eof_files.append(f.LocalPath()) outputs = [] if cr_files: outputs.append(output_api.PresubmitPromptWarning( - "Found a CR character in these files:", items=cr_files)) + 'Found a CR character in these files:', items=cr_files)) if eof_files: outputs.append(output_api.PresubmitPromptWarning( 'These files should end in one (and only one) newline character:', @@ -216,10 +216,10 @@ def CheckChangeHasNoTabs(input_api, output_api, source_file_filter=None): tabs = [] for f, line_num, line in input_api.RightHandSideLines(source_file_filter): if '\t' in line: - tabs.append("%s, line %s" % (f.LocalPath(), line_num)) + tabs.append('%s, line %s' % (f.LocalPath(), line_num)) if tabs: - return [output_api.PresubmitPromptWarning("Found a tab character in:", - long_text="\n".join(tabs))] + return [output_api.PresubmitPromptWarning('Found a tab character in:', + long_text='\n'.join(tabs))] return [] @@ -229,11 +229,11 @@ def CheckChangeHasNoStrayWhitespace(input_api, output_api, errors = [] for f, line_num, line in input_api.RightHandSideLines(source_file_filter): if line.rstrip() != line: - errors.append("%s, line %s" % (f.LocalPath(), line_num)) + errors.append('%s, line %s' % (f.LocalPath(), line_num)) if errors: return [output_api.PresubmitPromptWarning( - "Found line ending with white spaces in:", - long_text="\n".join(errors))] + 'Found line ending with white spaces in:', + long_text='\n'.join(errors))] return [] @@ -260,7 +260,7 @@ def CheckLongLines(input_api, output_api, maxlen=80, source_file_filter=None): break if bad: - msg = "Found lines longer than %s characters (first 5 shown)." % maxlen + msg = 'Found lines longer than %s characters (first 5 shown).' % maxlen return [output_api.PresubmitPromptWarning(msg, items=bad)] else: return [] @@ -281,7 +281,7 @@ def CheckLicense(input_api, output_api, license, source_file_filter=None): else: res_type = output_api.PresubmitNotifyResult return [res_type( - "Found a bad license header in these files:", items=bad_files)] + 'Found a bad license header in these files:', items=bad_files)] return [] @@ -327,7 +327,7 @@ def CheckSvnProperty(input_api, output_api, prop, expected, affected_files): res_type = output_api.PresubmitError else: res_type = output_api.PresubmitNotifyResult - message = "Run the command: svn pset %s %s \\" % (prop, expected) + message = 'Run the command: svn pset %s %s \\' % (prop, expected) return [res_type(message, items=bad)] return [] @@ -344,14 +344,15 @@ def CheckDoNotSubmit(input_api, output_api): def CheckTreeIsOpen(input_api, output_api, url, closed): """Checks that an url's content doesn't match a regexp that would mean that the tree is closed.""" - assert(input_api.is_committing) + if not input_api.is_committing: + return [] try: connection = input_api.urllib2.urlopen(url) status = connection.read() connection.close() if input_api.re.match(closed, status): long_text = status + '\n' + url - return [output_api.PresubmitPromptWarning("The tree is closed.", + return [output_api.PresubmitPromptWarning('The tree is closed.', long_text=long_text)] except IOError: pass @@ -375,7 +376,7 @@ def RunPythonUnitTests(input_api, output_api, unit_tests): cwd = None env = None unit_test_name = unit_test - # "python -m test.unit_test" doesn't work. We need to change to the right + # 'python -m test.unit_test' doesn't work. We need to change to the right # directory instead. if '.' in unit_test: # Tests imported in submodules (subdirectories) assume that the current @@ -394,8 +395,8 @@ def RunPythonUnitTests(input_api, output_api, unit_tests): subproc = input_api.subprocess.Popen( [ input_api.python_executable, - "-m", - "%s" % unit_test + '-m', + '%s' % unit_test ], cwd=cwd, env=env, @@ -405,9 +406,92 @@ def RunPythonUnitTests(input_api, output_api, unit_tests): stdoutdata, stderrdata = subproc.communicate() # Discard the output if returncode == 0 if subproc.returncode: - outputs.append("Test '%s' failed with code %d\n%s\n%s\n" % ( + outputs.append('Test \'%s\' failed with code %d\n%s\n%s\n' % ( unit_test_name, subproc.returncode, stdoutdata, stderrdata)) if outputs: - return [message_type("%d unit tests failed." % len(outputs), + return [message_type('%d unit tests failed.' % len(outputs), long_text='\n'.join(outputs))] return [] + + +def CheckRietveldTryJobExecution(input_api, output_api, host_url, platforms, + owner): + if not input_api.is_committing: + return [] + if not input_api.change.issue or not input_api.change.patchset: + return [] + url = '%s/%d/get_build_results/%d' % ( + host_url, input_api.change.issue, input_api.change.patchset) + try: + connection = input_api.urllib2.urlopen(url) + # platform|status|url + values = [item.split('|', 2) for item in connection.read().splitlines()] + connection.close() + except input_api.urllib2.HTTPError, e: + if e.code == 404: + # Fallback to no try job. + return [output_api.PresubmitPromptWarning( + 'You should try the patch first.')] + else: + # Another HTTP error happened, warn the user. + return [output_api.PresubmitPromptWarning( + 'Got %s while looking for try job status.' % str(e))] + + if not values: + # It returned an empty list. Probably a private review. + return [] + # Reformat as an dict of platform: [status, url] + values = dict([[v[0], [v[1], v[2]]] for v in values if len(v) == 3]) + if not values: + # It returned useless data. + return [output_api.PresubmitNotifyResult('Failed to parse try job results')] + + for platform in platforms: + values.setdefault(platform, ['not started', '']) + message = None + non_success = [k.upper() for k,v in values.iteritems() if v[0] != 'success'] + if 'failure' in [v[0] for v in values.itervalues()]: + message = 'Try job failures on %s!\n' % ', '.join(non_success) + elif non_success: + message = ('Unfinished (or not even started) try jobs on ' + '%s.\n') % ', '.join(non_success) + if message: + message += ( + 'Is try server wrong or broken? Please notify %s. ' + 'Thanks.\n' % owner) + return [output_api.PresubmitPromptWarning(message=message)] + return [] + + +def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings, + ignored): + if not input_api.json: + return [output_api.PresubmitPromptWarning( + 'Please install simplejson or upgrade to python 2.6+')] + try: + connection = input_api.urllib2.urlopen(url) + raw_data = connection.read() + connection.close() + except IOError: + return [output_api.PresubmitNotifyResult('%s is not accessible' % url)] + + try: + data = input_api.json.loads(raw_data) + except ValueError: + return [output_api.PresubmitNotifyResult('Received malformed json while ' + 'looking up buildbot status')] + + out = [] + for (builder_name, builder) in data.iteritems(): + if builder_name in ignored: + continue + pending_builds_len = len(builder.get('pending_builds', [])) + if pending_builds_len > max_pendings: + out.append('%s has %d build(s) pending' % + (builder_name, pending_builds_len)) + if out: + return [output_api.PresubmitPromptWarning( + 'Build(s) pending. It is suggested to wait that no more than %d ' + 'builds are pending.' % max_pendings, + long_text='\n'.join(out))] + return [] diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index d49256064..084d96d2d 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1078,6 +1078,7 @@ class CannedChecksUnittest(PresubmitTestsBase): 'CheckDoNotSubmit', 'CheckDoNotSubmitInDescription', 'CheckDoNotSubmitInFiles', 'CheckLongLines', 'CheckTreeIsOpen', 'RunPythonUnitTests', + 'CheckBuildbotPendingBuilds', 'CheckRietveldTryJobExecution', ] # If this test fails, you should add the relevant test. self.compareMembers(presubmit_canned_checks, members) @@ -1528,6 +1529,83 @@ class CannedChecksUnittest(PresubmitTestsBase): input_api, presubmit.OutputApi, ['test_module']) self.assertEquals(len(results), 0) + def testCheckRietveldTryJobExecutionBad(self): + change = self.mox.CreateMock(presubmit.SvnChange) + change.scm = 'svn' + change.issue = 2 + change.patchset = 5 + input_api = self.MockInputApi(change, True) + connection = self.mox.CreateMockAnything() + input_api.urllib2.urlopen('uurl/2/get_build_results/5').AndReturn( + connection) + connection.read().AndReturn('foo') + connection.close() + self.mox.ReplayAll() + + results = presubmit_canned_checks.CheckRietveldTryJobExecution( + input_api, presubmit.OutputApi, 'uurl', ('mac', 'linux'), 'georges') + self.assertEquals(len(results), 1) + self.assertEquals(results[0].__class__, + presubmit.OutputApi.PresubmitNotifyResult) + + def testCheckRietveldTryJobExecutionGood(self): + change = self.mox.CreateMock(presubmit.SvnChange) + change.scm = 'svn' + change.issue = 2 + change.patchset = 5 + input_api = self.MockInputApi(change, True) + connection = self.mox.CreateMockAnything() + input_api.urllib2.urlopen('uurl/2/get_build_results/5').AndReturn( + connection) + connection.read().AndReturn("""amiga|Foo|blah +linux|failure|bleh +mac|success|blew +""") + connection.close() + self.mox.ReplayAll() + + results = presubmit_canned_checks.CheckRietveldTryJobExecution( + input_api, presubmit.OutputApi, 'uurl', ('mac', 'linux', 'amiga'), + 'georges') + self.assertEquals(len(results), 1) + self.assertEquals(results[0].__class__, + presubmit.OutputApi.PresubmitPromptWarning) + + def testCheckBuildbotPendingBuildsBad(self): + input_api = self.MockInputApi(None, True) + input_api.json = presubmit.json + connection = self.mox.CreateMockAnything() + input_api.urllib2.urlopen('uurl').AndReturn(connection) + connection.read().AndReturn('foo') + connection.close() + self.mox.ReplayAll() + + results = presubmit_canned_checks.CheckBuildbotPendingBuilds( + input_api, presubmit.OutputApi, 'uurl', 2, ('foo')) + self.assertEquals(len(results), 1) + self.assertEquals(results[0].__class__, + presubmit.OutputApi.PresubmitNotifyResult) + + def testCheckBuildbotPendingBuildsGood(self): + input_api = self.MockInputApi(None, True) + input_api.json = presubmit.json + connection = self.mox.CreateMockAnything() + input_api.urllib2.urlopen('uurl').AndReturn(connection) + connection.read().AndReturn(""" + { + 'b1': { 'pending_builds': [0, 1, 2, 3, 4, 5, 6, 7] }, + 'foo': { 'pending_builds': [0, 1, 2, 3, 4, 5, 6, 7] }, + 'b2': { 'pending_builds': [0] } + }""") + connection.close() + self.mox.ReplayAll() + + results = presubmit_canned_checks.CheckBuildbotPendingBuilds( + input_api, presubmit.OutputApi, 'uurl', 2, ('foo')) + self.assertEquals(len(results), 1) + self.assertEquals(results[0].__class__, + presubmit.OutputApi.PresubmitNotifyResult) + if __name__ == '__main__': import unittest