Experiment: Configure test suite to use Jupyterhub Pytest Plugin

[Sheila-nk]

This pull request is a demonstration of how the JupyterHub Pytest Plugin specifically the hub_app fixture can be used in the DockerSpawner repository.
We achieved this through the following steps:

• installed the JupyterHub pytest plugin through pip
• loaded the jupyterhub_spawners_plugin in the conftest.py file
• used the plugin's hub_app fixture in place of the JupyterHub app fixture

[Sheila-nk]

Ping @GeorgianaElena I am getting the same errors here as well.
Trying to go through the logs and all the errors take this form:

sqlalchemy.exc.ArgumentError: Column expression, FROM clause, or other columns clause element expected, got [1]. Did you mean to say select(1)?

This seems to happen at the point the app is trying to connect to the database.
Maybe you can provide more insight on this.

[GeorgianaElena]

Hmm, looks like the tests error because of jupyterhub/jupyterhub#4312

Due to this, I believe the next steps are to:

• Open an issue in dockerspawner asking for feedback from the community about the best way to fix the sqalchemy issue, with the following context:

  • jupyterhub backported the sqalchemy compatibility fix to jupyterhub>=3.0
  • but it didn't pin sqalchemy to < 2.0.0 for jupyterhub<3.0
  • in dockerspawner we can go around this by either:
    • dropping support for jupyterhub < 3.0
    • pin sqalchemy to < 2.0.0 for jupyterhub<3.0 in either jupyterhub or dockerspawner

• 2. To not block this work on the first task ⬆️ add a new test strategy to the matrix to test against jupyterhub 3.1.1 that has fixed the sqlalchemy compatibility issue.
  

  dockerspawner/.github/workflows/test.yml

  Lines 19 to 20 in 8503af6

  	matrix:
  	include:

  
  and only care about that tests while trying to integrate the plugin into dockerspawner.

[manics]

I think it's reasonable to only support supported JupyterHub versions, it's always possible for someone to install an older version if they're still running an older JupyterHub version.

[GeorgianaElena]

2. To not block this work on the first task ⬆️ add a new test strategy to the matrix to test against jupyterhub 3.1.1 that has fixed the sqlalchemy compatibility issue.

@Sheila-nk, for point 2, I suggest opening a new PR, separate than this one since that is beneficial outside of this context too.

I think it's reasonable to only support supported JupyterHub versions, it's always possible for someone to install an older version if they're still running an older JupyterHub version.

@manics, I agree 👍🏼 I suggested @Sheila-nk to propose this in #477 rather than have that be a bug report

[Sheila-nk]

@GeorgianaElena
I've been trying to figure out the reason why the event loop is closing prematurely. From a bit of research, the reasons can be:

• a blocking non-async task
• exceptions are not handled properly
• leaked resources
• some async functions are not being awaited properly

With exceptions, I have tried to use the try/finally block to ensure the cleanup is always done even if an exception happens, but that did not solve the problem. Like this:

@pytest.fixture
async def hub_app(configured_mockhub_instance):
     # Function to create hub instance
    async def _create_hub_app(config={}):
        app = configured_mockhub_instance(config)
        await app.initialize()
        await app.start()

        return app

    # Function to clean up the hub instance and resources
    async def _cleanup_hub_instance():
        app = MockHub.instance()
        app.log.handlers = []
        try:
            if app.http_server:
                app.http_server.stop()
            await app.shutdown_cancel_tasks()
        except Exception as e:
            print("Error stopping Hub: %s" % e, file=sys.stderr)
        finally:
            MockHub.clear_instance()

    try:
        yield _create_hub_app
    finally:
        await _cleanup_hub_instance()

Will continue looking into it.

[GeorgianaElena]

@Sheila-nk, before analyzing this in more detail, I believe first thing should be to remove all imports from jupyterhub's conftest that are no longer relevant (event_loop, ssl_tmpdir, app, etc) to make sure things don't get mingled again and end up with different event loops and fixture mismatch.

Prior, pytest needed all the fixtures that the app fixture depended on to also be imported, event though they weren't all explicitly used in the dockerspawner tests. This is because an import translates into "the fixture is defined in this file" for pytest. Hence, if app was "defined" in dockerspaner conftest, all the other fixtures that app depended on, also needed to be imported in order for pytest to be able to find them.

But this is not the case anymore with pytest plugins, so we can safely get rid of those imports.

The "Fixture availabilty` section of the pytest docs provides great info and visual of this fixture execution and load order, or overrinding and even how plugin come into picture https://docs.pytest.org/en/latest/reference/fixtures.html#fixture-availability

[Sheila-nk]

@Sheila-nk, before analyzing this in more detail, I believe first thing should be to remove all imports from jupyterhub's conftest that are no longer relevant (event_loop, ssl_tmpdir, app, etc) to make sure things don't get mingled again and end up with different event loops and fixture mismatch.

😄 I am so annoyed! I spent hours trying to find a solution to the event loop closing and all I needed to do was remove the fixture imports we don't need anymore.

Thank you, @GeorgianaElena. The tests using JupyterHub v3.1.1 are all passing. 🚀
I will now pin sqlalchemy to 1.4 on this draft PR to ensure the plugin works for all JupyterHub versions.

[Sheila-nk]

@minrk I think we might need to install pytest-jupyterhub from GitHub so the changes from the merged PR jupyterhub/pytest-jupyterhub#62 can reflect here.