From 2e72bb1c2dda3500e1ed8615cabc248173c649f2 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Tue, 17 Jan 2012 15:18:35 +0000 Subject: [PATCH] Overhaul breakpad to optionally disable output. Add git-cl dcommit unit test. This required fixing the adhoc mock to be more versatile. TBR=cmp@chromium.org BUG= TEST= Review URL: http://codereview.chromium.org/9178019 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@117896 0039d316-1c4b-4281-b951-d872f2087c98 --- breakpad.py | 72 +++++++------- gcl.py | 4 +- git_cl.py | 3 +- tests/git_cl_test.py | 221 +++++++++++++++++++++++++++++++------------ 4 files changed, 201 insertions(+), 99 deletions(-) diff --git a/breakpad.py b/breakpad.py index 6ae5b2361f..3540b261fe 100644 --- a/breakpad.py +++ b/breakpad.py @@ -42,13 +42,19 @@ IS_ENABLED = ( def post(url, params): """HTTP POST with timeout when it's supported.""" + if not IS_ENABLED: + # Make sure to not send anything for non googler. + return kwargs = {} if (sys.version_info[0] * 10 + sys.version_info[1]) >= 26: kwargs['timeout'] = 4 - request = urllib2.urlopen(url, urllib.urlencode(params), **kwargs) - out = request.read() - request.close() - return out + try: + request = urllib2.urlopen(url, urllib.urlencode(params), **kwargs) + out = request.read() + request.close() + return out + except IOError: + return 'There was a failure while trying to send the stack trace. Too bad.' def FormatException(e): @@ -79,46 +85,36 @@ def FormatException(e): return out -def SendStack(last_tb, stack, url=None, maxlen=50): +def SendStack(last_tb, stack, url=None, maxlen=50, verbose=True): """Sends the stack trace to the breakpad server.""" if not IS_ENABLED: - # Make sure to not send anything for non googler. return - if not url: - url = DEFAULT_URL + '/breakpad' - print 'Sending crash report ...' - try: - params = { - 'args': sys.argv, - 'stack': stack[0:4096], - 'user': getpass.getuser(), - 'exception': FormatException(last_tb), - 'host': _HOST_NAME, - 'cwd': os.getcwd(), - 'version': sys.version, - } - # pylint: disable=W0702 - print('\n'.join(' %s: %s' % (k, params[k][0:maxlen]) - for k in sorted(params))) - print(post(url, params)) - except IOError: - print('There was a failure while trying to send the stack trace. Too bad.') + def p(o): + if verbose: + print(o) + p('Sending crash report ...') + params = { + 'args': sys.argv, + 'cwd': os.getcwd(), + 'exception': FormatException(last_tb), + 'host': _HOST_NAME, + 'stack': stack[0:4096], + 'user': getpass.getuser(), + 'version': sys.version, + } + p('\n'.join(' %s: %s' % (k, params[k][0:maxlen]) for k in sorted(params))) + p(post(url or DEFAULT_URL + '/breakpad', params)) def SendProfiling(url=None): - try: - if not url: - url = DEFAULT_URL + '/profiling' - params = { - 'argv': ' '.join(sys.argv), - # Strip the hostname. - 'domain': _HOST_NAME.split('.', 1)[-1], - 'duration': time.time() - _TIME_STARTED, - 'platform': sys.platform, - } - post(url, params) - except IOError: - pass + params = { + 'argv': ' '.join(sys.argv), + # Strip the hostname. + 'domain': _HOST_NAME.split('.', 1)[-1], + 'duration': time.time() - _TIME_STARTED, + 'platform': sys.platform, + } + post(url or DEFAULT_URL + '/profiling', params) def CheckForException(): diff --git a/gcl.py b/gcl.py index 8a0dfc64e9..86dc43917e 100755 --- a/gcl.py +++ b/gcl.py @@ -747,9 +747,11 @@ def GenerateDiff(files): def OptionallyDoPresubmitChecks(change_info, committing, args): if FilterFlag(args, "--no_presubmit") or FilterFlag(args, "--force"): breakpad.SendStack( + breakpad.DEFAULT_URL + '/breakpad', 'GclHooksBypassedCommit', 'Issue %s/%s bypassed hook when committing' % - (change_info.rietveld, change_info.issue)) + (change_info.rietveld, change_info.issue), + verbose=False) return presubmit_support.PresubmitOutput() return DoPresubmitChecks(change_info, committing, True) diff --git a/git_cl.py b/git_cl.py index 2922345af8..9ba9062692 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1090,7 +1090,8 @@ def SendUpstream(parser, args, cmd): breakpad.SendStack( 'GitClHooksBypassedCommit', 'Issue %s/%s bypassed hook when committing' % - (cl.GetRietveldServer(), cl.GetIssue())) + (cl.GetRietveldServer(), cl.GetIssue()), + verbose=False) description = options.message if not description and cl.GetIssue(): diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 7d32648601..8b46eba0a5 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -18,49 +18,47 @@ import git_cl import subprocess2 +class PresubmitMock(object): + def __init__(self, *args, **kwargs): + self.reviewers = [] + @staticmethod + def should_continue(): + return True + + +class RietveldMock(object): + def __init__(self, *args, **kwargs): + pass + @staticmethod + def get_description(issue): + return 'Issue: %d' % issue + + +class WatchlistsMock(object): + def __init__(self, _): + pass + @staticmethod + def GetWatchersForPaths(_): + return ['joe@example.com'] + + class TestGitCl(TestCase): def setUp(self): super(TestGitCl, self).setUp() self.calls = [] self._calls_done = 0 - def mock_call(args, **kwargs): - expected_args, result = self.calls.pop(0) - self.assertEquals( - expected_args, - args, - '@%d Expected: %r Actual: %r' % ( - self._calls_done, expected_args, args)) - self._calls_done += 1 - return result - self.mock(subprocess2, 'call', mock_call) - self.mock(subprocess2, 'check_call', mock_call) - self.mock(subprocess2, 'check_output', mock_call) - self.mock(subprocess2, 'communicate', mock_call) - self.mock(subprocess2, 'Popen', mock_call) - + self.mock(subprocess2, 'call', self._mocked_call) + self.mock(subprocess2, 'check_call', self._mocked_call) + self.mock(subprocess2, 'check_output', self._mocked_call) + self.mock(subprocess2, 'communicate', self._mocked_call) + self.mock(subprocess2, 'Popen', self._mocked_call) self.mock(git_cl, 'FindCodereviewSettingsFile', lambda: '') - - class PresubmitMock(object): - def __init__(self, *args, **kwargs): - self.reviewers = [] - @staticmethod - def should_continue(): - return True + self.mock(git_cl, 'ask_for_data', self._mocked_call) + self.mock(git_cl.breakpad, 'post', self._mocked_call) + self.mock(git_cl.breakpad, 'SendStack', self._mocked_call) self.mock(git_cl.presubmit_support, 'DoPresubmitChecks', PresubmitMock) - - class RietveldMock(object): - def __init__(self, *args, **kwargs): - pass self.mock(git_cl.rietveld, 'Rietveld', RietveldMock) - self.mock(git_cl.upload, 'RealMain', self.fail) - - class WatchlistsMock(object): - def __init__(self, _): - pass - @staticmethod - def GetWatchersForPaths(_): - return ['joe@example.com'] self.mock(git_cl.watchlists, 'Watchlists', WatchlistsMock) # It's important to reset settings to not have inter-tests interference. git_cl.settings = None @@ -70,6 +68,19 @@ class TestGitCl(TestCase): self.assertEquals([], self.calls) super(TestGitCl, self).tearDown() + def _mocked_call(self, *args, **kwargs): + self.assertTrue( + self.calls, + '@%d Expected: Actual: %r' % (self._calls_done, args)) + expected_args, result = self.calls.pop(0) + self.assertEquals( + expected_args, + args, + '@%d Expected: %r Actual: %r' % ( + self._calls_done, expected_args, args)) + self._calls_done += 1 + return result + @classmethod def _upload_calls(cls): return cls._git_base_calls() + cls._git_upload_calls() @@ -77,38 +88,116 @@ class TestGitCl(TestCase): @staticmethod def _git_base_calls(): return [ - (['git', 'update-index', '--refresh', '-q'], ''), - (['git', 'diff-index', 'HEAD'], ''), - (['git', 'config', 'rietveld.server'], 'codereview.example.com'), - (['git', 'symbolic-ref', 'HEAD'], 'master'), - (['git', 'config', 'branch.master.merge'], 'master'), - (['git', 'config', 'branch.master.remote'], 'origin'), - (['git', 'rev-parse', '--show-cdup'], ''), - (['git', 'rev-parse', 'HEAD'], '12345'), - (['git', 'diff', '--name-status', '-r', 'master...', '.'], - 'M\t.gitignore\n'), - (['git', 'rev-parse', '--git-dir'], '.git'), - (['git', 'config', 'branch.master.rietveldissue'], ''), - (['git', 'config', 'branch.master.rietveldpatchset'], ''), - (['git', 'log', '--pretty=format:%s%n%n%b', 'master...'], 'foo'), - (['git', 'config', 'user.email'], 'me@example.com'), - (['git', 'diff', '--no-ext-diff', '--stat', '-M', 'master...'], '+dat'), - (['git', 'log', '--pretty=format:%s\n\n%b', 'master..'], 'desc\n'), + ((['git', 'update-index', '--refresh', '-q'],), ''), + ((['git', 'diff-index', 'HEAD'],), ''), + ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), + ((['git', 'symbolic-ref', 'HEAD'],), 'master'), + ((['git', 'config', 'branch.master.merge'],), 'master'), + ((['git', 'config', 'branch.master.remote'],), 'origin'), + ((['git', 'rev-parse', '--show-cdup'],), ''), + ((['git', 'rev-parse', 'HEAD'],), '12345'), + ((['git', 'diff', '--name-status', '-r', 'master...', '.'],), + 'M\t.gitignore\n'), + ((['git', 'rev-parse', '--git-dir'],), '.git'), + ((['git', 'config', 'branch.master.rietveldissue'],), ''), + ((['git', 'config', 'branch.master.rietveldpatchset'],), ''), + ((['git', 'log', '--pretty=format:%s%n%n%b', 'master...'],), 'foo'), + ((['git', 'config', 'user.email'],), 'me@example.com'), + ((['git', 'diff', '--no-ext-diff', '--stat', '-M', 'master...'],), + '+dat'), + ((['git', 'log', '--pretty=format:%s\n\n%b', 'master..'],), 'desc\n'), ] @staticmethod def _git_upload_calls(): return [ - (['git', 'config', 'rietveld.cc'], ''), - (['git', 'config', '--get-regexp', '^svn-remote\\.'], (('', None), 0)), - (['git', 'rev-parse', '--show-cdup'], ''), - (['git', 'svn', 'info'], ''), - (['git', 'config', 'branch.master.rietveldissue', '1'], ''), - (['git', 'config', 'branch.master.rietveldserver', - 'https://codereview.example.com'], ''), - (['git', 'config', 'branch.master.rietveldpatchset', '2'], ''), + ((['git', 'config', 'rietveld.cc'],), ''), + ((['git', 'config', '--get-regexp', '^svn-remote\\.'],), + (('', None), 0)), + ((['git', 'rev-parse', '--show-cdup'],), ''), + ((['git', 'svn', 'info'],), ''), + ((['git', 'config', 'branch.master.rietveldissue', '1'],), ''), + ((['git', 'config', 'branch.master.rietveldserver', + 'https://codereview.example.com'],), ''), + ((['git', 'config', 'branch.master.rietveldpatchset', '2'],), ''), ] + @classmethod + def _dcommit_calls_1(cls): + return [ + ((['git', 'config', '--get-regexp', '^svn-remote\\.'],), + ((('svn-remote.svn.url svn://svn.chromium.org/chrome\n' + 'svn-remote.svn.fetch trunk/src:refs/remotes/origin/master'), + None), + 0)), + ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), + ((['git', 'symbolic-ref', 'HEAD'],), 'refs/heads/working'), + ((['git', 'config', 'branch.working.merge'],), 'refs/heads/master'), + ((['git', 'config', 'branch.working.remote'],), 'origin'), + ((['git', 'update-index', '--refresh', '-q'],), ''), + ((['git', 'diff-index', 'HEAD'],), ''), + ((['git', 'rev-list', '^refs/heads/working', + 'refs/remotes/origin/master'],), + ''), + ((['git', 'log', '--grep=^git-svn-id:', '-1', '--pretty=format:%H'],), + '3fc18b62c4966193eb435baabe2d18a3810ec82e'), + ((['git', 'rev-list', '^3fc18b62c4966193eb435baabe2d18a3810ec82e', + 'refs/remotes/origin/master'],), ''), + ] + + @classmethod + def _dcommit_calls_normal(cls): + return [ + ((['git', 'rev-parse', '--show-cdup'],), ''), + ((['git', 'rev-parse', 'HEAD'],), + '00ff397798ea57439712ed7e04ab96e13969ef40'), + ((['git', 'diff', '--name-status', '-r', 'refs/remotes/origin/master...', + '.'],), + 'M\tPRESUBMIT.py'), + ((['git', 'rev-parse', '--git-dir'],), '.git'), + ((['git', 'config', 'branch.working.rietveldissue'],), '12345'), + ((['git', 'config', 'branch.working.rietveldserver'],), + 'codereview.example.com'), + ((['git', 'config', 'branch.working.rietveldpatchset'],), '31137'), + ((['git', 'config', 'user.email'],), 'author@example.com'), + ((['git', 'config', 'rietveld.tree-status-url'],), ''), + ] + + @classmethod + def _dcommit_calls_bypassed(cls): + return [ + ((['git', 'rev-parse', '--git-dir'],), '.git'), + ((['git', 'config', 'branch.working.rietveldissue'],), '12345'), + ((['git', 'config', 'branch.working.rietveldserver'],), + 'codereview.example.com'), + (('GitClHooksBypassedCommit', + 'Issue https://codereview.example.com/12345 bypassed hook when ' + 'committing'), None), + ] + + @classmethod + def _dcommit_calls_3(cls): + return [ + ((['git', 'diff', '--stat', 'refs/remotes/origin/master', + 'refs/heads/working'],), + (' PRESUBMIT.py | 2 +-\n' + ' 1 files changed, 1 insertions(+), 1 deletions(-)\n')), + (('About to commit; enter to confirm.',), None), + ((['git', 'show-ref', '--quiet', '--verify', + 'refs/heads/git-cl-commit'],), + (('', None), 0)), + ((['git', 'branch', '-D', 'git-cl-commit'],), ''), + ((['git', 'rev-parse', '--show-cdup'],), '\n'), + ((['git', 'checkout', '-q', '-b', 'git-cl-commit'],), ''), + ((['git', 'reset', '--soft', 'refs/remotes/origin/master'],), ''), + ((['git', 'commit', '-m', + 'Issue: 12345\n\nReview URL: https://codereview.example.com/12345'],), + ''), + ((['git', 'svn', 'dcommit', '--no-rebase', '--rmdir'],), (('', None), 0)), + ((['git', 'checkout', '-q', 'working'],), ''), + ((['git', 'branch', '-D', 'git-cl-commit'],), ''), + ] + @staticmethod def _cmd_line(description, args): """Returns the upload command line passed to upload.RealMain().""" @@ -217,6 +306,20 @@ class TestGitCl(TestCase): self.assertEquals( 'Must specify reviewers to send email.\n', mock.buf.getvalue()) + def test_dcommit(self): + self.calls = ( + self._dcommit_calls_1() + + self._dcommit_calls_normal() + + self._dcommit_calls_3()) + git_cl.main(['dcommit']) + + def test_dcommit_bypass_hooks(self): + self.calls = ( + self._dcommit_calls_1() + + self._dcommit_calls_bypassed() + + self._dcommit_calls_3()) + git_cl.main(['dcommit', '--bypass-hooks']) + if __name__ == '__main__': unittest.main()