gerrit_util: Refactor ReadHttpResponse and add more tests.

Bug: 1016601
Change-Id: Ie6afc5b1ea29888b0bf40bdb39b2b492d2d0494c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1880014Reviewed-by: Anthony Polito <apolito@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

.vpython3

 python_version: "3.8"
+
+# Used by:
+#   auth.py
+#   gerrit_util.py
+#   git_cl.py
+#   my_activity.py
+#   TODO(crbug.com/1002153): Add ninjalog_uploader.py
+wheel: <
+  name: "infra/python/wheels/httplib2-py3"
+  version: "version:0.13.1"
+>
+
+# Used by:
+#   my_activity.py
+wheel: <
+  name: "infra/python/wheels/python-dateutil-py2_py3"
+  version: "version:2.7.3"
+>
+wheel: <
+  name: "infra/python/wheels/six-py2_py3"
+  version: "version:1.10.0"
+>

tests/gerrit_util_test.py

-#!/usr/bin/env vpython
+#!/usr/bin/env vpython3
 # coding=utf-8
 # Copyright (c) 2019 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
@@ -9,6 +9,7 @@ from __future__ import unicode_literals
 
 
 import base64
+import json
 import os
 import sys
 import unittest
@@ -19,8 +20,15 @@ from third_party import mock
 
 import gerrit_util
 import gclient_utils
+import metrics
+import metrics_utils
 import subprocess2
 
+if sys.version_info.major == 2:
+  from cStringIO import StringIO
+else:
+  from io import StringIO
+
 
 class CookiesAuthenticatorTest(unittest.TestCase):
   _GITCOOKIES = '\n'.join([
@@ -167,5 +175,214 @@ class CookiesAuthenticatorTest(unittest.TestCase):
     self.assertIsNone(auth.get_auth_email('some-review.example.com'))
 
 
+class GerritUtilTest(unittest.TestCase):
+  def setUp(self):
+    super(GerritUtilTest, self).setUp()
+    mock.patch('gerrit_util.LOGGER').start()
+    mock.patch('gerrit_util.time_sleep').start()
+    mock.patch('metrics.collector').start()
+    mock.patch(
+        'metrics_utils.extract_http_metrics',
+        return_value='http_metrics').start()
+    self.addCleanup(mock.patch.stopall)
+
+  def testQueryString(self):
+    self.assertEqual('', gerrit_util._QueryString([]))
+    self.assertEqual(
+        'first%20param%2B', gerrit_util._QueryString([], 'first param+'))
+    self.assertEqual(
+        'key:val+foo:bar',
+         gerrit_util._QueryString([('key', 'val'), ('foo', 'bar')]))
+    self.assertEqual(
+        'first%20param%2B+key:val+foo:bar',
+         gerrit_util._QueryString(
+            [('key', 'val'), ('foo', 'bar')], 'first param+'))
+
+  @mock.patch('gerrit_util.Authenticator')
+  def testCreateHttpConn_Basic(self, mockAuth):
+    mockAuth.get().get_auth_header.return_value = None
+    conn = gerrit_util.CreateHttpConn('host.example.com', 'foo/bar')
+    self.assertEqual('host.example.com', conn.req_host)
+    self.assertEqual({
+        'uri': 'https://host.example.com/foo/bar',
+        'method': 'GET',
+        'headers': {},
+        'body': None,
+    }, conn.req_params)
+
+  @mock.patch('gerrit_util.Authenticator')
+  def testCreateHttpConn_Authenticated(self, mockAuth):
+    mockAuth.get().get_auth_header.return_value = 'Bearer token'
+    conn = gerrit_util.CreateHttpConn(
+        'host.example.com', 'foo/bar', headers={'header': 'value'})
+    self.assertEqual('host.example.com', conn.req_host)
+    self.assertEqual({
+        'uri': 'https://host.example.com/a/foo/bar',
+        'method': 'GET',
+        'headers': {'Authorization': 'Bearer token', 'header': 'value'},
+        'body': None,
+    }, conn.req_params)
+
+  @mock.patch('gerrit_util.Authenticator')
+  def testCreateHttpConn_Body(self, mockAuth):
+    mockAuth.get().get_auth_header.return_value = None
+    conn = gerrit_util.CreateHttpConn(
+        'host.example.com', 'foo/bar', body={'l': [1, 2, 3], 'd': {'k': 'v'}})
+    self.assertEqual('host.example.com', conn.req_host)
+    self.assertEqual({
+        'uri': 'https://host.example.com/foo/bar',
+        'method': 'GET',
+        'headers': {'Content-Type': 'application/json'},
+        'body': '{"d": {"k": "v"}, "l": [1, 2, 3]}',
+    }, conn.req_params)
+
+  def testReadHttpResponse_200(self):
+    conn = mock.Mock()
+    conn.req_params = {'uri': 'uri', 'method': 'method'}
+    conn.request.return_value = (mock.Mock(status=200), b'content')
+
+    content = gerrit_util.ReadHttpResponse(conn)
+    self.assertEqual('content', content.getvalue())
+    metrics.collector.add_repeated.assert_called_once_with(
+        'http_requests', 'http_metrics')
+
+  def testReadHttpResponse_AuthenticationIssue(self):
+    for status in (302, 401, 403):
+      response = mock.Mock(status=status)
+      response.get.return_value = None
+      conn = mock.Mock(req_params={'uri': 'uri', 'method': 'method'})
+      conn.request.return_value = (response, b'')
+
+      with mock.patch('sys.stdout', StringIO()):
+        with self.assertRaises(gerrit_util.GerritError) as cm:
+          gerrit_util.ReadHttpResponse(conn)
+
+        self.assertEqual(status, cm.exception.http_status)
+        self.assertIn(
+            'Your Gerrit credentials might be misconfigured',
+            sys.stdout.getvalue())
+
+  def testReadHttpResponse_ClientError(self):
+    conn = mock.Mock(req_params={'uri': 'uri', 'method': 'method'})
+    conn.request.return_value = (mock.Mock(status=404), b'')
+
+    with self.assertRaises(gerrit_util.GerritError) as cm:
+      gerrit_util.ReadHttpResponse(conn)
+
+    self.assertEqual(404, cm.exception.http_status)
+
+  def testReadHttpResponse_ServerError(self):
+    conn = mock.Mock(req_params={'uri': 'uri', 'method': 'method'})
+    conn.request.return_value = (mock.Mock(status=500), b'')
+
+    with self.assertRaises(gerrit_util.GerritError) as cm:
+      gerrit_util.ReadHttpResponse(conn)
+
+    self.assertEqual(500, cm.exception.http_status)
+    self.assertEqual(gerrit_util.TRY_LIMIT, len(conn.request.mock_calls))
+    self.assertEqual(
+        [mock.call(1.5), mock.call(3)], gerrit_util.time_sleep.mock_calls)
+
+  def testReadHttpResponse_ServerErrorAndSuccess(self):
+    conn = mock.Mock(req_params={'uri': 'uri', 'method': 'method'})
+    conn.request.side_effect = [
+        (mock.Mock(status=500), b''),
+        (mock.Mock(status=200), b'content'),
+    ]
+
+    self.assertEqual('content', gerrit_util.ReadHttpResponse(conn).getvalue())
+    self.assertEqual(2, len(conn.request.mock_calls))
+    gerrit_util.time_sleep.assert_called_once_with(1.5)
+
+  def testReadHttpResponse_Expected404(self):
+    conn = mock.Mock()
+    conn.req_params = {'uri': 'uri', 'method': 'method'}
+    conn.request.return_value = (mock.Mock(status=404), b'content')
+
+    content = gerrit_util.ReadHttpResponse(conn, (404,))
+    self.assertEqual('', content.getvalue())
+
+  @mock.patch('gerrit_util.ReadHttpResponse')
+  def testReadHttpJsonResponse_NotJSON(self, mockReadHttpResponse):
+    mockReadHttpResponse.return_value = StringIO('not json')
+    with self.assertRaises(gerrit_util.GerritError) as cm:
+      gerrit_util.ReadHttpJsonResponse(None)
+    self.assertEqual(cm.exception.http_status, 200)
+    self.assertEqual(
+        cm.exception.message, '(200) Unexpected json output: not json')
+
+  @mock.patch('gerrit_util.ReadHttpResponse')
+  def testReadHttpJsonResponse_EmptyValue(self, mockReadHttpResponse):
+    mockReadHttpResponse.return_value = StringIO(')]}\'')
+    self.assertIsNone(gerrit_util.ReadHttpJsonResponse(None))
+
+  @mock.patch('gerrit_util.ReadHttpResponse')
+  def testReadHttpJsonResponse_JSON(self, mockReadHttpResponse):
+    expected_value = {'foo': 'bar', 'baz': [1, '2', 3]}
+    mockReadHttpResponse.return_value = StringIO(
+        ')]}\'\n' + json.dumps(expected_value))
+    self.assertEqual(expected_value, gerrit_util.ReadHttpJsonResponse(None))
+
+  @mock.patch('gerrit_util.CreateHttpConn')
+  @mock.patch('gerrit_util.ReadHttpJsonResponse')
+  def testQueryChanges(self, mockJsonResponse, mockCreateHttpConn):
+    gerrit_util.QueryChanges(
+        'host', [('key', 'val'), ('foo', 'bar')], 'first param', limit=500,
+        o_params=['PARAM_A', 'PARAM_B'], start='start')
+    mockCreateHttpConn.assert_called_once_with(
+        'host',
+        ('changes/?q=first%20param+key:val+foo:bar'
+         '&start=start'
+         '&n=500'
+         '&o=PARAM_A'
+         '&o=PARAM_B'))
+
+  def testQueryChanges_NoParams(self):
+    self.assertRaises(RuntimeError, gerrit_util.QueryChanges, 'host', [])
+
+  @mock.patch('gerrit_util.QueryChanges')
+  def testGenerateAllChanges(self, mockQueryChanges):
+    mockQueryChanges.side_effect = [
+        # First results page
+        [
+            {'_number': '4'},
+            {'_number': '3'},
+            {'_number': '2', '_more_changes': True},
+        ],
+        # Second results page, there are new changes, so second page includes
+        # some results from the first page.
+        [
+            {'_number': '2'},
+            {'_number': '1'},
+        ],
+        # GenerateAllChanges queries again from the start to get any new
+        # changes (5 in this case).
+        [
+            {'_number': '5'},
+            {'_number': '4'},
+            {'_number': '3', '_more_changes': True},
+
+        ],
+    ]
+
+    changes = list(gerrit_util.GenerateAllChanges('host', 'params'))
+    self.assertEqual(
+        [
+            {'_number': '4'},
+            {'_number': '3'},
+            {'_number': '2', '_more_changes': True},
+            {'_number': '1'},
+            {'_number': '5'},
+        ],
+        changes)
+    self.assertEqual(
+        [
+            mock.call('host', 'params', None, 500, None, 0),
+            mock.call('host', 'params', None, 500, None, 3),
+            mock.call('host', 'params', None, 500, None, 0),
+        ],
+        mockQueryChanges.mock_calls)
+
+
 if __name__ == '__main__':
   unittest.main()