neaten service test management

- cleanup services after each test
- more fixtures for services

jupyterhub/tests/conftest.py

@@ -167,6 +167,8 @@ async def cleanup_after(request, io_loop):
         app = MockHub.instance()
         if app.db_file.closed:
             return
+
+        # cleanup users
         for uid, user in list(app.users.items()):
             for name, spawner in list(user.spawners.items()):
                 if spawner.active:
@@ -181,6 +183,16 @@ async def cleanup_after(request, io_loop):
         # delete groups
         for group in app.db.query(orm.Group):
             app.db.delete(group)
+
+        # clear services
+        for name, service in app._service_map.items():
+            if service.managed:
+                service.stop()
+        for orm_service in app.db.query(orm.Service):
+            if orm_service.oauth_client:
+                app.oauth_provider.remove_client(orm_service.oauth_client_id)
+            app.db.delete(orm_service)
+        app._service_map.clear()
         app.db.commit()
 
 
@@ -262,10 +274,7 @@ class MockServiceSpawner(jupyterhub.services.service._ServiceSpawner):
     poll_interval = 1
 
 
-_mock_service_counter = 0
+async def _mockservice(request, app, name, external=False, url=False):
-
-
-async def _mockservice(request, app, external=False, url=False):
     """
     Add a service to the application
 
@@ -281,9 +290,6 @@ async def _mockservice(request, app, external=False, url=False):
           If True, register the service at a URL
           (as opposed to headless, API-only).
     """
-    global _mock_service_counter
-    _mock_service_counter += 1
-    name = 'mock-service-%i' % _mock_service_counter
     spec = {'name': name, 'command': mockservice_cmd, 'admin': True}
     if url:
         if app.internal_ssl:
@@ -330,22 +336,33 @@ async def _mockservice(request, app, external=False, url=False):
     return service
 
 
+_service_name_counter = 0
+
+
 @fixture
-async def mockservice(request, app):
+def service_name():
+    global _service_name_counter
+    _service_name_counter += 1
+    name = f'test-service-{_service_name_counter}'
+    return name
+
+
+@fixture
+async def mockservice(request, app, service_name):
     """Mock a service with no external service url"""
-    yield await _mockservice(request, app, url=False)
+    yield await _mockservice(request, app, name=service_name, url=False)
 
 
 @fixture
-async def mockservice_external(request, app):
+async def mockservice_external(request, app, service_name):
     """Mock an externally managed service (don't start anything)"""
-    yield await _mockservice(request, app, external=True, url=False)
+    yield await _mockservice(request, app, name=service_name, external=True, url=False)
 
 
 @fixture
-async def mockservice_url(request, app):
+async def mockservice_url(request, app, service_name):
     """Mock a service with its own url to test external services"""
-    yield await _mockservice(request, app, url=True)
+    yield await _mockservice(request, app, name=service_name, url=True)
 
 
 @fixture
@@ -537,12 +554,14 @@ def pytest_terminal_summary(terminalreporter, exitstatus, config):
 
 
 @fixture
-def service_data():
+def service_data(service_name):
     """Data used to create service at runtime"""
     return {
-        "oauth_client_id": "service-oauth-client-from-api",
+        "name": service_name,
-        "api_token": "api_token-from-api",
+        "oauth_client_id": f"service-{service_name}",
+        "api_token": f"api_token-{service_name}",
         "oauth_redirect_uri": "http://127.0.0.1:5555/oauth_callback-from-api",
         "oauth_no_confirm": True,
+        "oauth_client_allowed_scopes": ["inherit"],
         "info": {'foo': 'bar'},
     }

jupyterhub/tests/test_api.py

@@ -2099,9 +2099,8 @@ def service_admin_user(create_user_with_scopes):
 
 
 @mark.services
-async def test_create_service(app, service_admin_user, service_data):
+async def test_create_service(app, service_admin_user, service_name, service_data):
     db = app.db
-    service_name = 'service-from-api'
     r = await api_request(
         app,
         f'services/{service_name}',
@@ -2131,9 +2130,8 @@ async def test_create_service(app, service_admin_user, service_data):
 
 
 @mark.services
-async def test_create_service_no_role(app, service_data):
+async def test_create_service_no_role(app, service_name, service_data):
     db = app.db
-    service_name = 'service-from-api'
     r = await api_request(
         app,
         f'services/{service_name}',
@@ -2146,9 +2144,10 @@ async def test_create_service_no_role(app, service_data):
 
 
 @mark.services
-async def test_create_service_conflict(app, service_admin_user, service_data):
+async def test_create_service_conflict(
+    app, service_admin_user, mockservice, service_data, service_name
+):
     db = app.db
-    service_name = 'service-from-config'
     app.services = [{'name': service_name}]
     app.init_services()
 
@@ -2164,9 +2163,19 @@ async def test_create_service_conflict(app, service_admin_user, service_data):
 
 
 @mark.services
-async def test_create_service_duplication(app, service_admin_user, service_data):
+async def test_create_service_duplication(
+    app, service_admin_user, service_name, service_data
+):
     db = app.db
-    service_name = 'service-from-api'
+
+    r = await api_request(
+        app,
+        f'services/{service_name}',
+        headers=auth_header(db, service_admin_user.name),
+        data=json.dumps(service_data),
+        method='post',
+    )
+    assert r.status_code == 201
 
     r = await api_request(
         app,
@@ -2179,9 +2188,10 @@ async def test_create_service_duplication(app, service_admin_user, service_data)
 
 
 @mark.services
-async def test_create_managed_service(app, service_admin_user, service_data):
+async def test_create_managed_service(
+    app, service_admin_user, service_name, service_data
+):
     db = app.db
-    service_name = 'managed-service-from-api'
     managed_service_data = deepcopy(service_data)
     managed_service_data['command'] = ['foo']
     r = await api_request(
@@ -2199,9 +2209,8 @@ async def test_create_managed_service(app, service_admin_user, service_data):
 
 
 @mark.services
-async def test_create_admin_service(app, admin_user, service_data):
+async def test_create_admin_service(app, admin_user, service_name, service_data):
     db = app.db
-    service_name = 'admin-service-from-api'
     managed_service_data = deepcopy(service_data)
     managed_service_data['admin'] = True
     r = await api_request(
@@ -2219,10 +2228,9 @@ async def test_create_admin_service(app, admin_user, service_data):
 
 @mark.services
 async def test_create_admin_service_without_admin_right(
-    app, service_admin_user, service_data
+    app, service_admin_user, service_data, service_name
 ):
     db = app.db
-    service_name = 'managed-service-from-api'
     managed_service_data = deepcopy(service_data)
     managed_service_data['admin'] = True
     r = await api_request(
@@ -2240,9 +2248,10 @@ async def test_create_admin_service_without_admin_right(
 
 
 @mark.services
-async def test_create_service_with_scope(app, create_user_with_scopes, service_data):
+async def test_create_service_with_scope(
+    app, create_user_with_scopes, service_name, service_data
+):
     db = app.db
-    service_name = 'service-with-scope'
     managed_service_data = deepcopy(service_data)
     managed_service_data['oauth_client_allowed_scopes'] = ["admin:users"]
     managed_service_data['oauth_client_id'] = "service-client-with-scope"
@@ -2262,10 +2271,12 @@ async def test_create_service_with_scope(app, create_user_with_scopes, service_d
 
 @mark.services
 async def test_create_service_without_requested_scope(
-    app, service_admin_user, service_data
+    app,
+    service_admin_user,
+    service_data,
+    service_name,
 ):
     db = app.db
-    service_name = 'service-without-requested-scope'
     managed_service_data = deepcopy(service_data)
     managed_service_data['oauth_client_allowed_scopes'] = ["admin:users"]
     r = await api_request(
@@ -2283,9 +2294,16 @@ async def test_create_service_without_requested_scope(
 
 
 @mark.services
-async def test_remove_service(app, service_admin_user, service_data):
+async def test_remove_service(app, service_admin_user, service_name, service_data):
     db = app.db
-    service_name = 'service-from-api'
+    r = await api_request(
+        app,
+        f'services/{service_name}',
+        headers=auth_header(db, service_admin_user.name),
+        data=json.dumps(service_data),
+        method='post',
+    )
+    assert r.status_code == 201
 
     r = await api_request(
         app,
@@ -2309,9 +2327,9 @@ async def test_remove_service(app, service_admin_user, service_data):
 
 
 @mark.services
-async def test_remove_service_from_config(app, service_admin_user):
+async def test_remove_service_from_config(app, service_admin_user, mockservice):
     db = app.db
-    service_name = 'service-from-config'
+    service_name = mockservice.name
     r = await api_request(
         app,
         f'services/{service_name}',
@@ -2319,15 +2337,10 @@ async def test_remove_service_from_config(app, service_admin_user):
         method='delete',
     )
     assert r.status_code == 405
-    assert (
+    assert r.json()['message'] == f'Service {service_name} is not modifiable at runtime'
-        r.json()['message']
-        == 'Service service-from-config is not modifiable at runtime'
-    )
 
 
 async def test_root_api(app):
-    base_url = app.hub.url
-    url = ujoin(base_url, 'api')
     kwargs = {}
     if app.internal_ssl:
         kwargs['cert'] = (app.internal_ssl_cert, app.internal_ssl_key)

jupyterhub/tests/test_app.py

@@ -15,11 +15,10 @@ import pytest
 import traitlets
 from traitlets.config import Config
 
-from .. import orm, roles
+from .. import orm
 from ..app import COOKIE_SECRET_BYTES, JupyterHub
 from .mocking import MockHub
 from .test_api import add_user
-from .utils import api_request, auth_header
 
 
 def test_help_all():
@@ -221,7 +220,7 @@ def test_cookie_secret_env(tmpdir, request):
     assert not os.path.exists(hub.cookie_secret_file)
 
 
-def test_cookie_secret_string_():
+def test_cookie_secret_string():
     cfg = Config()
 
     cfg.JupyterHub.cookie_secret = "not hex"
@@ -271,18 +270,41 @@ async def test_load_groups(tmpdir, request):
     )
 
 
-async def test_resume_spawners(tmpdir, request):
+@pytest.fixture
-    if not os.getenv('JUPYTERHUB_TEST_DB_URL'):
+def persist_db(tmpdir):
-        p = patch.dict(
+    """ensure db will persist (overrides default sqlite://:memory:)"""
+    if os.getenv('JUPYTERHUB_TEST_DB_URL'):
+        # already using a db, no need
+        yield
+        return
+    with patch.dict(
         os.environ,
-            {
+        {'JUPYTERHUB_TEST_DB_URL': f"sqlite:///{tmpdir.join('jupyterhub.sqlite')}"},
-                'JUPYTERHUB_TEST_DB_URL': 'sqlite:///%s'
+    ):
-                % tmpdir.join('jupyterhub.sqlite')
+        yield
-            },
-        )
-        p.start()
-        request.addfinalizer(p.stop)
 
+
+@pytest.fixture
+def new_hub(request, tmpdir, persist_db):
+    """Fixture to launch a new hub for testing"""
+
+    async def new_hub():
+        kwargs = {}
+        ssl_enabled = getattr(request.module, "ssl_enabled", False)
+        if ssl_enabled:
+            kwargs['internal_certs_location'] = str(tmpdir)
+        app = MockHub(test_clean_db=False, **kwargs)
+        app.config.ConfigurableHTTPProxy.should_start = False
+        app.config.ConfigurableHTTPProxy.auth_token = 'unused'
+        request.addfinalizer(app.stop)
+        await app.initialize([])
+
+        return app
+
+    return new_hub
+
+
+async def test_resume_spawners(tmpdir, request, new_hub):
     async def new_hub():
         kwargs = {}
         ssl_enabled = getattr(request.module, "ssl_enabled", False)
@@ -476,100 +498,26 @@ async def test_user_creation(tmpdir, request):
     }
 
 
-@pytest.fixture(scope='module')
+async def test_recreate_service_from_database(
-def db_temp_path(tmp_path_factory):
+    request, new_hub, service_name, service_data
-    fn = tmp_path_factory.mktemp("db") / "jupyterhub.sqlite"
+):
-    return fn
+    # create a hub and add a service (not from config)
-
+    app = await new_hub()
-
+    app.service_from_spec(service_data, from_config=False)
-async def test_add_service_at_runtime(request, db_temp_path, service_data):
-    if not os.getenv('JUPYTERHUB_TEST_DB_URL'):
-        p = patch.dict(
-            os.environ,
-            {'JUPYTERHUB_TEST_DB_URL': 'sqlite:///%s' % str(db_temp_path)},
-        )
-        p.start()
-        request.addfinalizer(p.stop)
-
-    kwargs = {"test_clean_db": False}
-    ssl_enabled = getattr(request.module, "ssl_enabled", False)
-    if ssl_enabled:
-        kwargs['internal_certs_location'] = db_temp_path.parents[0]
-    app = MockHub(**kwargs)
-
-    def end():
-        app.log.handlers = []
-        MockHub.clear_instance()
-        try:
     app.stop()
-        except Exception as e:
-            print("Error stopping Hub: %s" % e, file=sys.stderr)
 
-    request.addfinalizer(end)
+    # new hub, should load service from db
+    app = await new_hub()
+    assert service_name in app._service_map
 
-    await app.initialize([])
+    # verify keys
-    await app.start()
+    service = app._service_map[service_name]
-    db = app.db
+    for key, value in service_data.items():
+        if key in {'api_token'}:
+            # skip some keys
+            continue
+        assert getattr(service, key) == value
 
-    user_name = 'admin_services'
-    service_role = {
-        'name': 'admin-services-role',
-        'description': '',
-        'users': [user_name],
-        'scopes': ['admin:services'],
-    }
-    roles.create_role(app.db, service_role)
-    user = add_user(app.db, name=user_name)
-    roles.update_roles(app.db, user, roles=['admin-services-role'])
-
-    service_name = 'service-from-api'
-
-    r = await api_request(
-        app,
-        f'services/{service_name}',
-        headers=auth_header(db, user_name),
-        data=json.dumps(service_data),
-        method='post',
-    )
-    assert r.status_code == 201
-    assert r.json()['name'] == service_name
-    oauth_client = (
-        app.db.query(orm.OAuthClient)
-        .filter_by(identifier=service_data['oauth_client_id'])
-        .first()
-    )
-    assert oauth_client.redirect_uri == service_data['oauth_redirect_uri']
-
-
-async def test_recreate_service_from_database(request, db_temp_path, service_data):
-    if not os.getenv('JUPYTERHUB_TEST_DB_URL'):
-        p = patch.dict(
-            os.environ,
-            {'JUPYTERHUB_TEST_DB_URL': 'sqlite:///%s' % str(db_temp_path)},
-        )
-        p.start()
-        request.addfinalizer(p.stop)
-    kwargs = {"test_clean_db": False}
-
-    ssl_enabled = getattr(request.module, "ssl_enabled", False)
-    if ssl_enabled:
-        kwargs['internal_certs_location'] = db_temp_path.parents[0]
-    app = MockHub(**kwargs)
-
-    def end():
-        app.log.handlers = []
-        MockHub.clear_instance()
-        try:
-            app.stop()
-        except Exception as e:
-            print("Error stopping Hub: %s" % e, file=sys.stderr)
-
-    request.addfinalizer(end)
-
-    await app.initialize([])
-    await app.start()
-
-    assert 'service-from-api' in app._service_map
     assert (
         service_data['oauth_client_id'] in app.tornado_settings['oauth_no_confirm_list']
     )
@@ -579,3 +527,11 @@ async def test_recreate_service_from_database(request, db_temp_path, service_dat
         .first()
     )
     assert oauth_client.redirect_uri == service_data['oauth_redirect_uri']
+
+    # delete service from db, start one more
+    app.db.delete(service.orm)
+    app.db.commit()
+
+    # start one more, service should be gone
+    app = await new_hub()
+    assert service_name not in app._service_map