From d4c6da52e1a73ce3cb44b68f90e9538250d9ab80 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 14:09:03 +0200 Subject: [PATCH 01/24] maint: let installer ensure /etc/sudoers.d directory exists --- integration-tests/Dockerfile | 2 -- tljh/installer.py | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/integration-tests/Dockerfile b/integration-tests/Dockerfile index 22cd2d4..c1c73d8 100644 --- a/integration-tests/Dockerfile +++ b/integration-tests/Dockerfile @@ -24,8 +24,6 @@ RUN find /etc/systemd/system \ -not -name '*systemd-user-sessions*' \ -exec rm \{} \; -RUN mkdir -p /etc/sudoers.d - RUN systemctl set-default multi-user.target STOPSIGNAL SIGRTMIN+3 diff --git a/tljh/installer.py b/tljh/installer.py index 668f30c..33cce12 100644 --- a/tljh/installer.py +++ b/tljh/installer.py @@ -126,6 +126,7 @@ def ensure_usergroups(): user.ensure_group("jupyterhub-users") logger.info("Granting passwordless sudo to JupyterHub admins...") + os.makedirs("/etc/sudoers.d/", exist_ok=True) with open("/etc/sudoers.d/jupyterhub-admins", "w") as f: # JupyterHub admins should have full passwordless sudo access f.write("%jupyterhub-admins ALL = (ALL) NOPASSWD: ALL\n") From ab947333d27dae748814d5ca2f0f19aec6735439 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 14:21:09 +0200 Subject: [PATCH 02/24] bootstrap.py: fix docstring summarizing --help output --- bootstrap/bootstrap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bootstrap/bootstrap.py b/bootstrap/bootstrap.py index 46acf3f..bea5bf5 100644 --- a/bootstrap/bootstrap.py +++ b/bootstrap/bootstrap.py @@ -36,7 +36,7 @@ Command line flags, from "bootstrap.py --help": logs can be accessed during installation. If this is passed, it will pass --progress-page-server-pid= to the tljh installer for later termination. - --version TLJH version or Git reference. Default 'latest' is + --version VERSION TLJH version or Git reference. Default 'latest' is the most recent release. Partial versions can be specified, for example '1', '1.0' or '1.0.0'. You can also pass a branch name such as 'main' or a From ecf11f0f2705e8793b46bafe689ff7f07dc67df0 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 22:48:06 +0200 Subject: [PATCH 03/24] bootstrap.py: let --version flag take precedence over env vars --- bootstrap/bootstrap.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/bootstrap/bootstrap.py b/bootstrap/bootstrap.py index bea5bf5..aec78b9 100644 --- a/bootstrap/bootstrap.py +++ b/bootstrap/bootstrap.py @@ -364,7 +364,7 @@ def main(): ) parser.add_argument( "--version", - default="latest", + default="", help=( "TLJH version or Git reference. " "Default 'latest' is the most recent release. " @@ -478,21 +478,26 @@ def main(): logger.info("Upgrading pip...") run_subprocess([hub_env_pip, "install", "--upgrade", "pip"]) - # Install/upgrade TLJH installer + # pip install TLJH installer based on + # + # 1. --version, _resolve_git_version is used + # 2. TLJH_BOOTSTRAP_PIP_SPEC (then also respect TLJH_BOOTSTRAP_DEV) + # 3. latest, _resolve_git_version is used + # tljh_install_cmd = [hub_env_pip, "install", "--upgrade"] - if os.environ.get("TLJH_BOOTSTRAP_DEV", "no") == "yes": - logger.info("Selected TLJH_BOOTSTRAP_DEV=yes...") - tljh_install_cmd.append("--editable") - bootstrap_pip_spec = os.environ.get("TLJH_BOOTSTRAP_PIP_SPEC") - if not bootstrap_pip_spec: + if args.version or not bootstrap_pip_spec: + version_to_resolve = args.version or "latest" bootstrap_pip_spec = ( "git+https://github.com/jupyterhub/the-littlest-jupyterhub.git@{}".format( - _resolve_git_version(args.version) + _resolve_git_version(version_to_resolve) ) ) - + elif os.environ.get("TLJH_BOOTSTRAP_DEV", "no") == "yes": + logger.info("Selected TLJH_BOOTSTRAP_DEV=yes...") + tljh_install_cmd.append("--editable") tljh_install_cmd.append(bootstrap_pip_spec) + if initial_setup: logger.info("Installing TLJH installer...") else: From 9e9596188674a13b049160e342a118f5ab5feed2 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 23:29:53 +0200 Subject: [PATCH 04/24] bootstrap.py: extract get_os_release_variable to make testing easier --- bootstrap/bootstrap.py | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/bootstrap/bootstrap.py b/bootstrap/bootstrap.py index aec78b9..ac52039 100644 --- a/bootstrap/bootstrap.py +++ b/bootstrap/bootstrap.py @@ -183,32 +183,32 @@ def run_subprocess(cmd, *args, **kwargs): return output +def get_os_release_variable(key): + """ + Return value for key from /etc/os-release + + /etc/os-release is a bash file, so should use bash to parse it. + + Returns empty string if key is not found. + """ + return ( + subprocess.check_output( + [ + "/bin/bash", + "-c", + "source /etc/os-release && echo ${{{key}}}".format(key=key), + ] + ) + .decode() + .strip() + ) + + def ensure_host_system_can_install_tljh(): """ Check if TLJH is installable in current host system and exit with a clear error message otherwise. """ - - def get_os_release_variable(key): - """ - Return value for key from /etc/os-release - - /etc/os-release is a bash file, so should use bash to parse it. - - Returns empty string if key is not found. - """ - return ( - subprocess.check_output( - [ - "/bin/bash", - "-c", - "source /etc/os-release && echo ${{{key}}}".format(key=key), - ] - ) - .decode() - .strip() - ) - # Require Ubuntu 20.04+ or Debian 11+ distro = get_os_release_variable("ID") version = get_os_release_variable("VERSION_ID") From 06edf1a76bbca5a599baadda614e91c9b94d5e26 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 14:07:45 +0200 Subject: [PATCH 05/24] test refactor: put pytest config in pyproject.toml --- integration-tests/requirements.txt | 1 + pyproject.toml | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/integration-tests/requirements.txt b/integration-tests/requirements.txt index a6f3d17..c086c7f 100644 --- a/integration-tests/requirements.txt +++ b/integration-tests/requirements.txt @@ -1,3 +1,4 @@ pytest +pytest-cov pytest-asyncio git+https://github.com/yuvipanda/hubtraf.git diff --git a/pyproject.toml b/pyproject.toml index 170be20..d0e97f8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -32,3 +32,24 @@ target_version = [ "py310", "py311", ] + + +# pytest is used for running Python based tests +# +# ref: https://docs.pytest.org/en/stable/ +# +[tool.pytest.ini_options] +addopts = "--verbose --color=yes --durations=10 --maxfail=1 --cov=tljh" +asyncio_mode = "auto" + + +# pytest-cov / coverage is used to measure code coverage of tests +# +# ref: https://coverage.readthedocs.io/en/stable/config.html +# +[tool.coverage.run] +parallel = true +omit = [ + "tests/**", + "integration-tests/**", +] From 2182f246ae7b802de1382e29c5c1944458bb70bf Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 14:02:37 +0200 Subject: [PATCH 06/24] test refactor: remove redundant pytest.mark.asyncio --- integration-tests/test_admin_installer.py | 2 -- integration-tests/test_hub.py | 8 -------- 2 files changed, 10 deletions(-) diff --git a/integration-tests/test_admin_installer.py b/integration-tests/test_admin_installer.py index c968978..083cf17 100644 --- a/integration-tests/test_admin_installer.py +++ b/integration-tests/test_admin_installer.py @@ -5,7 +5,6 @@ from hubtraf.auth.dummy import login_dummy from hubtraf.user import User -@pytest.mark.asyncio async def test_admin_login(): """ Test if the admin that was added during install can login with @@ -21,7 +20,6 @@ async def test_admin_login(): await u.ensure_server_simulate() -@pytest.mark.asyncio @pytest.mark.parametrize( "username, password", [ diff --git a/integration-tests/test_hub.py b/integration-tests/test_hub.py index faba9ea..7ea9ade 100644 --- a/integration-tests/test_hub.py +++ b/integration-tests/test_hub.py @@ -36,7 +36,6 @@ def test_hub_version(): assert V("4") <= V(info["version"]) <= V("5") -@pytest.mark.asyncio async def test_user_code_execute(): """ User logs in, starts a server & executes code @@ -68,7 +67,6 @@ async def test_user_code_execute(): assert pwd.getpwnam(f"jupyter-{username}") is not None -@pytest.mark.asyncio async def test_user_server_started_with_custom_base_url(): """ User logs in, starts a server with a custom base_url & executes code @@ -123,7 +121,6 @@ async def test_user_server_started_with_custom_base_url(): ) -@pytest.mark.asyncio async def test_user_admin_add(): """ User is made an admin, logs in and we check if they are in admin group @@ -168,7 +165,6 @@ async def test_user_admin_add(): # FIXME: Make this test pass -@pytest.mark.asyncio @pytest.mark.xfail(reason="Unclear why this is failing") async def test_user_admin_remove(): """ @@ -236,7 +232,6 @@ async def test_user_admin_remove(): assert f"jupyter-{username}" not in grp.getgrnam("jupyterhub-admins").gr_mem -@pytest.mark.asyncio async def test_long_username(): """ User with a long name logs in, and we check if their name is properly truncated. @@ -277,7 +272,6 @@ async def test_long_username(): raise -@pytest.mark.asyncio async def test_user_group_adding(): """ User logs in, and we check if they are added to the specified group. @@ -337,7 +331,6 @@ async def test_user_group_adding(): raise -@pytest.mark.asyncio async def test_idle_server_culled(): """ User logs in, starts a server & stays idle for 1 min. @@ -460,7 +453,6 @@ async def test_idle_server_culled(): ) -@pytest.mark.asyncio async def test_active_server_not_culled(): """ User logs in, starts a server & stays idle for 30s From e579d288c76e596d72a94980a1fe5cbbe7bbb898 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 14:03:12 +0200 Subject: [PATCH 07/24] test refactor: remove re-specification of hub_url --- integration-tests/test_hub.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/integration-tests/test_hub.py b/integration-tests/test_hub.py index 7ea9ade..bf56cc3 100644 --- a/integration-tests/test_hub.py +++ b/integration-tests/test_hub.py @@ -127,7 +127,6 @@ async def test_user_admin_add(): """ # This *must* be localhost, not an IP # aiohttp throws away cookies if we are connecting to an IP! - hub_url = "http://localhost" username = secrets.token_hex(8) assert ( @@ -174,7 +173,6 @@ async def test_user_admin_remove(): """ # This *must* be localhost, not an IP # aiohttp throws away cookies if we are connecting to an IP! - hub_url = "http://localhost" username = secrets.token_hex(8) assert ( @@ -236,9 +234,6 @@ async def test_long_username(): """ User with a long name logs in, and we check if their name is properly truncated. """ - # This *must* be localhost, not an IP - # aiohttp throws away cookies if we are connecting to an IP! - hub_url = "http://localhost" username = secrets.token_hex(32) assert ( @@ -278,7 +273,6 @@ async def test_user_group_adding(): """ # This *must* be localhost, not an IP # aiohttp throws away cookies if we are connecting to an IP! - hub_url = "http://localhost" username = secrets.token_hex(8) groups = {"somegroup": [username]} # Create the group we want to add the user to @@ -336,9 +330,6 @@ async def test_idle_server_culled(): User logs in, starts a server & stays idle for 1 min. (the user's server should be culled during this period) """ - # This *must* be localhost, not an IP - # aiohttp throws away cookies if we are connecting to an IP! - hub_url = "http://localhost" username = secrets.token_hex(8) assert ( @@ -460,7 +451,6 @@ async def test_active_server_not_culled(): """ # This *must* be localhost, not an IP # aiohttp throws away cookies if we are connecting to an IP! - hub_url = "http://localhost" username = secrets.token_hex(8) assert ( From 73e8eb2252bba31eb71e632966a208f18a99896e Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 14:22:25 +0200 Subject: [PATCH 08/24] test refactor: slim down workflow for readability --- .github/integration-test.py | 5 +---- .github/workflows/integration-test.yaml | 10 +++------- .github/workflows/unit-test.yaml | 12 ++++++------ 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/.github/integration-test.py b/.github/integration-test.py index b3fa090..b6fd9e9 100755 --- a/.github/integration-test.py +++ b/.github/integration-test.py @@ -187,10 +187,7 @@ def run_test( run_container_command( test_name, - # We abort pytest after two failures as a compromise between wanting to - # avoid a flood of logs while still understanding if multiple tests - # would fail. - "/opt/tljh/hub/bin/python3 -m pytest --verbose --maxfail=2 --color=yes --durations=10 --capture=no {}".format( + "/opt/tljh/hub/bin/python3 -m pytest --capture=no {}".format( " ".join( [os.path.join("/srv/src/integration-tests/", f) for f in test_files] ) diff --git a/.github/workflows/integration-test.yaml b/.github/workflows/integration-test.yaml index 1b7939e..e445347 100644 --- a/.github/workflows/integration-test.yaml +++ b/.github/workflows/integration-test.yaml @@ -59,16 +59,12 @@ jobs: with: python-version: "3.10" - - name: Install pytest - run: python3 -m pip install pytest + - name: Install integration-tests/requirements.txt + run: pip install -r integration-tests/requirements.txt - # We abort pytest after two failures as a compromise between wanting to - # avoid a flood of logs while still understanding if multiple tests would - # fail. - name: Run bootstrap tests (Runs in/Builds ${{ matrix.distro_image }} derived image) run: | - pytest --verbose --maxfail=2 --color=yes --durations=10 --capture=no \ - integration-tests/test_bootstrap.py + pytest --capture=no integration-tests/test_bootstrap.py timeout-minutes: 20 env: # integration-tests/test_bootstrap.py will build and start containers diff --git a/.github/workflows/unit-test.yaml b/.github/workflows/unit-test.yaml index 96b22bd..ea7e1e2 100644 --- a/.github/workflows/unit-test.yaml +++ b/.github/workflows/unit-test.yaml @@ -82,15 +82,15 @@ jobs: - name: Install Python dependencies run: | - python3 -m pip install -r dev-requirements.txt - python3 -m pip install -e . + pip install -r dev-requirements.txt + pip install -e . + + - name: List Python dependencies + run: | pip freeze - # We abort pytest after two failures as a compromise between wanting to - # avoid a flood of logs while still understanding if multiple tests would - # fail. - name: Run unit tests - run: pytest --verbose --maxfail=2 --color=yes --durations=10 --cov=tljh tests/ + run: pytest tests timeout-minutes: 15 - uses: codecov/codecov-action@v3 From 8f4cee1e46245142118b62f6d34cc2a5fabdf62c Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 14:25:00 +0200 Subject: [PATCH 09/24] test refactor: test_bootstrap.py, let container mount local dir and expose port --- integration-tests/test_bootstrap.py | 229 +++++++++++----------------- 1 file changed, 92 insertions(+), 137 deletions(-) diff --git a/integration-tests/test_bootstrap.py b/integration-tests/test_bootstrap.py index 545ba4c..d12357a 100644 --- a/integration-tests/test_bootstrap.py +++ b/integration-tests/test_bootstrap.py @@ -1,129 +1,78 @@ """ -Test running bootstrap script in different circumstances +This test file tests bootstrap.py ability to + +- error verbosely for old ubuntu +- error verbosely for no systemd +- start and provide a progress page web server """ import concurrent.futures import os import subprocess -import sys import time +GIT_REPO_PATH = os.path.abspath(os.path.dirname(os.path.dirname(__file__))) BASE_IMAGE = os.getenv("BASE_IMAGE", "ubuntu:20.04") -def install_pkgs(container_name, show_progress_page): - # Install python3 inside the ubuntu container - # There is no trusted Ubuntu+Python3 container we can use - pkgs = ["python3"] - if show_progress_page: - pkgs += ["systemd", "git", "curl"] - # Create the sudoers dir, so that the installer successfully gets to the - # point of starting jupyterhub and stopping the progress page server. - subprocess.check_output( - ["docker", "exec", container_name, "mkdir", "-p", "etc/sudoers.d"] - ) - - subprocess.check_output(["docker", "exec", container_name, "apt-get", "update"]) - subprocess.check_output( - ["docker", "exec", container_name, "apt-get", "install", "--yes"] + pkgs +def _stop_container(): + """ + Stops a container if its already running. + """ + subprocess.run( + ["docker", "rm", "--force", "test-bootstrap"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, ) -def get_bootstrap_script_location(container_name, show_progress_page): - # Copy only the bootstrap script to container when progress page not enabled, to be faster - source_path = "bootstrap/" - bootstrap_script = "/srv/src/bootstrap.py" - if show_progress_page: - source_path = os.path.abspath( - os.path.join(os.path.dirname(__file__), os.pardir) - ) - bootstrap_script = "/srv/src/bootstrap/bootstrap.py" - - subprocess.check_call(["docker", "cp", source_path, f"{container_name}:/srv/src"]) - return bootstrap_script - - -# FIXME: Refactor this function to easier to understand using the following -# parameters -# -# - param: container_apt_packages -# - param: bootstrap_tljh_source -# - local: copies local tljh repo to container and configures bootstrap to -# install tljh from copied repo -# - github: configures bootstrap to install tljh from the official github repo -# - : configures bootstrap to install tljh from any given remote location -# - param: bootstrap_flags -# -# FIXME: Consider stripping logic in this file to only testing if the bootstrap -# script successfully detects the too old Ubuntu version and the lack of -# systemd. The remaining test named test_progress_page could rely on -# running against the systemd container that cab be built by -# integration-test.py. -# -def run_bootstrap_after_preparing_container( - container_name, image, show_progress_page=False -): +def _run_bootstrap_in_container(image, complete_setup=True): """ - 1. Stops old container - 2. Starts --detached container - 3. Installs apt packages in container - 4. Two situations - - A) limited test (--show-progress-page=false) - - Copies ./bootstrap/ folder content to container /srv/src - - Runs copied bootstrap/bootstrap.py without flags - - B) full test (--show-progress-page=true) - - Copies ./ folder content to the container /srv/src - - Runs copied bootstrap/bootstrap.py with environment variables - - TLJH_BOOTSTRAP_DEV=yes - This makes --editable be used when installing the tljh package - - TLJH_BOOTSTRAP_PIP_SPEC=/srv/src - This makes us install tljh from the given location instead of from - github.com/jupyterhub/the-littlest-jupyterhub + 1. (Re-)starts a container named test-bootstrap based on image, mounting + local git repo and exposing port 8080 to the containers port 80. + 2. Installs python3, systemd, git, and curl in container + 3. Runs bootstrap/bootstrap.py in container to install the mounted git + repo's tljh package in --editable mode. """ - # stop container if it is already running - subprocess.run(["docker", "rm", "-f", container_name]) + _stop_container() # Start a detached container - subprocess.check_call( + subprocess.check_output( [ "docker", "run", "--env=DEBIAN_FRONTEND=noninteractive", + "--env=TLJH_BOOTSTRAP_DEV=yes", + "--env=TLJH_BOOTSTRAP_PIP_SPEC=/srv/src", + f"--volume={GIT_REPO_PATH}:/srv/src", + "--publish=8080:80", "--detach", - f"--name={container_name}", + "--name=test-bootstrap", image, - "/bin/bash", + "bash", "-c", - "sleep 1000s", + "sleep 300s", ] ) - install_pkgs(container_name, show_progress_page) - - bootstrap_script = get_bootstrap_script_location(container_name, show_progress_page) - - exec_flags = [ - "-i", - container_name, - "python3", - bootstrap_script, - "--version", - "main", - ] - if show_progress_page: - exec_flags = ( - ["-e", "TLJH_BOOTSTRAP_DEV=yes", "-e", "TLJH_BOOTSTRAP_PIP_SPEC=/srv/src"] - + exec_flags - + ["--show-progress-page"] + run = ["docker", "exec", "-i", "test-bootstrap"] + subprocess.check_output(run + ["apt-get", "update"]) + subprocess.check_output(run + ["apt-get", "install", "--yes", "python3"]) + if complete_setup: + subprocess.check_output( + run + ["apt-get", "install", "--yes", "systemd", "git", "curl"] ) - # Run bootstrap script, return the output + run_bootstrap = run + [ + "python3", + "/srv/src/bootstrap/bootstrap.py", + "--show-progress-page", + ] + + # Run bootstrap script inside detached container, return the output return subprocess.run( - ["docker", "exec"] + exec_flags, - check=False, - stdout=subprocess.PIPE, - encoding="utf-8", + run_bootstrap, + text=True, + capture_output=True, ) @@ -131,66 +80,72 @@ def test_ubuntu_too_old(): """ Error with a useful message when running in older Ubuntu """ - output = run_bootstrap_after_preparing_container("old-distro-test", "ubuntu:18.04") + output = _run_bootstrap_in_container("ubuntu:18.04", False) + _stop_container() assert output.stdout == "The Littlest JupyterHub requires Ubuntu 20.04 or higher\n" assert output.returncode == 1 -def test_inside_no_systemd_docker(): - output = run_bootstrap_after_preparing_container( - "plain-docker-test", - BASE_IMAGE, - ) +def test_no_systemd(): + output = _run_bootstrap_in_container("ubuntu:22.04", False) assert "Systemd is required to run TLJH" in output.stdout assert output.returncode == 1 -def verify_progress_page(expected_status_code, timeout): - progress_page_status = False +def _wait_for_progress_page_response(expected_status_code, timeout): start = time.time() - while not progress_page_status and (time.time() - start < timeout): + while time.time() - start < timeout: try: resp = subprocess.check_output( [ - "docker", - "exec", - "progress-page", "curl", - "-i", - "http://localhost/index.html", - ] + "--include", + "http://localhost:8080/index.html", + ], + text=True, + stderr=subprocess.DEVNULL, ) - if b"HTTP/1.0 200 OK" in resp: - progress_page_status = True - break - else: - print( - f"Unexpected progress page response: {resp[:100]}", file=sys.stderr - ) - except Exception as e: - print(f"Error getting progress page: {e}", file=sys.stderr) - time.sleep(1) - continue + if "HTTP/1.0 200 OK" in resp: + return True + except Exception: + pass + time.sleep(1) - return progress_page_status + return False -def test_progress_page(): +def test_show_progress_page(): with concurrent.futures.ThreadPoolExecutor() as executor: - installer = executor.submit( - run_bootstrap_after_preparing_container, - "progress-page", - BASE_IMAGE, - True, + run_bootstrap_job = executor.submit(_run_bootstrap_in_container, BASE_IMAGE) + + # Check that the bootstrap script started the web server reporting + # progress successfully responded. + success = _wait_for_progress_page_response( + expected_status_code=200, timeout=180 ) + if success: + # Let's terminate the test here and save a minute or so in test + # executation time, because we can know that the will be stopped + # successfully in other tests as otherwise traefik won't be able to + # start and use the same port for example. + return - # Check if progress page started - started = verify_progress_page(expected_status_code=200, timeout=180) - assert started + # Now await an expected failure to startup JupyterHub by tljh.installer, + # which should have taken over the work started by the bootstrap script. + # + # This failure is expected to occur in + # tljh.installer.ensure_jupyterhub_service calling systemd.reload_daemon + # like this: + # + # > System has not been booted with systemd as init system (PID 1). + # > Can't operate. + # + output = run_bootstrap_job.result() + print(output.stdout) + print(output.stderr) - # This will fail start tljh but should successfully get to the point - # Where it stops the progress page server. - output = installer.result() - - # Check if progress page stopped + # At this point we should be able to see that tljh.installer + # intentionally stopped the web server reporting progress as the port + # were about to become needed by Traefik. assert "Progress page server stopped successfully." in output.stdout + assert success From 835c6a115469e5d5f687fab468ef6775dc229064 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 6 Jun 2023 16:53:16 +0200 Subject: [PATCH 10/24] test refactor: refactoring of .github/integration-test.py --- .github/integration-test.py | 275 +++++++++++----------- .github/workflows/integration-test.yaml | 64 ++--- dev-requirements.txt | 1 + integration-tests/test_admin_installer.py | 5 +- integration-tests/test_bootstrap.py | 4 + tests/test_bootstrap_functions.py | 6 +- 6 files changed, 173 insertions(+), 182 deletions(-) diff --git a/.github/integration-test.py b/.github/integration-test.py index b6fd9e9..eb01cb1 100755 --- a/.github/integration-test.py +++ b/.github/integration-test.py @@ -1,154 +1,160 @@ #!/usr/bin/env python3 import argparse +import functools import os import subprocess import time from shutil import which +GIT_REPO_PATH = os.path.abspath(os.path.dirname(os.path.dirname(__file__))) +TEST_IMAGE_NAME = "test-systemd" -def container_runtime(): + +@functools.lru_cache() +def _get_container_runtime_cli(): runtimes = ["docker", "podman"] for runtime in runtimes: if which(runtime): return runtime - raise RuntimeError(f"No container runtime found, tried: {' '.join(runtimes)}") + raise RuntimeError(f"No container runtime CLI found, tried: {' '.join(runtimes)}") -def container_check_output(*args, **kwargs): - cmd = [container_runtime()] + list(*args) - print(f"Running {cmd} {kwargs}") - return subprocess.check_output(cmd, **kwargs) +def _cli(args, log_failure=True): + cmd = [_get_container_runtime_cli(), *args] + try: + return subprocess.check_output(cmd, text=True, stderr=subprocess.STDOUT) + except subprocess.CalledProcessError: + if log_failure: + print(f"{cmd} failed!") + raise -def container_run(*args, **kwargs): - cmd = [container_runtime()] + list(*args) - print(f"Running {cmd} {kwargs}") - return subprocess.run(cmd, **kwargs) - - -def build_systemd_image(image_name, source_path, build_args=None): +def _await_container_startup(container_name, timeout=60): """ - Build docker image with systemd at source_path. - - Built image is tagged with image_name + Await container to become ready, as checked by attempting to run a basic + command (id) inside it. """ - cmd = ["build", f"-t={image_name}", source_path] - if build_args: - cmd.extend([f"--build-arg={ba}" for ba in build_args]) - container_check_output(cmd) - - -def check_container_ready(container_name, timeout=60): - """ - Check if container is ready to run tests - """ - now = time.time() + start = time.time() while True: try: - out = container_check_output(["exec", "-t", container_name, "id"]) - print(out.decode()) + _cli(["exec", "-t", container_name, "id"], log_failure=False) return - except subprocess.CalledProcessError as e: - print(e) - try: - out = container_check_output(["inspect", container_name]) - print(out.decode()) - except subprocess.CalledProcessError as e: - print(e) - try: - out = container_check_output(["logs", container_name]) - print(out.decode()) - except subprocess.CalledProcessError as e: - print(e) - if time.time() - now > timeout: - raise RuntimeError(f"Container {container_name} hasn't started") - time.sleep(5) + except subprocess.CalledProcessError: + if time.time() - start > timeout: + inspect = "" + logs = "" + try: + inspect = _cli(["inspect", container_name], log_failure=False) + except subprocess.CalledProcessError as e: + inspect = e.output + try: + logs = _cli(["logs", container_name], log_failure=False) + except subprocess.CalledProcessError as e: + logs = e.output + raise RuntimeError( + f"Container {container_name} failed to start! Debugging info follows...\n\n" + f"> docker inspect {container_name}\n" + "----------------------------------------\n" + f"{inspect}\n" + f"> docker logs {container_name}\n" + "----------------------------------------\n" + f"{logs}\n" + ) + time.sleep(1) -def run_systemd_image(image_name, container_name, bootstrap_pip_spec): +def build_image(build_args=None): """ - Run docker image with systemd + Build Dockerfile with systemd in the integration-tests folder to run tests + from. + """ + cmd = [ + _get_container_runtime_cli(), + "build", + f"--tag={TEST_IMAGE_NAME}", + "integration-tests", + ] + if build_args: + cmd.extend([f"--build-arg={ba}" for ba in build_args]) - Image named image_name should be built with build_systemd_image. + subprocess.run(cmd, check=True, text=True) - Container named container_name will be started. + +def start_container(container_name, bootstrap_pip_spec): + """ + Starts a container based on an image expected to start systemd. """ cmd = [ "run", - "--privileged", + "--rm", "--detach", + "--privileged", f"--name={container_name}", # A bit less than 1GB to ensure TLJH runs on 1GB VMs. # If this is changed all docs references to the required memory must be changed too. "--memory=900m", ] - if bootstrap_pip_spec: - cmd.append("-e") - cmd.append(f"TLJH_BOOTSTRAP_PIP_SPEC={bootstrap_pip_spec}") + cmd.append(f"--env=TLJH_BOOTSTRAP_PIP_SPEC={bootstrap_pip_spec}") + else: + cmd.append("--env=TLJH_BOOTSTRAP_DEV=yes") + cmd.append("--env=TLJH_BOOTSTRAP_PIP_SPEC=/srv/src") + cmd.append(TEST_IMAGE_NAME) - cmd.append(image_name) - - container_check_output(cmd) + return _cli(cmd) def stop_container(container_name): """ - Stop & remove docker container if it exists. + Stop and remove docker container if it exists. """ try: - container_check_output(["inspect", container_name], stderr=subprocess.STDOUT) + return _cli(["rm", "--force", container_name], log_failure=False) except subprocess.CalledProcessError: - # No such container exists, nothing to do - return - container_check_output(["rm", "-f", container_name]) + pass -def run_container_command(container_name, cmd): +def run_command(container_name, cmd): """ - Run cmd in a running container with a bash shell + Run a bash command in a running container and error if it fails """ - proc = container_run( - ["exec", "-t", container_name, "/bin/bash", "-c", cmd], - check=True, - ) + cmd = [ + _get_container_runtime_cli(), + "exec", + "-t", + container_name, + "/bin/bash", + "-c", + cmd, + ] + subprocess.run(cmd, check=True, text=True) def copy_to_container(container_name, src_path, dest_path): """ - Copy files from src_path to dest_path inside container_name + Copy files from a path on the local file system to a destination in a + running container """ - container_check_output(["cp", src_path, f"{container_name}:{dest_path}"]) + _cli(["cp", src_path, f"{container_name}:{dest_path}"]) def run_test( - image_name, - test_name, + container_name, bootstrap_pip_spec, test_files, upgrade_from, installer_args, ): """ - Starts a new container based on image_name, runs the bootstrap script to - setup tljh with installer_args, and runs test_name. + (Re-)starts a named container with given (Systemd based) image, then runs + the bootstrap script inside it to setup tljh with installer_args. + + Thereafter, source files are copied to the container and """ - stop_container(test_name) - run_systemd_image(image_name, test_name, bootstrap_pip_spec) - - check_container_ready(test_name) - - source_path = os.path.abspath(os.path.join(os.path.dirname(__file__), os.pardir)) - - copy_to_container(test_name, os.path.join(source_path, "bootstrap/."), "/srv/src") - copy_to_container( - test_name, os.path.join(source_path, "integration-tests/"), "/srv/src" - ) - - # These logs can be very relevant to debug a container startup failure - print(f"--- Start of logs from the container: {test_name}") - print(container_check_output(["logs", test_name]).decode()) - print(f"--- End of logs from the container: {test_name}") + stop_container(container_name) + start_container(container_name, bootstrap_pip_spec) + _await_container_startup(container_name) + copy_to_container(container_name, GIT_REPO_PATH, "/srv/src") # To test upgrades, we run a bootstrap.py script two times instead of one, # where the initial run first installs some older version. @@ -156,38 +162,32 @@ def run_test( # We want to support testing a PR by upgrading from "main", "latest" (latest # released version), and from a previous major-like version. # - # FIXME: We currently always rely on the main branch's bootstrap.py script. - # Realistically, we should run previous versions of the bootstrap - # script which also installs previous versions of TLJH. - # - # 2023-04-15 Erik observed that https://tljh.jupyter.org/bootstrap.py - # is referencing to the master (now main) branch which didn't seem - # obvious, thinking it could have been the latest released version - # also. - # if upgrade_from: - run_container_command( - test_name, - f"curl -L https://tljh.jupyter.org/bootstrap.py | python3 - --version={upgrade_from}", + run_command( + container_name, + f"python3 /srv/src/bootstrap/bootstrap.py --version={upgrade_from}", ) - run_container_command(test_name, f"python3 /srv/src/bootstrap.py {installer_args}") + run_command( + container_name, f"python3 /srv/src/bootstrap/bootstrap.py {installer_args}" + ) # Install pkgs from requirements in hub's pip, where # the bootstrap script installed the others - run_container_command( - test_name, + run_command( + container_name, "/opt/tljh/hub/bin/python3 -m pip install -r /srv/src/integration-tests/requirements.txt", ) # show environment - run_container_command( - test_name, + run_command( + container_name, "/opt/tljh/hub/bin/python3 -m pip freeze", ) - run_container_command( - test_name, - "/opt/tljh/hub/bin/python3 -m pytest --capture=no {}".format( + # run tests + run_command( + container_name, + "/opt/tljh/hub/bin/python3 -m pytest {}".format( " ".join( [os.path.join("/srv/src/integration-tests/", f) for f in test_files] ) @@ -197,12 +197,20 @@ def run_test( def show_logs(container_name): """ - Print logs from inside container to stdout + Print jupyterhub and traefik status and logs from both. + + tljh logs ref: https://tljh.jupyter.org/en/latest/troubleshooting/logs.html """ - run_container_command(container_name, "journalctl --no-pager") - run_container_command( + systemctl = run_command( container_name, "systemctl --no-pager status jupyterhub traefik" ) + print(systemctl) + + jupyterhub_logs = run_command(container_name, "journalctl --no-pager -u jupyterhub") + print(jupyterhub_logs) + + traefik_logs = run_command(container_name, "journalctl --no-pager -u traefik") + print(traefik_logs) def main(): @@ -210,18 +218,14 @@ def main(): subparsers = argparser.add_subparsers(dest="action") build_image_parser = subparsers.add_parser("build-image") - build_image_parser.add_argument( - "--build-arg", - action="append", - dest="build_args", - ) - - stop_container_parser = subparsers.add_parser("stop-container") - stop_container_parser.add_argument("container_name") + build_image_parser.add_argument("--build-arg", action="append", dest="build_args") start_container_parser = subparsers.add_parser("start-container") start_container_parser.add_argument("container_name") + stop_container_parser = subparsers.add_parser("stop-container") + stop_container_parser.add_argument("container_name") + run_parser = subparsers.add_parser("run") run_parser.add_argument("container_name") run_parser.add_argument("command") @@ -234,10 +238,8 @@ def main(): run_test_parser = subparsers.add_parser("run-test") run_test_parser.add_argument("--installer-args", default="") run_test_parser.add_argument("--upgrade-from", default="") - run_test_parser.add_argument( - "--bootstrap-pip-spec", nargs="?", default="", type=str - ) - run_test_parser.add_argument("test_name") + run_test_parser.add_argument("--bootstrap-pip-spec", default="/srv/src") + run_test_parser.add_argument("container_name") run_test_parser.add_argument("test_files", nargs="+") show_logs_parser = subparsers.add_parser("show-logs") @@ -245,12 +247,19 @@ def main(): args = argparser.parse_args() - image_name = "tljh-systemd" - - if args.action == "run-test": + if args.action == "build-image": + build_image(args.build_args) + elif args.action == "start-container": + start_container(args.container_name, args.bootstrap_pip_spec) + elif args.action == "stop-container": + stop_container(args.container_name) + elif args.action == "run": + run_command(args.container_name, args.command) + elif args.action == "copy": + copy_to_container(args.container_name, args.src, args.dest) + elif args.action == "run-test": run_test( - image_name, - args.test_name, + args.container_name, args.bootstrap_pip_spec, args.test_files, args.upgrade_from, @@ -258,16 +267,6 @@ def main(): ) elif args.action == "show-logs": show_logs(args.container_name) - elif args.action == "run": - run_container_command(args.container_name, args.command) - elif args.action == "copy": - copy_to_container(args.container_name, args.src, args.dest) - elif args.action == "start-container": - run_systemd_image(image_name, args.container_name, args.bootstrap_pip_spec) - elif args.action == "stop-container": - stop_container(args.container_name) - elif args.action == "build-image": - build_systemd_image(image_name, "integration-tests", args.build_args) if __name__ == "__main__": diff --git a/.github/workflows/integration-test.yaml b/.github/workflows/integration-test.yaml index e445347..b020e06 100644 --- a/.github/workflows/integration-test.yaml +++ b/.github/workflows/integration-test.yaml @@ -64,8 +64,8 @@ jobs: - name: Run bootstrap tests (Runs in/Builds ${{ matrix.distro_image }} derived image) run: | - pytest --capture=no integration-tests/test_bootstrap.py - timeout-minutes: 20 + pytest integration-tests/test_bootstrap.py + timeout-minutes: 10 env: # integration-tests/test_bootstrap.py will build and start containers # based on this environment variable. This is similar to how @@ -73,64 +73,52 @@ jobs: # setting the base image via a Dockerfile ARG. BASE_IMAGE: ${{ matrix.distro_image }} - # We build a docker image from wherein we will work - - name: Build systemd image (Builds ${{ matrix.distro_image }} derived image) + - name: Build systemd image, derived from ${{ matrix.distro_image }} run: | .github/integration-test.py build-image \ --build-arg "BASE_IMAGE=${{ matrix.distro_image }}" - # FIXME: Make the logic below easier to follow. - # - In short, setting BOOTSTRAP_PIP_SPEC here, specifies from what - # location the tljh python package should be installed from. In this - # GitHub Workflow's test job, we provide a remote reference to itself as - # found on GitHub - this could be the HEAD of a PR branch or the default - # branch on merge. - # # Overview of how this logic influences the end result. # - integration-test.yaml: - # Runs integration-test.py by passing --bootstrap-pip-spec flag with a - # reference to the pull request on GitHub. - # - integration-test.py: - # Starts a pre-build systemd container, setting the - # TLJH_BOOTSTRAP_PIP_SPEC based on its passed --bootstrap-pip-spec value. - # - systemd container: - # Runs bootstrap.py - # - bootstrap.py - # Makes use of TLJH_BOOTSTRAP_PIP_SPEC environment variable to install - # the tljh package from a given location, which could be a local git - # clone of this repo where setup.py resides, or a reference to some - # GitHub branch for example. - - name: Set BOOTSTRAP_PIP_SPEC value - run: | - BOOTSTRAP_PIP_SPEC="git+https://github.com/$GITHUB_REPOSITORY.git@$GITHUB_REF" - echo "BOOTSTRAP_PIP_SPEC=$BOOTSTRAP_PIP_SPEC" >> $GITHUB_ENV - echo $BOOTSTRAP_PIP_SPEC - - - name: Run basic tests (Runs in ${{ matrix.distro_image }} derived image) + # + # - Runs integration-test.py build-image, to build a systemd based image + # to use later. + # - Runs integration-test.py run-tests, to start a systemd based + # container, run the bootstrap.py script inside it, and then run + # pytest from the hub python environment setup by the bootstrap + # script. + # + - name: pytest integration-tests/ run: | .github/integration-test.py run-test basic-tests \ - --bootstrap-pip-spec "$BOOTSTRAP_PIP_SPEC" \ ${{ matrix.extra_flags }} \ test_hub.py \ test_proxy.py \ test_install.py \ test_extensions.py - timeout-minutes: 15 + timeout-minutes: 10 + - name: show logs + run: | + .github/integration-test.py show-logs basic-tests - - name: Run admin tests (Runs in ${{ matrix.distro_image }} derived image) + - name: pytest integration-tests/test_admin_installer.py run: | .github/integration-test.py run-test admin-tests \ --installer-args "--admin admin:admin" \ - --bootstrap-pip-spec "$BOOTSTRAP_PIP_SPEC" \ ${{ matrix.extra_flags }} \ test_admin_installer.py - timeout-minutes: 15 + timeout-minutes: 5 + - name: show logs + run: | + .github/integration-test.py show-logs admin-tests - - name: Run plugin tests (Runs in ${{ matrix.distro_image }} derived image) + - name: pytest integration-tests/test_simplest_plugin.py run: | .github/integration-test.py run-test plugin-tests \ - --bootstrap-pip-spec "$BOOTSTRAP_PIP_SPEC" \ --installer-args "--plugin /srv/src/integration-tests/plugins/simplest" \ ${{ matrix.extra_flags }} \ test_simplest_plugin.py - timeout-minutes: 15 + timeout-minutes: 5 + - name: show logs + run: | + .github/integration-test.py show-logs plugin-tests diff --git a/dev-requirements.txt b/dev-requirements.txt index 9f14bc3..672ad32 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,4 +1,5 @@ packaging pytest pytest-cov +pytest-asyncio pytest-mock diff --git a/integration-tests/test_admin_installer.py b/integration-tests/test_admin_installer.py index 083cf17..44fa18a 100644 --- a/integration-tests/test_admin_installer.py +++ b/integration-tests/test_admin_installer.py @@ -4,13 +4,14 @@ import pytest from hubtraf.auth.dummy import login_dummy from hubtraf.user import User +hub_url = "http://localhost" + async def test_admin_login(): """ Test if the admin that was added during install can login with the password provided. """ - hub_url = "http://localhost" username = "admin" password = "admin" @@ -32,8 +33,6 @@ async def test_unsuccessful_login(username, password): """ Ensure nobody but the admin that was added during install can login """ - hub_url = "http://localhost" - async with User(username, hub_url, partial(login_dummy, password="")) as u: user_logged_in = await u.login() diff --git a/integration-tests/test_bootstrap.py b/integration-tests/test_bootstrap.py index d12357a..09ae140 100644 --- a/integration-tests/test_bootstrap.py +++ b/integration-tests/test_bootstrap.py @@ -4,6 +4,10 @@ This test file tests bootstrap.py ability to - error verbosely for old ubuntu - error verbosely for no systemd - start and provide a progress page web server + +FIXME: The last test stands out and could be part of the other tests, and the + first two could be more like unit tests. Ideally, this file is + significantly reduced. """ import concurrent.futures import os diff --git a/tests/test_bootstrap_functions.py b/tests/test_bootstrap_functions.py index 799dc3c..b3625b8 100644 --- a/tests/test_bootstrap_functions.py +++ b/tests/test_bootstrap_functions.py @@ -1,12 +1,12 @@ # Unit test some functions from bootstrap.py -# Since bootstrap.py isn't part of the package, it's not automatically importable import os import sys -sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) - import pytest +# Since bootstrap.py isn't part of the package, it's not automatically importable +GIT_REPO_PATH = os.path.abspath(os.path.dirname(os.path.dirname(__file__))) +sys.path.insert(0, GIT_REPO_PATH) from bootstrap import bootstrap From 7f873966b57145de80c242e05afc9fd7638b7835 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Wed, 7 Jun 2023 00:21:15 +0200 Subject: [PATCH 11/24] test refactor: combine admin and plugin tests --- .github/workflows/integration-test.yaml | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/.github/workflows/integration-test.yaml b/.github/workflows/integration-test.yaml index b020e06..1cca9f6 100644 --- a/.github/workflows/integration-test.yaml +++ b/.github/workflows/integration-test.yaml @@ -101,24 +101,14 @@ jobs: run: | .github/integration-test.py show-logs basic-tests - - name: pytest integration-tests/test_admin_installer.py + - name: pytest integration-tests/test_simplest_plugin.py integration-tests/test_admin_installer.py run: | - .github/integration-test.py run-test admin-tests \ - --installer-args "--admin admin:admin" \ - ${{ matrix.extra_flags }} \ - test_admin_installer.py - timeout-minutes: 5 - - name: show logs - run: | - .github/integration-test.py show-logs admin-tests - - - name: pytest integration-tests/test_simplest_plugin.py - run: | - .github/integration-test.py run-test plugin-tests \ - --installer-args "--plugin /srv/src/integration-tests/plugins/simplest" \ + .github/integration-test.py run-test admin-plugin-tests \ + --installer-args "--admin admin:admin --plugin /srv/src/integration-tests/plugins/simplest" \ ${{ matrix.extra_flags }} \ + test_admin_installer.py \ test_simplest_plugin.py timeout-minutes: 5 - name: show logs run: | - .github/integration-test.py show-logs plugin-tests + .github/integration-test.py show-logs admin-plugin-tests From f127322aa32c32490e557613a47d3f63d4d51071 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Wed, 7 Jun 2023 00:34:02 +0200 Subject: [PATCH 12/24] test refactor: be more generous with timeout values --- .github/workflows/integration-test.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/integration-test.yaml b/.github/workflows/integration-test.yaml index 1cca9f6..2942d16 100644 --- a/.github/workflows/integration-test.yaml +++ b/.github/workflows/integration-test.yaml @@ -96,7 +96,7 @@ jobs: test_proxy.py \ test_install.py \ test_extensions.py - timeout-minutes: 10 + timeout-minutes: 15 - name: show logs run: | .github/integration-test.py show-logs basic-tests @@ -108,7 +108,7 @@ jobs: ${{ matrix.extra_flags }} \ test_admin_installer.py \ test_simplest_plugin.py - timeout-minutes: 5 + timeout-minutes: 15 - name: show logs run: | .github/integration-test.py show-logs admin-plugin-tests From cdc4a9d3880bbe194da259ec2891901bcbcde096 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Wed, 7 Jun 2023 01:35:34 +0200 Subject: [PATCH 13/24] test refactor: reduce timeout for starting user server, sleep less --- integration-tests/test_admin_installer.py | 2 +- integration-tests/test_hub.py | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/integration-tests/test_admin_installer.py b/integration-tests/test_admin_installer.py index 44fa18a..88ca9e7 100644 --- a/integration-tests/test_admin_installer.py +++ b/integration-tests/test_admin_installer.py @@ -18,7 +18,7 @@ async def test_admin_login(): async with User(username, hub_url, partial(login_dummy, password=password)) as u: await u.login() # If user is not logged in, this will raise an exception - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) @pytest.mark.parametrize( diff --git a/integration-tests/test_hub.py b/integration-tests/test_hub.py index bf56cc3..502df94 100644 --- a/integration-tests/test_hub.py +++ b/integration-tests/test_hub.py @@ -59,7 +59,7 @@ async def test_user_code_execute(): async with User(username, hub_url, partial(login_dummy, password="")) as u: await u.login() - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) await u.start_kernel() await u.assert_code_output("5 * 4", "20", 5, 5) @@ -102,7 +102,7 @@ async def test_user_server_started_with_custom_base_url(): async with User(username, hub_url, partial(login_dummy, password="")) as u: await u.login() - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # unset base_url to avoid problems with other tests assert ( @@ -154,7 +154,7 @@ async def test_user_admin_add(): async with User(username, hub_url, partial(login_dummy, password="")) as u: await u.login() - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists assert pwd.getpwnam(f"jupyter-{username}") is not None @@ -200,7 +200,7 @@ async def test_user_admin_remove(): async with User(username, hub_url, partial(login_dummy, password="")) as u: await u.login() - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists assert pwd.getpwnam(f"jupyter-{username}") is not None @@ -224,7 +224,7 @@ async def test_user_admin_remove(): ) await u.stop_server() - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user does *not* have admin rights assert f"jupyter-{username}" not in grp.getgrnam("jupyterhub-admins").gr_mem @@ -254,7 +254,7 @@ async def test_long_username(): try: async with User(username, hub_url, partial(login_dummy, password="")) as u: await u.login() - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists system_username = generate_system_username(f"jupyter-{username}") @@ -307,7 +307,7 @@ async def test_user_group_adding(): try: async with User(username, hub_url, partial(login_dummy, password="")) as u: await u.login() - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists system_username = generate_system_username(f"jupyter-{username}") @@ -379,7 +379,7 @@ async def test_idle_server_culled(): await u.login() # Start user's server - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists assert pwd.getpwnam(f"jupyter-{username}") is not None @@ -498,7 +498,7 @@ async def test_active_server_not_culled(): async with User(username, hub_url, partial(login_dummy, password="")) as u: await u.login() # Start user's server - await u.ensure_server_simulate() + await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists assert pwd.getpwnam(f"jupyter-{username}") is not None From 971b7d5c7ef7931731f3cfaae72d8a741f3f713c Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Wed, 7 Jun 2023 01:25:35 +0200 Subject: [PATCH 14/24] test refactor: show logs from jupyterhub and traefik after tests --- .github/integration-test.py | 55 +++++++++---------------- .github/workflows/integration-test.yaml | 4 ++ 2 files changed, 23 insertions(+), 36 deletions(-) diff --git a/.github/integration-test.py b/.github/integration-test.py index eb01cb1..39d27f4 100755 --- a/.github/integration-test.py +++ b/.github/integration-test.py @@ -25,7 +25,7 @@ def _cli(args, log_failure=True): return subprocess.check_output(cmd, text=True, stderr=subprocess.STDOUT) except subprocess.CalledProcessError: if log_failure: - print(f"{cmd} failed!") + print(f"{cmd} failed!", flush=True) raise @@ -114,7 +114,7 @@ def stop_container(container_name): pass -def run_command(container_name, cmd): +def run_command(container_name, command): """ Run a bash command in a running container and error if it fails """ @@ -125,8 +125,9 @@ def run_command(container_name, cmd): container_name, "/bin/bash", "-c", - cmd, + command, ] + print(f"\nRunning: {cmd}\n----------------------------------------", flush=True) subprocess.run(cmd, check=True, text=True) @@ -163,36 +164,25 @@ def run_test( # released version), and from a previous major-like version. # if upgrade_from: - run_command( - container_name, - f"python3 /srv/src/bootstrap/bootstrap.py --version={upgrade_from}", - ) - run_command( - container_name, f"python3 /srv/src/bootstrap/bootstrap.py {installer_args}" - ) + command = f"python3 /srv/src/bootstrap/bootstrap.py --version={upgrade_from}" + run_command(container_name, command) + + command = f"python3 /srv/src/bootstrap/bootstrap.py {installer_args}" + run_command(container_name, command) # Install pkgs from requirements in hub's pip, where # the bootstrap script installed the others - run_command( - container_name, - "/opt/tljh/hub/bin/python3 -m pip install -r /srv/src/integration-tests/requirements.txt", - ) + command = "/opt/tljh/hub/bin/python3 -m pip install -r /srv/src/integration-tests/requirements.txt" + run_command(container_name, command) # show environment - run_command( - container_name, - "/opt/tljh/hub/bin/python3 -m pip freeze", - ) + command = "/opt/tljh/hub/bin/python3 -m pip freeze" + run_command(container_name, command) # run tests - run_command( - container_name, - "/opt/tljh/hub/bin/python3 -m pytest {}".format( - " ".join( - [os.path.join("/srv/src/integration-tests/", f) for f in test_files] - ) - ), - ) + test_files = " ".join([f"/srv/src/integration-tests/{f}" for f in test_files]) + command = f"/opt/tljh/hub/bin/python3 -m pytest {test_files}" + run_command(container_name, command) def show_logs(container_name): @@ -201,16 +191,9 @@ def show_logs(container_name): tljh logs ref: https://tljh.jupyter.org/en/latest/troubleshooting/logs.html """ - systemctl = run_command( - container_name, "systemctl --no-pager status jupyterhub traefik" - ) - print(systemctl) - - jupyterhub_logs = run_command(container_name, "journalctl --no-pager -u jupyterhub") - print(jupyterhub_logs) - - traefik_logs = run_command(container_name, "journalctl --no-pager -u traefik") - print(traefik_logs) + run_command(container_name, "systemctl --no-pager status jupyterhub traefik") + run_command(container_name, "journalctl --no-pager -u jupyterhub") + run_command(container_name, "journalctl --no-pager -u traefik") def main(): diff --git a/.github/workflows/integration-test.yaml b/.github/workflows/integration-test.yaml index 2942d16..d53eec0 100644 --- a/.github/workflows/integration-test.yaml +++ b/.github/workflows/integration-test.yaml @@ -89,6 +89,7 @@ jobs: # script. # - name: pytest integration-tests/ + id: basic-tests run: | .github/integration-test.py run-test basic-tests \ ${{ matrix.extra_flags }} \ @@ -98,10 +99,12 @@ jobs: test_extensions.py timeout-minutes: 15 - name: show logs + if: always() && steps.basic-tests.outcome != 'skipped' run: | .github/integration-test.py show-logs basic-tests - name: pytest integration-tests/test_simplest_plugin.py integration-tests/test_admin_installer.py + id: admin-plugin-tests run: | .github/integration-test.py run-test admin-plugin-tests \ --installer-args "--admin admin:admin --plugin /srv/src/integration-tests/plugins/simplest" \ @@ -110,5 +113,6 @@ jobs: test_simplest_plugin.py timeout-minutes: 15 - name: show logs + if: always() && steps.admin-plugin-tests.outcome != 'skipped' run: | .github/integration-test.py show-logs admin-plugin-tests From 40b88cb780cb009d55fd9128a00ff705b428eb34 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 8 Jun 2023 22:44:21 +0200 Subject: [PATCH 15/24] test refactor: update simplest plugin tests With this refactor, the simplest plugin stops interfering with other tests. --- .../plugins/simplest/tljh_simplest.py | 36 +++----- integration-tests/test_simplest_plugin.py | 86 ++++++++++--------- 2 files changed, 60 insertions(+), 62 deletions(-) diff --git a/integration-tests/plugins/simplest/tljh_simplest.py b/integration-tests/plugins/simplest/tljh_simplest.py index a134083..015b504 100644 --- a/integration-tests/plugins/simplest/tljh_simplest.py +++ b/integration-tests/plugins/simplest/tljh_simplest.py @@ -1,55 +1,47 @@ """ -Simplest plugin that exercises all the hooks +Simplest plugin that exercises all the hooks defined in tljh/hooks.py. """ from tljh.hooks import hookimpl @hookimpl def tljh_extra_user_conda_packages(): - return [ - "hypothesis", - ] + return ["tqdm"] @hookimpl def tljh_extra_user_pip_packages(): - return [ - "django", - ] + return ["django"] @hookimpl def tljh_extra_hub_pip_packages(): - return [ - "there", - ] + return ["there"] @hookimpl def tljh_extra_apt_packages(): - return [ - "sl", - ] - - -@hookimpl -def tljh_config_post_install(config): - # Put an arbitrary marker we can test for - config["simplest_plugin"] = {"present": True} + return ["sl"] @hookimpl def tljh_custom_jupyterhub_config(c): - c.JupyterHub.authenticator_class = "tmpauthenticator.TmpAuthenticator" + c.Test.jupyterhub_config_set_by_simplest_plugin = True + + +@hookimpl +def tljh_config_post_install(config): + config["Test"] = {"tljh_config_set_by_simplest_plugin": True} @hookimpl def tljh_post_install(): - with open("test_post_install", "w") as f: - f.write("123456789") + with open("test_tljh_post_install", "w") as f: + f.write("file_written_by_simplest_plugin") @hookimpl def tljh_new_user_create(username): with open("test_new_user_create", "w") as f: + f.write("file_written_by_simplest_plugin") f.write(username) diff --git a/integration-tests/test_simplest_plugin.py b/integration-tests/test_simplest_plugin.py index 171578c..96eb0a5 100644 --- a/integration-tests/test_simplest_plugin.py +++ b/integration-tests/test_simplest_plugin.py @@ -1,79 +1,85 @@ """ -Test simplest plugin +Test the plugin in integration-tests/plugins/simplest that makes use of all tljh +recognized plugin hooks that are defined in tljh/hooks.py. """ import os import subprocess -import requests from ruamel.yaml import YAML from tljh import user from tljh.config import CONFIG_FILE, HUB_ENV_PREFIX, USER_ENV_PREFIX +GIT_REPO_PATH = os.path.abspath(os.path.dirname(os.path.dirname(__file__))) yaml = YAML(typ="rt") -def test_apt_packages(): - """ - Test extra apt packages are installed - """ - assert os.path.exists("/usr/games/sl") +def test_tljh_extra_user_conda_packages(): + subprocess.check_call([f"{USER_ENV_PREFIX}/bin/python3", "-c", "import tqdm"]) -def test_pip_packages(): - """ - Test extra user & hub pip packages are installed - """ +def test_tljh_extra_user_pip_packages(): subprocess.check_call([f"{USER_ENV_PREFIX}/bin/python3", "-c", "import django"]) + +def test_tljh_extra_hub_pip_packages(): subprocess.check_call([f"{HUB_ENV_PREFIX}/bin/python3", "-c", "import there"]) -def test_conda_packages(): - """ - Test extra user conda packages are installed - """ - subprocess.check_call([f"{USER_ENV_PREFIX}/bin/python3", "-c", "import hypothesis"]) +def test_tljh_extra_apt_packages(): + assert os.path.exists("/usr/games/sl") -def test_config_hook(): +def test_tljh_custom_jupyterhub_config(): """ - Check config changes are present + Test that the provided tljh_custom_jupyterhub_config hook has made the tljh + jupyterhub load additional jupyterhub config. + """ + tljh_jupyterhub_config = os.path.join(GIT_REPO_PATH, "tljh", "jupyterhub_config.py") + output = subprocess.check_output( + [ + f"{HUB_ENV_PREFIX}/bin/python3", + "-m", + "jupyterhub", + "--show-config", + "--config", + tljh_jupyterhub_config, + ], + text=True, + ) + assert "jupyterhub_config_set_by_simplest_plugin" in output + + +def test_tljh_config_post_install(): + """ + Test that the provided tljh_config_post_install hook has made tljh recognize + additional tljh config. """ with open(CONFIG_FILE) as f: - data = yaml.load(f) - - assert data["simplest_plugin"]["present"] + tljh_config = yaml.load(f) + assert tljh_config["Test"]["tljh_config_set_by_simplest_plugin"] -def test_jupyterhub_config_hook(): +def test_tljh_post_install(): """ - Test that tmpauthenticator is enabled by our custom config plugin + Test that the provided tljh_post_install hook has been executed by looking + for a specific file written. """ - resp = requests.get("http://localhost/hub/tmplogin", allow_redirects=False) - assert resp.status_code == 302 - assert resp.headers["Location"] == "/hub/spawn" - - -def test_post_install_hook(): - """ - Test that the test_post_install file has the correct content - """ - with open("test_post_install") as f: + with open("test_tljh_post_install") as f: content = f.read() - - assert content == "123456789" + assert "file_written_by_simplest_plugin" in content -def test_new_user_create(): +def test_tljh_new_user_create(): """ - Test that plugin receives username as arg + Test that the provided tljh_new_user_create hook has been executed by + looking for a specific file written. """ + # Trigger the hook by letting tljh's code create a user username = "user1" - # Call ensure_user to make sure the user plugin gets called user.ensure_user(username) with open("test_new_user_create") as f: content = f.read() - - assert content == username + assert "file_written_by_simplest_plugin" in content + assert username in content From 111e9bee86d99d0c1cd475deb63f9079b91e70d0 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Thu, 8 Jun 2023 23:35:45 +0200 Subject: [PATCH 16/24] test refactor: combine two separate installation setups --- .github/workflows/integration-test.yaml | 33 ++++++++++++------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/.github/workflows/integration-test.yaml b/.github/workflows/integration-test.yaml index d53eec0..3243692 100644 --- a/.github/workflows/integration-test.yaml +++ b/.github/workflows/integration-test.yaml @@ -83,36 +83,35 @@ jobs: # # - Runs integration-test.py build-image, to build a systemd based image # to use later. + # # - Runs integration-test.py run-tests, to start a systemd based # container, run the bootstrap.py script inside it, and then run # pytest from the hub python environment setup by the bootstrap # script. # + # About passed --installer-args: + # + # - --admin admin:admin + # Required for test_admin_installer.py + # + # - --plugin /srv/src/integration-tests/plugins/simplest + # Required for test_simplest_plugin.py + # - name: pytest integration-tests/ - id: basic-tests + id: integration-tests run: | - .github/integration-test.py run-test basic-tests \ + .github/integration-test.py run-test integration-tests \ + --installer-args "--admin test-admin-username:test-admin-password" \ + --installer-args "--plugin /srv/src/integration-tests/plugins/simplest" \ ${{ matrix.extra_flags }} \ test_hub.py \ test_proxy.py \ test_install.py \ - test_extensions.py - timeout-minutes: 15 - - name: show logs - if: always() && steps.basic-tests.outcome != 'skipped' - run: | - .github/integration-test.py show-logs basic-tests - - - name: pytest integration-tests/test_simplest_plugin.py integration-tests/test_admin_installer.py - id: admin-plugin-tests - run: | - .github/integration-test.py run-test admin-plugin-tests \ - --installer-args "--admin admin:admin --plugin /srv/src/integration-tests/plugins/simplest" \ - ${{ matrix.extra_flags }} \ + test_extensions.py \ test_admin_installer.py \ test_simplest_plugin.py timeout-minutes: 15 - name: show logs - if: always() && steps.admin-plugin-tests.outcome != 'skipped' + if: always() && steps.integration-tests.outcome != 'skipped' run: | - .github/integration-test.py show-logs admin-plugin-tests + .github/integration-test.py show-logs integration-tests From b1c7e53be454c74e484061690675edf863ce0e4a Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 9 Jun 2023 00:20:58 +0200 Subject: [PATCH 17/24] test refactor: try decouple admin tests from other tests --- .github/integration-test.py | 4 ++-- integration-tests/test_admin_installer.py | 8 ++++---- tljh/installer.py | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/integration-test.py b/.github/integration-test.py index 39d27f4..fcb8768 100755 --- a/.github/integration-test.py +++ b/.github/integration-test.py @@ -167,7 +167,7 @@ def run_test( command = f"python3 /srv/src/bootstrap/bootstrap.py --version={upgrade_from}" run_command(container_name, command) - command = f"python3 /srv/src/bootstrap/bootstrap.py {installer_args}" + command = f"python3 /srv/src/bootstrap/bootstrap.py {' '.join(installer_args)}" run_command(container_name, command) # Install pkgs from requirements in hub's pip, where @@ -219,7 +219,7 @@ def main(): copy_parser.add_argument("dest") run_test_parser = subparsers.add_parser("run-test") - run_test_parser.add_argument("--installer-args", default="") + run_test_parser.add_argument("--installer-args", action="append") run_test_parser.add_argument("--upgrade-from", default="") run_test_parser.add_argument("--bootstrap-pip-spec", default="/srv/src") run_test_parser.add_argument("container_name") diff --git a/integration-tests/test_admin_installer.py b/integration-tests/test_admin_installer.py index 88ca9e7..8314bfa 100644 --- a/integration-tests/test_admin_installer.py +++ b/integration-tests/test_admin_installer.py @@ -12,8 +12,8 @@ async def test_admin_login(): Test if the admin that was added during install can login with the password provided. """ - username = "admin" - password = "admin" + username = "test-admin-username" + password = "test-admin-password" async with User(username, hub_url, partial(login_dummy, password=password)) as u: await u.login() @@ -24,8 +24,8 @@ async def test_admin_login(): @pytest.mark.parametrize( "username, password", [ - ("admin", ""), - ("admin", "wrong_passw"), + ("test-admin-username", ""), + ("test-admin-username", "wrong_passw"), ("user", "password"), ], ) diff --git a/tljh/installer.py b/tljh/installer.py index 33cce12..dc787f4 100644 --- a/tljh/installer.py +++ b/tljh/installer.py @@ -284,7 +284,7 @@ def ensure_user_environment(user_requirements_txt_file): def ensure_admins(admin_password_list): """ - Setup given list of users as admins. + Setup given list of user[:password] strings as admins. """ os.makedirs(STATE_DIR, mode=0o700, exist_ok=True) From d1c2e5152541fbd95a99645215f7c9aca2292cf9 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 9 Jun 2023 00:33:41 +0200 Subject: [PATCH 18/24] test refactor: avoid a redundant server startup test --- integration-tests/test_admin_installer.py | 33 ++++++++--------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/integration-tests/test_admin_installer.py b/integration-tests/test_admin_installer.py index 8314bfa..99fdf65 100644 --- a/integration-tests/test_admin_installer.py +++ b/integration-tests/test_admin_installer.py @@ -7,33 +7,22 @@ from hubtraf.user import User hub_url = "http://localhost" -async def test_admin_login(): - """ - Test if the admin that was added during install can login with - the password provided. - """ - username = "test-admin-username" - password = "test-admin-password" - - async with User(username, hub_url, partial(login_dummy, password=password)) as u: - await u.login() - # If user is not logged in, this will raise an exception - await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) - - @pytest.mark.parametrize( - "username, password", + "username, password, expect_successful_login", [ - ("test-admin-username", ""), - ("test-admin-username", "wrong_passw"), - ("user", "password"), + ("test-admin-username", "wrong_passw", False), + ("test-admin-username", "test-admin-password", True), + ("test-admin-username", "", False), + ("user", "", False), + ("user", "password", False), ], ) -async def test_unsuccessful_login(username, password): +async def test_pre_configured_admin_login(username, password, expect_successful_login): """ - Ensure nobody but the admin that was added during install can login + Verify that the "--admin :" flag allows that user/pass + combination and no other user can login. """ - async with User(username, hub_url, partial(login_dummy, password="")) as u: + async with User(username, hub_url, partial(login_dummy, password=password)) as u: user_logged_in = await u.login() - assert user_logged_in == False + assert user_logged_in == expect_successful_login From ec3ee03dc8446daeb462e0d1194a605d604eb7db Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 9 Jun 2023 00:42:07 +0200 Subject: [PATCH 19/24] test refactor: ignore code coverage warning --- pyproject.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index d0e97f8..d256f58 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -41,6 +41,9 @@ target_version = [ [tool.pytest.ini_options] addopts = "--verbose --color=yes --durations=10 --maxfail=1 --cov=tljh" asyncio_mode = "auto" +filterwarnings = [ + 'ignore:.*Module bootstrap was never imported.*:coverage.exceptions.CoverageWarning', +] # pytest-cov / coverage is used to measure code coverage of tests From 3e9ee8ca8f823c4fff415380bf5ab9bdcc0cb5f0 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 9 Jun 2023 01:14:01 +0200 Subject: [PATCH 20/24] test refactor: remove test with broken assumption To remove a user from the admin list doesn't matter, even though its clearly something one may expect should matter. The issue is that JupyterHub itself makes use of the admin_users list as a way to declare new admins, not to remove users having been marked as admin in the past. What to do about this is not clear, but having a test for this doesn't make sense. --- integration-tests/test_hub.py | 67 ----------------------------------- 1 file changed, 67 deletions(-) diff --git a/integration-tests/test_hub.py b/integration-tests/test_hub.py index 502df94..c6f5a57 100644 --- a/integration-tests/test_hub.py +++ b/integration-tests/test_hub.py @@ -163,73 +163,6 @@ async def test_user_admin_add(): assert f"jupyter-{username}" in grp.getgrnam("jupyterhub-admins").gr_mem -# FIXME: Make this test pass -@pytest.mark.xfail(reason="Unclear why this is failing") -async def test_user_admin_remove(): - """ - User is made an admin, logs in and we check if they are in admin group. - - Then we remove them from admin group, and check they *aren't* in admin group :D - """ - # This *must* be localhost, not an IP - # aiohttp throws away cookies if we are connecting to an IP! - username = secrets.token_hex(8) - - assert ( - 0 - == await ( - await asyncio.create_subprocess_exec( - *TLJH_CONFIG_PATH, "set", "auth.type", "dummy" - ) - ).wait() - ) - assert ( - 0 - == await ( - await asyncio.create_subprocess_exec( - *TLJH_CONFIG_PATH, "add-item", "users.admin", username - ) - ).wait() - ) - assert ( - 0 - == await ( - await asyncio.create_subprocess_exec(*TLJH_CONFIG_PATH, "reload") - ).wait() - ) - - async with User(username, hub_url, partial(login_dummy, password="")) as u: - await u.login() - await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) - - # Assert that the user exists - assert pwd.getpwnam(f"jupyter-{username}") is not None - - # Assert that the user has admin rights - assert f"jupyter-{username}" in grp.getgrnam("jupyterhub-admins").gr_mem - - assert ( - 0 - == await ( - await asyncio.create_subprocess_exec( - *TLJH_CONFIG_PATH, "remove-item", "users.admin", username - ) - ).wait() - ) - assert ( - 0 - == await ( - await asyncio.create_subprocess_exec(*TLJH_CONFIG_PATH, "reload") - ).wait() - ) - - await u.stop_server() - await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) - - # Assert that the user does *not* have admin rights - assert f"jupyter-{username}" not in grp.getgrnam("jupyterhub-admins").gr_mem - - async def test_long_username(): """ User with a long name logs in, and we check if their name is properly truncated. From 1d203ccd2a43c6f03d91e059a9291c8a6c8034d2 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 9 Jun 2023 01:48:00 +0200 Subject: [PATCH 21/24] test refactor: reduce time spent waiting on culling/not culling --- integration-tests/test_hub.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/integration-tests/test_hub.py b/integration-tests/test_hub.py index c6f5a57..5066d98 100644 --- a/integration-tests/test_hub.py +++ b/integration-tests/test_hub.py @@ -260,7 +260,7 @@ async def test_user_group_adding(): async def test_idle_server_culled(): """ - User logs in, starts a server & stays idle for 1 min. + User logs in, starts a server & stays idle for a while. (the user's server should be culled during this period) """ username = secrets.token_hex(8) @@ -291,12 +291,12 @@ async def test_idle_server_culled(): ) ).wait() ) - # Cull servers and users after 30s, regardless of activity + # Cull servers and users after a while, regardless of activity assert ( 0 == await ( await asyncio.create_subprocess_exec( - *TLJH_CONFIG_PATH, "set", "services.cull.max_age", "30" + *TLJH_CONFIG_PATH, "set", "services.cull.max_age", "15" ) ).wait() ) @@ -349,7 +349,7 @@ async def test_idle_server_culled(): # Wait for culling # step 1: check if the server is still running - timeout = 100 + timeout = 30 async def server_stopped(): """Has the server been stopped?""" @@ -365,7 +365,7 @@ async def test_idle_server_culled(): # step 2. wait for user to be deleted async def user_removed(): - # Check that after 60s, the user has been culled + # Check that after a while, the user has been culled r = await hub_api_request() print(f"{r.status} {r.url}") return r.status == 403 @@ -379,7 +379,7 @@ async def test_idle_server_culled(): async def test_active_server_not_culled(): """ - User logs in, starts a server & stays idle for 30s + User logs in, starts a server & stays idle for a while (the user's server should not be culled during this period). """ # This *must* be localhost, not an IP @@ -412,12 +412,12 @@ async def test_active_server_not_culled(): ) ).wait() ) - # Cull servers and users after 30s, regardless of activity + # Cull servers and users after a while, regardless of activity assert ( 0 == await ( await asyncio.create_subprocess_exec( - *TLJH_CONFIG_PATH, "set", "services.cull.max_age", "60" + *TLJH_CONFIG_PATH, "set", "services.cull.max_age", "30" ) ).wait() ) @@ -441,7 +441,7 @@ async def test_active_server_not_culled(): assert r.status == 200 async def server_has_stopped(): - # Check that after 30s, we can still reach the user's server + # Check that after a while, we can still reach the user's server r = await u.session.get(user_url, allow_redirects=False) print(f"{r.status} {r.url}") return r.status != 200 @@ -450,7 +450,7 @@ async def test_active_server_not_culled(): await exponential_backoff( server_has_stopped, "User's server is still reachable (good!)", - timeout=30, + timeout=15, ) except asyncio.TimeoutError: # timeout error means the test passed - the server didn't go away while we were waiting From b1822d7098f7fef2d9a01db2c19414672126f85b Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 9 Jun 2023 02:00:30 +0200 Subject: [PATCH 22/24] test refactor: small details in test_hub.py --- integration-tests/test_admin_installer.py | 4 +-- integration-tests/test_hub.py | 39 +++++++++++------------ 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/integration-tests/test_admin_installer.py b/integration-tests/test_admin_installer.py index 99fdf65..3b910cc 100644 --- a/integration-tests/test_admin_installer.py +++ b/integration-tests/test_admin_installer.py @@ -4,7 +4,7 @@ import pytest from hubtraf.auth.dummy import login_dummy from hubtraf.user import User -hub_url = "http://localhost" +HUB_URL = "http://localhost" @pytest.mark.parametrize( @@ -22,7 +22,7 @@ async def test_pre_configured_admin_login(username, password, expect_successful_ Verify that the "--admin :" flag allows that user/pass combination and no other user can login. """ - async with User(username, hub_url, partial(login_dummy, password=password)) as u: + async with User(username, HUB_URL, partial(login_dummy, password=password)) as u: user_logged_in = await u.login() assert user_logged_in == expect_successful_login diff --git a/integration-tests/test_hub.py b/integration-tests/test_hub.py index 5066d98..55a35cd 100644 --- a/integration-tests/test_hub.py +++ b/integration-tests/test_hub.py @@ -21,16 +21,16 @@ TLJH_CONFIG_PATH = ["sudo", "tljh-config"] # This *must* be localhost, not an IP # aiohttp throws away cookies if we are connecting to an IP! -hub_url = "http://localhost" +HUB_URL = "http://localhost" def test_hub_up(): - r = requests.get(hub_url) + r = requests.get(HUB_URL) r.raise_for_status() def test_hub_version(): - r = requests.get(hub_url + "/hub/api") + r = requests.get(HUB_URL + "/hub/api") r.raise_for_status() info = r.json() assert V("4") <= V(info["version"]) <= V("5") @@ -57,15 +57,12 @@ async def test_user_code_execute(): ).wait() ) - async with User(username, hub_url, partial(login_dummy, password="")) as u: - await u.login() + async with User(username, HUB_URL, partial(login_dummy, password="")) as u: + assert await u.login() await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) await u.start_kernel() await u.assert_code_output("5 * 4", "20", 5, 5) - # Assert that the user exists - assert pwd.getpwnam(f"jupyter-{username}") is not None - async def test_user_server_started_with_custom_base_url(): """ @@ -74,7 +71,7 @@ async def test_user_server_started_with_custom_base_url(): # This *must* be localhost, not an IP # aiohttp throws away cookies if we are connecting to an IP! base_url = "/custom-base" - hub_url = f"http://localhost{base_url}" + custom_hub_url = f"{HUB_URL}{base_url}" username = secrets.token_hex(8) assert ( @@ -100,8 +97,8 @@ async def test_user_server_started_with_custom_base_url(): ).wait() ) - async with User(username, hub_url, partial(login_dummy, password="")) as u: - await u.login() + async with User(username, custom_hub_url, partial(login_dummy, password="")) as u: + assert await u.login() await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # unset base_url to avoid problems with other tests @@ -152,8 +149,8 @@ async def test_user_admin_add(): ).wait() ) - async with User(username, hub_url, partial(login_dummy, password="")) as u: - await u.login() + async with User(username, HUB_URL, partial(login_dummy, password="")) as u: + assert await u.login() await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists @@ -185,8 +182,8 @@ async def test_long_username(): ) try: - async with User(username, hub_url, partial(login_dummy, password="")) as u: - await u.login() + async with User(username, HUB_URL, partial(login_dummy, password="")) as u: + assert await u.login() await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists @@ -238,8 +235,8 @@ async def test_user_group_adding(): ) try: - async with User(username, hub_url, partial(login_dummy, password="")) as u: - await u.login() + async with User(username, HUB_URL, partial(login_dummy, password="")) as u: + assert await u.login() await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists @@ -307,9 +304,9 @@ async def test_idle_server_culled(): ).wait() ) - async with User(username, hub_url, partial(login_dummy, password="")) as u: + async with User(username, HUB_URL, partial(login_dummy, password="")) as u: # Login the user - await u.login() + assert await u.login() # Start user's server await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) @@ -428,8 +425,8 @@ async def test_active_server_not_culled(): ).wait() ) - async with User(username, hub_url, partial(login_dummy, password="")) as u: - await u.login() + async with User(username, HUB_URL, partial(login_dummy, password="")) as u: + assert await u.login() # Start user's server await u.ensure_server_simulate(timeout=60, spawn_refresh_time=5) # Assert that the user exists From b02a8b044f0ee497b243c6d7c4660437317b92d4 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 9 Jun 2023 02:55:06 +0200 Subject: [PATCH 23/24] test refactor: mitigate persisted state between tests --- integration-tests/test_admin_installer.py | 34 +++++++++++++++++++++-- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/integration-tests/test_admin_installer.py b/integration-tests/test_admin_installer.py index 3b910cc..7eb2533 100644 --- a/integration-tests/test_admin_installer.py +++ b/integration-tests/test_admin_installer.py @@ -1,20 +1,48 @@ +import asyncio from functools import partial import pytest from hubtraf.auth.dummy import login_dummy from hubtraf.user import User +# Use sudo to invoke it, since this is how users invoke it. +# This catches issues with PATH +TLJH_CONFIG_PATH = ["sudo", "tljh-config"] + +# This *must* be localhost, not an IP +# aiohttp throws away cookies if we are connecting to an IP! HUB_URL = "http://localhost" +# FIXME: Other tests may have set the auth.type to dummy, so we reset it here to +# get the default of firstuseauthenticator. Tests should cleanup after +# themselves to a better degree, but its a bit trouble to reload the +# jupyterhub between each test as well if thats needed... +async def test_restore_relevant_tljh_state(): + assert ( + 0 + == await ( + await asyncio.create_subprocess_exec( + *TLJH_CONFIG_PATH, + "set", + "auth.type", + "firstuseauthenticator.FirstUseAuthenticator", + ) + ).wait() + ) + assert ( + 0 + == await ( + await asyncio.create_subprocess_exec(*TLJH_CONFIG_PATH, "reload") + ).wait() + ) + + @pytest.mark.parametrize( "username, password, expect_successful_login", [ - ("test-admin-username", "wrong_passw", False), ("test-admin-username", "test-admin-password", True), - ("test-admin-username", "", False), ("user", "", False), - ("user", "password", False), ], ) async def test_pre_configured_admin_login(username, password, expect_successful_login): From cb200d9702956133415bf420bee4024b699c8b52 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 9 Jun 2023 03:40:36 +0200 Subject: [PATCH 24/24] test refactor: comment about test_bootstrap.py --- .github/workflows/integration-test.yaml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/integration-test.yaml b/.github/workflows/integration-test.yaml index 3243692..905f996 100644 --- a/.github/workflows/integration-test.yaml +++ b/.github/workflows/integration-test.yaml @@ -59,7 +59,14 @@ jobs: with: python-version: "3.10" - - name: Install integration-tests/requirements.txt + # FIXME: The test_bootstrap.py script has duplicated logic to run build + # and start images and run things in them. This makes tests slower, + # and adds code to maintain. Let's try to remove it. + # + # - bootstrap.py's failure detections, put in unit tests? + # - bootstrap.py's --show-progress-page test, include as integration test? + # + - name: Install integration-tests/requirements.txt for test_bootstrap.py run: pip install -r integration-tests/requirements.txt - name: Run bootstrap tests (Runs in/Builds ${{ matrix.distro_image }} derived image)