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 <ehmaldonado@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

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),
       })

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):

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'],),

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'])