From 8dd95bd0577ba4f1ccfef439d01b96838670f8f2 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Wed, 22 Jan 2020 17:41:49 +0100 Subject: [PATCH] tests: Replace httpretty with responses. responses is an module analogous to httpretty for mocking external URLs, with a very similar interface (potentially cleaner in that it makes use of context managers). The most important (in the moment) problem with httpretty is that it breaks the ability to use redis in parts of code where httpretty is enabled. From more research, the module in general has tendency to have various troublesome bugs with breaking URLs that it shouldn't be affecting, caused by it working at the socket interface layer. While those issues could be fixed, responses seems to be less buggy (based on both third-party reports like ckan/ckan#4755 and our own experience in removing workarounds for bugs in httpretty) and is more actively maintained. --- requirements/dev.in | 2 +- requirements/dev.txt | 5 +- tools/test-backend | 60 ++++----- version.py | 2 +- zerver/tests/test_auth_backends.py | 205 ++++++++++++----------------- zerver/tests/test_internet.py | 20 +++ 6 files changed, 133 insertions(+), 161 deletions(-) create mode 100644 zerver/tests/test_internet.py diff --git a/requirements/dev.in b/requirements/dev.in index f3e234f0aa..63b545d687 100644 --- a/requirements/dev.in +++ b/requirements/dev.in @@ -20,7 +20,7 @@ coverage https://github.com/zulip/fakeldap/archive/ff32deaad34b91d5ba5735f314362c0456c92607.zip#egg=fakeldap==0.6.1zulip # For testing mock http requests -httpretty +responses # For sorting imports isort diff --git a/requirements/dev.txt b/requirements/dev.txt index bc80d35ba6..1f143a0b17 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -307,8 +307,6 @@ html2text==2019.9.26 \ httplib2==0.14.0 \ --hash=sha256:34537dcdd5e0f2386d29e0e2c6d4a1703a3b982d34c198a5102e6e5d6194b107 \ --hash=sha256:409fa5509298f739b34d5a652df762cb0042507dc93f6633e306b11289d6249d -httpretty==0.9.7 \ - --hash=sha256:66216f26b9d2c52e81808f3e674a6fb65d4bf719721394a1a9be926177e55fbe hypchat==0.21 \ --hash=sha256:ef37a9cd8103bb13ad772b28ba9223ca9d4278371e374450c3ea2918df70a8e9 hyper==0.7.0 \ @@ -722,8 +720,7 @@ requests[security]==2.22.0 \ # via docker, hypchat, matrix-client, moto, premailer, pyoembed, python-digitalocean, python-gcm, python-twitter, requests-oauthlib, responses, social-auth-core, sphinx, stripe, transifex-client, twilio responses==0.10.7 \ --hash=sha256:46d4e546a19fc6106bc7e804edd4551ef04690405e41e7e750ebc295d042623b \ - --hash=sha256:93b1e0f2f960c0f3304ca4436856241d64c33683ef441431b9caf1d05d9d9e23 \ - # via moto + --hash=sha256:93b1e0f2f960c0f3304ca4436856241d64c33683ef441431b9caf1d05d9d9e23 rsa==4.0 \ --hash=sha256:14ba45700ff1ec9eeb206a2ce76b32814958a98e372006c8fb76ba820211be66 \ --hash=sha256:1a836406405730121ae9823e19c6e806c62bbad73f890574fff50efa4122c487 \ diff --git a/tools/test-backend b/tools/test-backend index e864f10432..14ffeda914 100755 --- a/tools/test-backend +++ b/tools/test-backend @@ -4,18 +4,18 @@ from lib import sanity_check sanity_check.check_venv(__file__) -from typing import List, Any +from typing import List, Iterator import glob import argparse +import contextlib +import mock import os import shlex import sys import subprocess import tempfile import ujson -import httplib2 -import httpretty -import requests +import responses import django from django.conf import settings @@ -154,32 +154,27 @@ def write_failed_tests(failed_tests: List[str]) -> None: with open(FAILED_TEST_PATH, 'w') as f: ujson.dump(failed_tests, f) -def block_internet() -> None: - # We are blocking internet currently by assuming mostly any test would use - # httplib2 to access internet. - requests_orig = requests.request +@contextlib.contextmanager +def block_internet() -> Iterator[None]: + # Monkey-patching - responses library raises requests.ConnectionError when access to an unregistered URL + # is attempted. We want to replace that with our own exception, so that it propagates all the way: + with mock.patch.object(responses, 'ConnectionError', new=ZulipInternetBlockedError): + # We'll run all tests in this context manager. It'll cause an error to be raised (see above comment), + # if any code attempts to access the internet. + with responses.RequestsMock(): + yield - def internet_guard_requests(*args: Any, **kwargs: Any) -> Any: - if httpretty.is_enabled(): - return requests_orig(*args, **kwargs) - raise Exception("Outgoing network requests are not allowed in the Zulip tests. " - "More details and advice are available here:" - "https://zulip.readthedocs.io/en/latest/testing/testing.html#internet-access-inside-test-suites") - - requests.request = internet_guard_requests - - http2lib_request_orig = httplib2.Http.request - - def internet_guard_httplib2(*args: Any, **kwargs: Any) -> Any: - if httpretty.is_enabled(): - return http2lib_request_orig(*args, **kwargs) - raise Exception("Outgoing network requests are not allowed in the Zulip tests. " - "More details and advice are available here:" - "https://zulip.readthedocs.io/en/latest/testing/testing.html#internet-access-inside-test-suites") - httplib2.Http.request = internet_guard_httplib2 +class ZulipInternetBlockedError(Exception): + def __init__(self, original_msg: str) -> None: + zulip_msg = ( + "Outgoing network requests are not allowed in the Zulip tests. " + "More details and advice are available here:" + "https://zulip.readthedocs.io/en/latest/testing/testing.html#internet-access-inside-test-suites" + ) + msg = "{}\nResponses library error message: {}".format(zulip_msg, original_msg) + super().__init__(msg) def main() -> None: - block_internet() TOOLS_DIR = os.path.dirname(os.path.abspath(__file__)) os.chdir(os.path.dirname(TOOLS_DIR)) sys.path.insert(0, os.path.dirname(TOOLS_DIR)) @@ -381,11 +376,12 @@ def main() -> None: else: print("-- Running tests in serial mode.", flush=True) - test_runner = TestRunner(failfast=options.fatal_errors, verbosity=2, - parallel=parallel, reverse=options.reverse, - keepdb=True) - failures, failed_tests = test_runner.run_tests(suites, full_suite=full_suite, - include_webhooks=include_webhooks) + with block_internet(): + test_runner = TestRunner(failfast=options.fatal_errors, verbosity=2, + parallel=parallel, reverse=options.reverse, + keepdb=True) + failures, failed_tests = test_runner.run_tests(suites, full_suite=full_suite, + include_webhooks=include_webhooks) write_failed_tests(failed_tests) templates_not_rendered = test_runner.get_shallow_tested_templates() diff --git a/version.py b/version.py index f7a2ca775d..c48057853a 100644 --- a/version.py +++ b/version.py @@ -26,4 +26,4 @@ LATEST_RELEASE_ANNOUNCEMENT = "https://blog.zulip.org/2019/12/13/zulip-2-1-relea # historical commits sharing the same major version, in which case a # minor version bump suffices. -PROVISION_VERSION = '67.0' +PROVISION_VERSION = '67.1' diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index b2c3d52b80..e6eacb6106 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -11,7 +11,7 @@ from typing import Any, Callable, Dict, List, Optional, Tuple from django.core import signing from django.urls import reverse -import httpretty +import responses import ldap import jwt @@ -384,38 +384,15 @@ class AuthBackendTest(ZulipTestCase): backends_to_test = { 'google': { 'urls': [ - { - 'url': "https://accounts.google.com/o/oauth2/token", - 'method': httpretty.POST, - 'status': 200, - 'body': json.dumps(token_data_dict), - }, - { - 'url': "https://www.googleapis.com/oauth2/v3/userinfo", - 'method': httpretty.GET, - 'status': 200, - 'body': json.dumps(google_email_data), - }, + # The limited process that we test here doesn't require mocking any urls. ], 'backend': GoogleAuthBackend, }, 'github': { 'urls': [ - { - 'url': "https://github.com/login/oauth/access_token", - 'method': httpretty.POST, - 'status': 200, - 'body': json.dumps(token_data_dict), - }, - { - 'url': "https://api.github.com/user", - 'method': httpretty.GET, - 'status': 200, - 'body': json.dumps(dict(email=user.email, name=user.full_name)), - }, { 'url': "https://api.github.com/user/emails", - 'method': httpretty.GET, + 'method': responses.GET, 'status': 200, 'body': json.dumps(github_email_data), }, @@ -448,42 +425,40 @@ class AuthBackendTest(ZulipTestCase): return google_email_data['email'] for backend_name in backends_to_test: - httpretty.enable(allow_net_connect=False) - urls = backends_to_test[backend_name]['urls'] # type: List[Dict[str, Any]] - for details in urls: - httpretty.register_uri( - details['method'], - details['url'], - status=details['status'], - body=details['body']) - backend_class = backends_to_test[backend_name]['backend'] - backend = backend_class() - backend.strategy = DjangoStrategy(storage=BaseDjangoStorage()) + with responses.RequestsMock(assert_all_requests_are_fired=True) as requests_mock: + urls = backends_to_test[backend_name]['urls'] # type: List[Dict[str, Any]] + for details in urls: + requests_mock.add( + details['method'], + details['url'], + status=details['status'], + body=details['body']) + backend_class = backends_to_test[backend_name]['backend'] + backend = backend_class() + backend.strategy = DjangoStrategy(storage=BaseDjangoStorage()) - orig_authenticate = backend_class.authenticate - backend.authenticate = patched_authenticate - orig_get_verified_emails = backend_class.get_verified_emails - if backend_name == "google": - backend.get_verified_emails = patched_get_verified_emails + orig_authenticate = backend_class.authenticate + backend.authenticate = patched_authenticate + orig_get_verified_emails = backend_class.get_verified_emails + if backend_name == "google": + backend.get_verified_emails = patched_get_verified_emails - good_kwargs = dict(backend=backend, strategy=backend.strategy, - storage=backend.strategy.storage, - response=token_data_dict, - subdomain='zulip') - bad_kwargs = dict(subdomain='acme') - with mock.patch('zerver.views.auth.redirect_and_log_into_subdomain', - return_value=user): - self.verify_backend(backend, - good_kwargs=good_kwargs, - bad_kwargs=bad_kwargs) - bad_kwargs['subdomain'] = "zephyr" - self.verify_backend(backend, - good_kwargs=good_kwargs, - bad_kwargs=bad_kwargs) - backend.authenticate = orig_authenticate - backend.get_verified_emails = orig_get_verified_emails - httpretty.disable() - httpretty.reset() + good_kwargs = dict(backend=backend, strategy=backend.strategy, + storage=backend.strategy.storage, + response=token_data_dict, + subdomain='zulip') + bad_kwargs = dict(subdomain='acme') + with mock.patch('zerver.views.auth.redirect_and_log_into_subdomain', + return_value=user): + self.verify_backend(backend, + good_kwargs=good_kwargs, + bad_kwargs=bad_kwargs) + bad_kwargs['subdomain'] = "zephyr" + self.verify_backend(backend, + good_kwargs=good_kwargs, + bad_kwargs=bad_kwargs) + backend.authenticate = orig_authenticate + backend.get_verified_emails = orig_get_verified_emails class CheckPasswordStrengthTest(ZulipTestCase): def test_check_password_strength(self) -> None: @@ -530,7 +505,7 @@ class SocialAuthBase(ZulipTestCase): from social_core.backends.utils import load_backends load_backends(settings.AUTHENTICATION_BACKENDS, force_load=True) - def register_extra_endpoints(self, + def register_extra_endpoints(self, requests_mock: responses.RequestsMock, account_data_dict: Dict[str, str], **extra_data: Any) -> None: pass @@ -602,66 +577,50 @@ class SocialAuthBase(ZulipTestCase): # We register callbacks for the key URLs on Identity Provider that # auth completion url will call - httpretty.enable(allow_net_connect=False) - httpretty.register_uri( - httpretty.POST, - self.ACCESS_TOKEN_URL, - match_querystring=False, - status=200, - body=json.dumps(token_data_dict)) - httpretty.register_uri( - httpretty.GET, - self.USER_INFO_URL, - status=200, - body=json.dumps(account_data_dict) - ) - self.register_extra_endpoints(account_data_dict, **extra_data) - - parsed_url = urllib.parse.urlparse(result.url) - csrf_state = urllib.parse.parse_qs(parsed_url.query)['state'] - result = self.client_get(self.AUTH_FINISH_URL, - dict(state=csrf_state), **headers) - - if expect_choose_email_screen and result.status_code == 200: - # For authentication backends such as GitHub that - # successfully authenticate multiple email addresses, - # we'll have an additional screen where the user selects - # which email address to login using (this screen is a - # "partial" state of the python-social-auth pipeline). - # - # TODO: Generalize this testing code for use with other - # authentication backends; for now, we just assert that - # it's definitely the GitHub authentication backend. - self.assert_in_success_response(["Select account"], result) - assert self.AUTH_FINISH_URL == "/complete/github/" - - # Testing hack: When the pipeline goes to the partial - # step, the below given URL is called again in the same - # test. If the below URL is not registered again as done - # below, the URL returns emails from previous tests. e.g - # email = 'invalid' may be one of the emails in the list - # in a test function followed by it. This is probably a - # bug in httpretty. - httpretty.disable() - httpretty.enable() - httpretty.register_uri( - httpretty.GET, - "https://api.github.com/user/emails", + with responses.RequestsMock(assert_all_requests_are_fired=False) as requests_mock: + requests_mock.add( + requests_mock.POST, + self.ACCESS_TOKEN_URL, + match_querystring=False, status=200, - body=json.dumps(self.email_data) + body=json.dumps(token_data_dict)) + requests_mock.add( + requests_mock.GET, + self.USER_INFO_URL, + status=200, + body=json.dumps(account_data_dict) ) - result = self.client_get(self.AUTH_FINISH_URL, - dict(state=csrf_state, email=account_data_dict['email']), **headers) - elif self.AUTH_FINISH_URL == "/complete/github/": - # We want to be explicit about when we expect a test to - # use the "choose email" screen, but of course we should - # only check for that screen with the GitHub backend, - # because this test code is shared with other - # authentication backends that structurally will never use - # that screen. - assert not expect_choose_email_screen + self.register_extra_endpoints(requests_mock, account_data_dict, **extra_data) + + parsed_url = urllib.parse.urlparse(result.url) + csrf_state = urllib.parse.parse_qs(parsed_url.query)['state'] + result = self.client_get(self.AUTH_FINISH_URL, + dict(state=csrf_state), **headers) + + if expect_choose_email_screen and result.status_code == 200: + # For authentication backends such as GitHub that + # successfully authenticate multiple email addresses, + # we'll have an additional screen where the user selects + # which email address to login using (this screen is a + # "partial" state of the python-social-auth pipeline). + # + # TODO: Generalize this testing code for use with other + # authentication backends; for now, we just assert that + # it's definitely the GitHub authentication backend. + self.assert_in_success_response(["Select account"], result) + assert self.AUTH_FINISH_URL == "/complete/github/" + + result = self.client_get(self.AUTH_FINISH_URL, + dict(state=csrf_state, email=account_data_dict['email']), **headers) + elif self.AUTH_FINISH_URL == "/complete/github/": + # We want to be explicit about when we expect a test to + # use the "choose email" screen, but of course we should + # only check for that screen with the GitHub backend, + # because this test code is shared with other + # authentication backends that structurally will never use + # that screen. + assert not expect_choose_email_screen - httpretty.disable() return result def test_social_auth_no_key(self) -> None: @@ -1340,7 +1299,7 @@ class GitHubAuthBackendTest(SocialAuthBase): AUTH_FINISH_URL = "/complete/github/" CONFIG_ERROR_URL = "/config-error/github" - def register_extra_endpoints(self, + def register_extra_endpoints(self, requests_mock: responses.RequestsMock, account_data_dict: Dict[str, str], **extra_data: Any) -> None: # Keeping a verified email before the primary email makes sure @@ -1358,15 +1317,15 @@ class GitHubAuthBackendTest(SocialAuthBase): ] email_data = extra_data.get("email_data", email_data) - httpretty.register_uri( - httpretty.GET, + requests_mock.add( + requests_mock.GET, "https://api.github.com/user/emails", status=200, body=json.dumps(email_data) ) - httpretty.register_uri( - httpretty.GET, + requests_mock.add( + requests_mock.GET, "https://api.github.com/teams/zulip-webapp/members/None", status=200, body=json.dumps(email_data) diff --git a/zerver/tests/test_internet.py b/zerver/tests/test_internet.py new file mode 100644 index 0000000000..0fee3306e5 --- /dev/null +++ b/zerver/tests/test_internet.py @@ -0,0 +1,20 @@ +from zerver.lib.test_classes import ZulipTestCase + +import responses +import requests + +class ResponsesTest(ZulipTestCase): + def test_responses(self) -> None: + # With our test setup, accessing the internet should be blocked. + with self.assertRaises(Exception): + result = requests.request('GET', 'https://www.google.com') + + # A test can invoke its own responses.RequestsMock context manager + # and register URLs to mock, accessible from within the context. + with responses.RequestsMock() as requests_mock: + requests_mock.add(responses.GET, 'https://www.google.com', + body='{}', status=200, + content_type='application/json') + result = requests.request('GET', 'https://www.google.com') + self.assertEqual(result.status_code, 200) + self.assertEqual(result.text, '{}')