jupyter / nb2kg

Other
73 stars 31 forks source link

Preferred framework for unit/integration tests? #40

Closed dleen closed 5 years ago

dleen commented 5 years ago

Hi,

I was planning on helping out by adding some unit and integration tests to this package. Do you have any preferences or advice?

I noticed that one type of integration test uses NotebookTestBase from Jupyter notebook: https://github.com/jupyter/notebook/blob/d17caf92c0817e8bc771209a1e867f0c73e34724/notebook/tests/launchnotebook.py#L39. An example of such a test is: https://github.com/jupyter/notebook/blob/d17caf92c0817e8bc771209a1e867f0c73e34724/notebook/services/kernelspecs/tests/test_kernelspecs_api.py#L72. Here they start a test server using NotebookTestBase and make a API call against the local server.

The other uses Tornado testing tools. For example the base class uses AsyncHTTPTestCase: https://github.com/jupyter/kernel_gateway/blob/master/kernel_gateway/tests/test_gatewayapp.py#L82 and the test itself: https://github.com/jupyter/kernel_gateway/blob/master/kernel_gateway/tests/test_notebook_http.py#L57

kevin-bates commented 5 years ago

Hi David, thanks for the issue.

Yes, the lack of tests are glaring. However, now that Notebook 6.0 is out, I'm not sure how much value this would have. The reason I say that is that Notebook 6 embeds the nb2kg extension directly into the server. This enables the ability to simply start Notebook with --gateway-url <url> and skip its installation, extension enablement and class overrides completely. In addition, I added tests to Notebook when I added the gateway package: https://github.com/jupyter/notebook/blob/d17caf92c0817e8bc771209a1e867f0c73e34724/notebook/tests/test_gateway.py. As a result, this repo is essentially in down-level maintenance mode.

If you'd like to still add tests, I suppose using the framework from that used in notebook (as well as those tests themselves) would be a good starting point. But, yeah, we should update this repo to reflect its (diminished) status.

dleen commented 5 years ago

Hi Kevin,

Thanks for the info! I don't know how I missed that it's included in the notebook now.

I sent a PR to update the README.