From 01f4a4ff1cb2c4491c7fb33c1fece43cea94754a Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Sat, 3 Nov 2018 00:40:38 +0000 Subject: [PATCH] git-cl: Add tests for metrics collection. Bug: 897394 Change-Id: I07959e870fef4e6a6b8e6e7c974397d3306460c1 Reviewed-on: https://chromium-review.googlesource.com/c/1315839 Commit-Queue: Edward Lesmes Reviewed-by: Andrii Shyshkalov --- git_cl.py | 10 +++++-- metrics_utils.py | 2 +- tests/git_cl_test.py | 67 ++++++++++++++++++++++++++++++------------- tests/metrics_test.py | 4 +-- 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/git_cl.py b/git_cl.py index 176e0b715..2a0f47a9a 100755 --- a/git_cl.py +++ b/git_cl.py @@ -188,6 +188,12 @@ def time_sleep(seconds): return time.sleep(seconds) +def time_time(): + # Use this so that it can be mocked in tests without interfering with python + # system machinery. + return time.time() + + def ask_for_data(prompt): try: return raw_input(prompt) @@ -3008,7 +3014,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): GERRIT_ERR_LOGGER.info(line) filter_fn = FilterHeaders() - before_push = time.time() + before_push = time_time() push_returncode = 0 push_stdout = gclient_utils.CheckCallAndFilter( ['git', 'push', self.GetRemoteUrl(), refspec], @@ -3026,7 +3032,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): finally: metrics.collector.add_repeated('sub_commands', { 'command': 'git push', - 'execution_time': time.time() - before_push, + 'execution_time': time_time() - before_push, 'exit_code': push_returncode, 'arguments': metrics_utils.extract_known_subcommand_args(refspec_opts), }) diff --git a/metrics_utils.py b/metrics_utils.py index 883c6946e..7815e16fb 100644 --- a/metrics_utils.py +++ b/metrics_utils.py @@ -205,7 +205,7 @@ def extract_known_subcommand_args(args): arg = arg.split('=')[0] if arg in KNOWN_SUBCOMMAND_ARGS: known_args.append(arg) - return known_args + return sorted(known_args) def extract_http_metrics(request_uri, method, status, response_time): diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 63b9b0c5d..4560065c6 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -622,6 +622,10 @@ class TestGitCl(TestCase): super(TestGitCl, self).setUp() self.calls = [] self._calls_done = [] + self.mock(git_cl, 'time_time', + lambda: self._mocked_call('time.time')) + self.mock(git_cl.metrics.collector, 'add_repeated', + lambda *a: self._mocked_call('add_repeated', *a)) self.mock(subprocess2, 'call', self._mocked_call) self.mock(subprocess2, 'check_call', self._mocked_call) self.mock(subprocess2, 'check_output', self._mocked_call) @@ -1007,16 +1011,22 @@ class TestGitCl(TestCase): '1hashPerLine\n'), ] + metrics_arguments = [] + if notify: ref_suffix = '%ready,notify=ALL' + metrics_arguments += ['ready', 'notify=ALL'] else: if not issue and squash: ref_suffix = '%wip' + metrics_arguments.append('wip') else: ref_suffix = '%notify=NONE' + metrics_arguments.append('notify=NONE') if title: ref_suffix += ',m=' + title + metrics_arguments.append('m') calls += [ ((['git', 'config', 'rietveld.cc'],), ''), @@ -1025,9 +1035,11 @@ class TestGitCl(TestCase): # All reviwers and ccs get into ref_suffix. for r in sorted(reviewers): ref_suffix += ',r=%s' % r + metrics_arguments.append('r') for c in sorted(['chromium-reviews+test-more-cc@chromium.org', 'joe@example.com'] + cc): ref_suffix += ',cc=%s' % c + metrics_arguments.append('cc') reviewers, cc = [], [] else: # TODO(crbug/877717): remove this case. @@ -1043,36 +1055,51 @@ class TestGitCl(TestCase): for r in sorted(reviewers): if r != 'bad-account-or-email': ref_suffix += ',r=%s' % r + metrics_arguments.append('r') reviewers.remove(r) for c in sorted(['joe@example.com'] + cc): ref_suffix += ',cc=%s' % c + metrics_arguments.append('cc') if c in cc: cc.remove(c) if not tbr: for k, v in sorted((labels or {}).items()): ref_suffix += ',l=%s+%d' % (k, v) + metrics_arguments.append('l=%s+%d' % (k, v)) + + calls += [ + (('time.time',), 1000,), + ((['git', 'push', + 'https://%s.googlesource.com/my/repo' % short_hostname, + ref_to_push + ':refs/for/refs/heads/master' + ref_suffix],), + (('remote:\n' + 'remote: Processing changes: (\)\n' + 'remote: Processing changes: (|)\n' + 'remote: Processing changes: (/)\n' + 'remote: Processing changes: (-)\n' + 'remote: Processing changes: new: 1 (/)\n' + 'remote: Processing changes: new: 1, done\n' + 'remote:\n' + 'remote: New Changes:\n' + 'remote: https://%s-review.googlesource.com/#/c/my/repo/+/123456' + ' XXX\n' + 'remote:\n' + 'To https://%s.googlesource.com/my/repo\n' + ' * [new branch] hhhh -> refs/for/refs/heads/master\n' + ) % (short_hostname, short_hostname)),), + (('time.time',), 2000,), + (('add_repeated', + 'sub_commands', + { + 'execution_time': 1000, + 'command': 'git push', + 'exit_code': 0, + 'arguments': sorted(metrics_arguments), + }), + None,), + ] - calls.append(( - (['git', 'push', - 'https://%s.googlesource.com/my/repo' % short_hostname, - ref_to_push + ':refs/for/refs/heads/master' + ref_suffix],), - (('remote:\n' - 'remote: Processing changes: (\)\n' - 'remote: Processing changes: (|)\n' - 'remote: Processing changes: (/)\n' - 'remote: Processing changes: (-)\n' - 'remote: Processing changes: new: 1 (/)\n' - 'remote: Processing changes: new: 1, done\n' - 'remote:\n' - 'remote: New Changes:\n' - 'remote: https://%s-review.googlesource.com/#/c/my/repo/+/123456' - ' XXX\n' - 'remote:\n' - 'To https://%s.googlesource.com/my/repo\n' - ' * [new branch] hhhh -> refs/for/refs/heads/master\n' - ) % (short_hostname, short_hostname)) - )) if squash: calls += [ ((['git', 'config', 'branch.master.gerritissue', '123456'],), diff --git a/tests/metrics_test.py b/tests/metrics_test.py index 763658882..2debaec64 100644 --- a/tests/metrics_test.py +++ b/tests/metrics_test.py @@ -722,11 +722,11 @@ class MetricsUtilsTest(unittest.TestCase): """Tests that we can extract known subcommand args.""" result = metrics_utils.extract_known_subcommand_args([ 'm=Fix issue with ccs', 'cc=foo@example.com', 'cc=bar@example.com']) - self.assertEqual(['cc', 'cc', 'm'], sorted(result)) + self.assertEqual(['cc', 'cc', 'm'], result) result = metrics_utils.extract_known_subcommand_args([ 'm=Some title mentioning cc and hashtag', 'notify=NONE', 'private']) - self.assertEqual(['m', 'notify=NONE', 'private'], sorted(result)) + self.assertEqual(['m', 'notify=NONE', 'private'], result) result = metrics_utils.extract_known_subcommand_args([ 'foo=bar', 'another_unkwnon_arg'])