From 1bd4b677f7a2ca24926f18d06cf84d9deeb04c33 Mon Sep 17 00:00:00 2001 From: Oliver Bristow Date: Fri, 23 Oct 2020 00:01:16 +0100 Subject: [PATCH] Simplify subprocess var coalescing + Improve isolation in testing --- notebooker/execute_notebook.py | 30 +++++------ notebooker/web/app.py | 10 +++- tests/conftest.py | 52 +++++++++++++++--- tests/integration/test_e2e.py | 5 +- tests/integration/test_execute_notebook.py | 10 +++- tests/integration/test_report_hunter.py | 11 ++-- tests/regression/test_execute_templates.py | 26 ++++----- tests/sanity/test_template_sanity.py | 16 +++--- tests/unit/test_app.py | 62 +++++++++++++--------- 9 files changed, 139 insertions(+), 83 deletions(-) diff --git a/notebooker/execute_notebook.py b/notebooker/execute_notebook.py index d771e89f..5ced6655 100644 --- a/notebooker/execute_notebook.py +++ b/notebooker/execute_notebook.py @@ -263,6 +263,14 @@ def _get_overrides(overrides_as_json: AnyStr, iterate_override_values_of: Option return all_overrides +def env_coupled_var(var_value: Optional[str], env_name: str) -> Optional[str]: + """Coalesce a value from the given one and environment, then update the environment.""" + if var_value is not None: + os.environ[env_name] = var_value + return var_value + return os.environ.get(env_name) + + @click.command() @click.option("--report-name", help="The name of the template to execute, relative to the template directory.") @click.option( @@ -336,24 +344,14 @@ def main( ): if report_name is None: raise ValueError("Error! Please provide a --report-name.") - mongo_db_name = mongo_db_name or os.environ.get("DATABASE_NAME", "notebooker") - mongo_host = mongo_host or os.environ.get("MONGO_HOST", "research") - mongo_user = mongo_user if mongo_user is not None else os.environ.get("MONGO_USER") - if mongo_user is None: - os.environ.pop("MONGO_USER", None) - else: - os.environ["MONGO_USER"] = mongo_user - - mongo_password = mongo_password if mongo_password is not None else os.environ.get("MONGO_PASSWORD") - if mongo_password is None: - os.environ.pop("MONGO_PASSWORD", None) - else: - os.environ["MONGO_PASSWORD"] = mongo_password # FIXME: rather insecure.. + mongo_db_name = env_coupled_var(mongo_db_name, "DATABASE_NAME") + mongo_host = env_coupled_var(mongo_host, "MONGO_HOST") + mongo_user = env_coupled_var(mongo_user, "MONGO_USER") + mongo_password = env_coupled_var(mongo_password, "MONGO_PASSWORD") + result_collection_name = env_coupled_var(result_collection_name, "RESULT_COLLECTION_NAME") + notebook_kernel_name = env_coupled_var(notebook_kernel_name, "NOTEBOOK_KERNEL_NAME") - result_collection_name = result_collection_name or os.environ.get("RESULT_COLLECTION_NAME", "NOTEBOOK_OUTPUT") - if notebook_kernel_name: - os.environ["NOTEBOOK_KERNEL_NAME"] = notebook_kernel_name report_title = report_title or report_name output_dir, template_dir, _ = initialise_base_dirs(output_dir=output_base_dir, template_dir=template_base_dir) all_overrides = _get_overrides(overrides_as_json, iterate_override_values_of) diff --git a/notebooker/web/app.py b/notebooker/web/app.py index 81b837bf..d188a83a 100644 --- a/notebooker/web/app.py +++ b/notebooker/web/app.py @@ -73,13 +73,19 @@ def setup_env_vars(): config = getattr(settings, f"{notebooker_environment}Config")() set_vars = [] logger.info("Running Notebooker with the following params:") - for attribute in (c for c in dir(config) if "__" not in c): - existing = os.getenv(attribute) + for attribute in dir(config): + if attribute.startswith("_"): + continue + existing = os.environ.get(attribute) if existing is None: os.environ[attribute] = str(getattr(config, attribute)) set_vars.append(attribute) + if "PASSWORD" not in attribute: logger.info(f"{attribute} = {os.environ[attribute]}") + else: + logger.info(f"{attribute} = *******") + return set_vars diff --git a/tests/conftest.py b/tests/conftest.py index a4680cc0..1951f17a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,12 +1,14 @@ +import os + import pytest -TEST_DB_NAME = "notebookertest" -TEST_LIB = "NB_OUTPUT" +from notebooker.utils import caching +from notebooker.web.config import settings @pytest.fixture -def bson_library(mongo_server): - return mongo_server.api[TEST_DB_NAME][TEST_LIB] +def bson_library(mongo_server, test_db_name, test_lib_name): + return mongo_server.api[test_db_name][test_lib_name] @pytest.fixture @@ -15,10 +17,44 @@ def mongo_host(mongo_server): @pytest.fixture -def test_db_name(): - return TEST_DB_NAME +def test_db_name(config): + return config.DATABASE_NAME + + +@pytest.fixture +def test_lib_name(config): + return config.RESULT_COLLECTION_NAME + +@pytest.fixture +def template_dir(workspace, monkeypatch): + monkeypatch.setenv("TEMPLATE_DIR", workspace.workspace) + return workspace.workspace + +@pytest.fixture +def output_dir(workspace, monkeypatch): + monkeypatch.setenv("OUTPUT_DIR", workspace.workspace) + return workspace.workspace + +@pytest.fixture +def config(): + return settings.DevConfig @pytest.fixture -def test_lib_name(): - return TEST_LIB +def clean_file_cache(monkeypatch, workspace): + """Set up cache encironment.""" + assert caching.cache is None + monkeypatch.setenv("CACHE_DIR", workspace.workspace) + yield + # purge the cache + caching.cache = None + + +@pytest.fixture(autouse=True) +def _unset_notebooker_environ(config): + """Remove Notebooker values from os.environ after each test.""" + yield + for attribute in dir(config): + if attribute.startswith("_"): + continue + os.environ.pop(attribute, None) diff --git a/tests/integration/test_e2e.py b/tests/integration/test_e2e.py index 28e9ecd9..da69f2e9 100644 --- a/tests/integration/test_e2e.py +++ b/tests/integration/test_e2e.py @@ -69,7 +69,10 @@ def environ(monkeypatch, mongo_host, workspace, test_db_name, test_lib_name): _setup_workspace(workspace) update = _environ(mongo_host, workspace, test_db_name, test_lib_name) for k, v in update.items(): - monkeypatch.setenv(k, v) + if v is None: + monkeypatch.delenv(k, raising=False) + else: + monkeypatch.setenv(k, v) def _check_report_output(job_id, serialiser, **kwargs): diff --git a/tests/integration/test_execute_notebook.py b/tests/integration/test_execute_notebook.py index cf71a67d..478b3907 100644 --- a/tests/integration/test_execute_notebook.py +++ b/tests/integration/test_execute_notebook.py @@ -1,6 +1,8 @@ from __future__ import unicode_literals import mock +import os + from click.testing import CliRunner from nbformat import NotebookNode from nbformat import __version__ as nbv @@ -8,6 +10,7 @@ from notebooker import execute_notebook from notebooker.constants import NotebookResultComplete from notebooker.serialization.serializers import PyMongoNotebookResultSerializer +from notebooker.web.app import setup_env_vars def mock_nb_execute(input_path, output_path, **kw): @@ -29,12 +32,17 @@ def test_main(mongo_host): exec_nb.side_effect = mock_nb_execute job_id = "ttttteeeesssstttt" runner = CliRunner() + # usually the parent process calls this and sets up the environment, then also explicitly passes + # values on the CLI + setup_env_vars() cli_result = runner.invoke( execute_notebook.main, ["--report-name", "test_report", "--mongo-host", mongo_host, "--job-id", job_id] ) assert cli_result.exit_code == 0 serializer = PyMongoNotebookResultSerializer( - mongo_host=mongo_host, database_name="notebooker", result_collection_name="NOTEBOOK_OUTPUT" + mongo_host=mongo_host, + database_name=os.environ["DATABASE_NAME"], + result_collection_name=os.environ["RESULT_COLLECTION_NAME"], ) result = serializer.get_check_result(job_id) assert isinstance(result, NotebookResultComplete), "Result is not instance of {}, it is {}".format( diff --git a/tests/integration/test_report_hunter.py b/tests/integration/test_report_hunter.py index d7ec9a9a..bcaa245a 100644 --- a/tests/integration/test_report_hunter.py +++ b/tests/integration/test_report_hunter.py @@ -8,13 +8,13 @@ from notebooker.serialization.serialization import Serializer from notebooker.serialization.serializers import PyMongoNotebookResultSerializer from notebooker.utils.caching import get_report_cache -from notebooker.utils.filesystem import initialise_base_dirs from notebooker.web.report_hunter import _report_hunter -from ..utils import setup_and_cleanup_notebooker_filesystem +@pytest.fixture(autouse=True) +def clean_file_cache(clean_file_cache): + """Set up cache encironment.""" -@setup_and_cleanup_notebooker_filesystem def test_report_hunter_with_nothing(bson_library, mongo_host, test_db_name, test_lib_name): _report_hunter( Serializer.PYMONGO.value, @@ -25,7 +25,6 @@ def test_report_hunter_with_nothing(bson_library, mongo_host, test_db_name, test ) -@setup_and_cleanup_notebooker_filesystem @freezegun.freeze_time(datetime.datetime(2018, 1, 12)) def test_report_hunter_with_one(bson_library, mongo_host, test_db_name, test_lib_name): serializer = PyMongoNotebookResultSerializer( @@ -52,9 +51,7 @@ def test_report_hunter_with_one(bson_library, mongo_host, test_db_name, test_lib assert get_report_cache(report_name, job_id) == expected -@setup_and_cleanup_notebooker_filesystem def test_report_hunter_with_status_change(bson_library, mongo_host, test_db_name, test_lib_name): - initialise_base_dirs() serializer = PyMongoNotebookResultSerializer( database_name=test_db_name, mongo_host=mongo_host, result_collection_name=test_lib_name ) @@ -101,7 +98,6 @@ def test_report_hunter_with_status_change(bson_library, mongo_host, test_db_name assert get_report_cache(report_name, job_id) == expected -@setup_and_cleanup_notebooker_filesystem @pytest.mark.parametrize( "status, time_later, should_timeout", [ @@ -169,7 +165,6 @@ def test_report_hunter_timeout( assert get_report_cache(report_name, job_id) == expected -@setup_and_cleanup_notebooker_filesystem def test_report_hunter_pending_to_done(bson_library, mongo_host, test_db_name, test_lib_name): job_id = str(uuid.uuid4()) report_name = str(uuid.uuid4()) diff --git a/tests/regression/test_execute_templates.py b/tests/regression/test_execute_templates.py index d02887b7..b9160baa 100644 --- a/tests/regression/test_execute_templates.py +++ b/tests/regression/test_execute_templates.py @@ -4,23 +4,19 @@ import pytest from notebooker.execute_notebook import _run_checks -from notebooker.utils.filesystem import _cleanup_dirs, get_output_dir, get_template_dir from ..utils import _all_templates @pytest.mark.parametrize("template_name", _all_templates()) -def test_execution_of_templates(template_name): - try: - _run_checks( - "job_id_{}".format(str(uuid.uuid4())[:6]), - datetime.datetime.now(), - template_name, - template_name, - get_output_dir(), - get_template_dir(), - {}, - generate_pdf_output=False, - ) - finally: - _cleanup_dirs() +def test_execution_of_templates(template_name, template_dir, output_dir): + _run_checks( + "job_id_{}".format(str(uuid.uuid4())[:6]), + datetime.datetime.now(), + template_name, + template_name, + output_dir, + template_dir, + {}, + generate_pdf_output=False, + ) diff --git a/tests/sanity/test_template_sanity.py b/tests/sanity/test_template_sanity.py index 999b85e2..f490a514 100644 --- a/tests/sanity/test_template_sanity.py +++ b/tests/sanity/test_template_sanity.py @@ -3,30 +3,34 @@ import pytest from notebooker.utils.conversion import generate_ipynb_from_py -from notebooker.utils.filesystem import get_template_dir from notebooker.utils.templates import _get_parameters_cell_idx, _get_preview, template_name_to_notebook_node from ..utils import _all_templates logger = getLogger("template_sanity_check") +@pytest.fixture(autouse=True) +def clean_file_cache(clean_file_cache): + pass + @pytest.mark.parametrize("template_name", _all_templates()) -def test_conversion_doesnt_fail(template_name): +def test_conversion_doesnt_fail(template_name, template_dir): # Test conversion to ipynb - this will throw if stuff goes wrong - generate_ipynb_from_py(get_template_dir(), template_name, warn_on_local=False) + generate_ipynb_from_py(template_dir, template_name, warn_on_local=False) @pytest.mark.parametrize("template_name", _all_templates()) -def test_template_has_parameters(template_name): - generate_ipynb_from_py(get_template_dir(), template_name, warn_on_local=False) +def test_template_has_parameters(template_name, template_dir): + generate_ipynb_from_py(template_dir, template_name, warn_on_local=False) nb = template_name_to_notebook_node(template_name, warn_on_local=False) metadata_idx = _get_parameters_cell_idx(nb) assert metadata_idx is not None, 'Template {} does not have a "parameters"-tagged cell.'.format(template_name) @pytest.mark.parametrize("template_name", _all_templates()) -def test_template_can_generate_preview(template_name): +def test_template_can_generate_preview(template_dir, template_name): + print(template_name) preview = _get_preview(template_name, warn_on_local=False) # Previews in HTML are gigantic since they include all jupyter css and js. assert len(preview) > 1000, "Preview was not properly generated for {}".format(template_name) diff --git a/tests/unit/test_app.py b/tests/unit/test_app.py index 7a35d92b..c2bf3428 100644 --- a/tests/unit/test_app.py +++ b/tests/unit/test_app.py @@ -1,37 +1,47 @@ import os +import pytest + from notebooker.web.app import setup_env_vars +from notebooker.web.config import settings -def test_setup_env_vars(): - set_vars = setup_env_vars() - try: - assert os.getenv("PORT") == "11828" - assert os.getenv("MONGO_HOST") == "localhost" - finally: - for var in set_vars: - del os.environ[var] +@pytest.fixture +def dev_config(): + return settings.DevConfig -def test_setup_env_vars_override_default(): - os.environ["MONGO_HOST"] = "override" - set_vars = setup_env_vars() - try: - assert os.getenv("PORT") == "11828" - assert os.getenv("MONGO_HOST") == "override" - finally: - for var in set_vars: - del os.environ[var] - del os.environ["MONGO_HOST"] +@pytest.fixture +def prod_config(): + return settings.ProdConfig + +def safe_setup_env_vars(): + """Return a copy of the environment after running setup_env_vars.""" + original_env = os.environ.copy() -def test_setup_env_vars_prod(): - os.environ["NOTEBOOKER_ENVIRONMENT"] = "Prod" - set_vars = setup_env_vars() try: - assert os.getenv("PORT") == "11828" - assert os.getenv("MONGO_HOST") == "a-production-mongo-cluster" + setup_env_vars() + return os.environ.copy() finally: - for var in set_vars: - del os.environ[var] - del os.environ["NOTEBOOKER_ENVIRONMENT"] + os.environ.clear() + os.environ.update(original_env) + +def test_setup_env_vars(dev_config): + env = safe_setup_env_vars() + assert env["PORT"] == str(dev_config.PORT) + assert env["MONGO_HOST"] == str(dev_config.MONGO_HOST) + + +def test_setup_env_vars_override_default(monkeypatch, dev_config): + monkeypatch.setenv("MONGO_HOST","override") + env = safe_setup_env_vars() + assert env["PORT"] == str(dev_config.PORT) + assert env["MONGO_HOST"] == "override" + + +def test_setup_env_vars_prod(monkeypatch, prod_config): + monkeypatch.setenv("NOTEBOOKER_ENVIRONMENT", "Prod") + env = safe_setup_env_vars() + assert env["PORT"] == str(prod_config.PORT) + assert env["MONGO_HOST"] == str(prod_config.MONGO_HOST)