From 91adadfed094ff8ecc0be547f460b2cc56f663c2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Jan 2020 11:54:10 +0000 Subject: [PATCH 1/2] dont reassign os.environ os.environ is an `environ` object, not a dict. by only interacting with it through builtin functions we can ensure it remains properly accessible to third party libraries which might interact with it in different ways. See https://github.com/alphagov/notifications-api/commit/d2441466 for more detail --- tests/app/test_cloudfoundry_config.py | 7 +++---- tests/app/test_config.py | 15 +++++++++------ tests/conftest.py | 5 +++-- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index dfd2ccdf7..d03aa29d4 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -6,12 +6,11 @@ from app.cloudfoundry_config import extract_cloudfoundry_config @pytest.fixture -def cloudfoundry_environ(monkeypatch): - monkeypatch.setenv('VCAP_APPLICATION', '{"space_name":"🚀🌌"}') +def cloudfoundry_environ(os_environ): + os.environ['VCAP_APPLICATION'] = '{"space_name":"🚀🌌"}' -@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_extract_cloudfoundry_config_populates_other_vars(): +def test_extract_cloudfoundry_config_populates_other_vars(cloudfoundry_environ): extract_cloudfoundry_config() assert os.environ['NOTIFY_ENVIRONMENT'] == '🚀🌌' diff --git a/tests/app/test_config.py b/tests/app/test_config.py index 4d0c7387d..166e5d277 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -12,21 +12,24 @@ def cf_conf(): @pytest.fixture -def reload_config(): +def reload_config(os_environ): """ Reset config, by simply re-running config.py from a fresh environment """ old_env = os.environ.copy() + os.environ.clear() yield - os.environ = old_env + os.environ.clear() + for k, v in old_env.items(): + os.environ[k] = v importlib.reload(config) -def test_load_cloudfoundry_config_if_available(monkeypatch, reload_config): +def test_load_cloudfoundry_config_if_available(reload_config): os.environ['API_HOST_NAME'] = 'env' - monkeypatch.setenv('VCAP_APPLICATION', 'some json blob') + os.environ['VCAP_APPLICATION'] = 'some json blob' with mock.patch('app.cloudfoundry_config.extract_cloudfoundry_config', side_effect=cf_conf) as cf_config: # reload config so that its module level code (ie: all of it) is re-instantiated @@ -38,10 +41,10 @@ def test_load_cloudfoundry_config_if_available(monkeypatch, reload_config): assert config.Config.API_HOST_NAME == 'cf' -def test_load_config_if_cloudfoundry_not_available(monkeypatch, reload_config): +def test_load_config_if_cloudfoundry_not_available(reload_config): os.environ['API_HOST_NAME'] = 'env' - monkeypatch.delenv('VCAP_APPLICATION', raising=False) + os.environ.pop('VCAP_APPLICATION', None) with mock.patch('app.cloudfoundry_config.extract_cloudfoundry_config') as cf_config: # reload config so that its module level code (ie: all of it) is re-instantiated diff --git a/tests/conftest.py b/tests/conftest.py index c5e4b371f..b78bb5ce6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2827,9 +2827,10 @@ def os_environ(): """ # for use whenever you expect code to edit environment variables old_env = os.environ.copy() - os.environ = {} + os.environ.clear() yield - os.environ = old_env + for k, v in old_env.items(): + os.environ[k] = v @pytest.fixture From e5b2d81d227c0b4aa61e72c55a1aa0845493f054 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Jan 2020 11:57:38 +0000 Subject: [PATCH 2/2] increase reply to address validation timeout on preview Celery/SQS underperforms in low-traffic environments. Tasks will sit on celery queues for several seconds before getting picked up if they're the only thing on the queue. This is observable in our test environments like preview and staging, but we've got enough load on production that this isn't an issue. When we validate reply to email addresses, we expect a delivery receipt to have been processed within 45 seconds of the button being pressed. On preview, we often observe times over that, possibly due to the several queues involved in sending an email and processing its receipt. So, to ensure that functional tests can pass (when we don't really care how fast things are, just that the flow doesn't break), bump this timeout up to 120 seconds on preview. The functional tests were waiting for 120 seconds for the reply to address to be validated anyway. --- app/config.py | 5 +++++ app/main/views/service_settings.py | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index ad8951baf..cd694be4b 100644 --- a/app/config.py +++ b/app/config.py @@ -72,6 +72,8 @@ class Config(object): ACTIVITY_STATS_LIMIT_DAYS = 7 TEST_MESSAGE_FILENAME = 'Report' + REPLY_TO_EMAIL_ADDRESS_VALIDATION_TIMEOUT = 45 + NOTIFY_ENVIRONMENT = 'development' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-local' MOU_BUCKET_NAME = 'local-mou' @@ -147,6 +149,9 @@ class Preview(Config): ASSET_DOMAIN = 'static.notify.works' ASSET_PATH = 'https://static.notify.works/' + # On preview, extend the validation timeout to allow more leniency when running functional tests + REPLY_TO_EMAIL_ADDRESS_VALIDATION_TIMEOUT = 120 + class Staging(Config): SHOW_STYLEGUIDE = False diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 8596a431c..79447ebe5 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -478,7 +478,8 @@ def get_service_verify_reply_to_address_partials(service_id, notification_id): created_at_no_tz = notification["created_at"][:-6] seconds_since_sending = (datetime.utcnow() - datetime.strptime(created_at_no_tz, '%Y-%m-%dT%H:%M:%S.%f')).seconds if notification["status"] in FAILURE_STATUSES or ( - notification["status"] in SENDING_STATUSES and seconds_since_sending > 45 + notification["status"] in SENDING_STATUSES and + seconds_since_sending > current_app.config['REPLY_TO_EMAIL_ADDRESS_VALIDATION_TIMEOUT'] ): verification_status = "failure" form.email_address.data = notification['to']