Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace old nose tests with pytest based tests #152

Merged
merged 35 commits into from
Dec 6, 2019

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Dec 4, 2019

Okay, I've gone through every test and rewritten it using only pytest's fixture design/syntax.

This also uses the pytest-tornasync to enable an async/await syntax for tests that hit the server REST API.

One cool thing about these pytest fixtures is that the conftest.py offers some really nice fixtures that we could generalize for anyone writing a server extension (i.e. jupyterlab, voila, etc.). These fixtures can be used to easily spin up a configurable test ServerApp. Some helpful fixtures include:

  • serverapp : a vanilla running server app.
  • configurable_serverapp: a configurable (via argv or a config dict) server that will start running after configurable.
  • app: the server web application
  • fetch: a function that hits the serverapp's REST API.

@kevin-bates, @echarles, and @maartenbreddels

@Zsailer
Copy link
Member Author

Zsailer commented Dec 4, 2019

This is a massive PR. If the tests pass, I propose we merge and address problems as they arise.

@Zsailer
Copy link
Member Author

Zsailer commented Dec 4, 2019

@kevin-bates One of the gateway tests is failing. Here's the error I get. Do you have any immediate ideas?

______________________ test_gateway_get_named_kernelspec _______________________

init_gateway = None
fetch = <function fetch.<locals>.client_fetch at 0x1078de0d0>

    async def test_gateway_get_named_kernelspec(init_gateway, fetch):
        # Validate that a specific kernelspec can be retrieved from gateway (and an invalid spec can't)
        with mocked_gateway:
            r = await fetch(
                'api', 'kernelspecs', 'kspec_foo',
                method='GET'
            )
            assert r.code == 200
            kspec_foo = json.loads(r.body.decode('utf-8'), encoding='utf-8')
            assert kspec_foo.get('name') == 'kspec_foo'
    
            with pytest.raises(tornado.httpclient.HTTPClientError) as e:
                await fetch(
                    'api', 'kernelspecs', 'no_such_spec',
                    method='GET'
                )
>           assert expected_http_error(e, 404)
E           assert False
E            +  where False = expected_http_error(<ExceptionInfo HTTP 500: Internal Server Error tblen=1>, 404)

@Zsailer
Copy link
Member Author

Zsailer commented Dec 4, 2019

The 3.5 tests are failing due to a pathlib error.

@kevin-bates
Copy link
Member

@Zsailer - regarding the gateway test failure. I need to look into this. This doesn't occur when the tests are run from a separate location (as we were doing with the juptyer_server_tests repo). Only when collocated.

@kevin-bates
Copy link
Member

The gateway test failure was due to the fact that the test rewrite was based on the test code prior to PR #139. Since we were running tests against an older branch of jupyter_server, all was good. Once this PR was created, however, it introduced a new server code vs. old test code. The changes are only to the import statements surrounding the HTTPError (mess). Once I updated the reworked test with the updated imports from #139 the test passed. The updates to be merged into the pytest branch can be found here: Zsailer#1

Note that in either case (before or after my fix), the travis tests aren't running at all. With the change, all github actions pass (except for 3.8 on Windows - which we know about) and travis (where no tests are running).

@Zsailer
Copy link
Member Author

Zsailer commented Dec 5, 2019

@kevin-bates
Copy link
Member

@echarles and I just finished looking into the Windows 3.8 issues and the issue appears to be related to using the pytest-tornasync plugin. In there, it sets up an io_loop fixture so that it can ensure a new loop is setup for each test.

Having added print statements, it was determined that the patch in jupyter/notebook#5047 was establishing the correct event loop, even within each test, but the pytest-tornasync fixtures must be coming into play after that. Since we had already setup a asyncio_patch fixture, we found that overriding the plugin's io_loop fixture with a dependency on the asyncio_patch fixture proper behavior was exhibited. Here's the change...

@pytest.fixture
def asyncio_patch():
    _init_asyncio_patch()


@pytest.fixture
def io_loop(asyncio_patch):
    """
    Create new io loop for each test, and tear it down after.
    """
    loop = tornado.ioloop.IOLoop()
    loop.make_current()
    yield loop
    loop.clear_current()
    loop.close(all_fds=True)

I'm sure there's probably a better way to do this, but I also suspect this is the idea behind fixtures (last override wins). Still not sure why this was necessary since that patch's installment of the SelectorEventLoopPolicy should have taken place already.



@pytest.fixture
def io_loop(asyncio_patch):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be a good idea to add a comment here that this fixture (sans the asyncio_patch reference) comes directly from plugin pytest-tornasync and that we need to override fixture io_loop in order to ensure the appropriate event loop policy is in place and that these (asyncio_patch and this io_loop) fixtures can be removed once the ProactorEventLoop works correctly on Windows (and pytlhon 3.8+).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Zsailer
Copy link
Member Author

Zsailer commented Dec 6, 2019

ALL GREEN. 🎉

@echarles or @kevin-bates you do the honors!

@Zsailer
Copy link
Member Author

Zsailer commented Dec 6, 2019

@echarles told me to merge it!

@Zsailer Zsailer merged commit 6c88b31 into jupyter-server:master Dec 6, 2019
@echarles
Copy link
Member

echarles commented Dec 6, 2019

Congrats @Zsailer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants