jupyter-book / thebe

Turn static HTML pages into live documents with Jupyter kernels.
https://thebe.readthedocs.io
BSD 3-Clause "New" or "Revised" License
393 stars 68 forks source link

improve test coverage #228

Open minrk opened 4 years ago

minrk commented 4 years ago

as in...any test coverage

The only tests we currently have in this repo are that the bundle builds at all. Not a big guarantee of success! We should have real tests that exercise putting thebe on the page, requesting kernels, running code, etc.; I just haven't had a chance to set this up. @blink1073 mentioned that jupyterlab has some helper packages for testing.

It would be awesome to get some pointers from @jupyterlab folks on test scaffolding for a jupyterlab-based browser package. We can probably also look at the right subset of their tests for reference.

Basic things to test, especially with known issues we've had:

blink1073 commented 4 years ago

We have a testutils package that has among other things:

It also provides some standard jest config, that we use in our packages.

For end-to-end testing, we currently use puppeteer to launch the application, wait for it to load, and make sure that no console errors or uncaught JavaScript errors are thrown. The app is run from a python entry point.

We have better e2e test support coming via Galata

Happy to meet sometime next week to discuss any of the above at a Norway-friendly time 😉

minrk commented 4 years ago

@blink1073 that's awesome. Do you have time maybe next week, Wednesday or later? I tried for a while to get started with jest in #236, but failed to figure out webpack+browser+jest. I did end up getting it working with mocha+karma, but I think I recall jupyterlab used to use that and there's presumably a reason you folks switched to what you use now.

blink1073 commented 4 years ago

Sounds good, what time is good for you on Wednesday? I don't have anything until the Notebook meeting at 1530 UTC.

stevejpurves commented 3 years ago

I spent a little time looking around the test code which uses karma here so runs in browser, whcih is a bit at odds with the jest based setup used over in the debugger repo.

@blink1073 do we expect the JS wrapper for the Jupyter Server to be run in node? and we'd run that independent of the karma tests (maybe even using something like this to start the server up)?

stevejpurves commented 3 years ago

I've poked at this a bit more in here: https://github.com/executablebooks/thebe/pull/282

@minrk is there are strong reason to stay with karma.js rather than switch to jest. i.e. is testing in headless chrome is a requirement over using jsdom in jest?

There are advantages of switching to jest:

If jest is ok, I could flip what we have not from karna to jest...

minrk commented 3 years ago

@stevejpurves the only reason is that I couldn't get it to work. I tried and failed. If you can get it to work, please go for it!

Miniland1333 commented 3 years ago

@stevejpurves I'm trying out Jest based on the previous conversation you had here and it does seem to run faster. How difficult do you think it would be to write a copy of your tests in Jest as you were proposing?

I've swapped my new tests for readonly to Jest and it certainly was a lot less painful than what I was doing for Karma. I've tried to set up HTML fixtures for testing and configured Puppeteer so I can run tests on Thebe's output.

stevejpurves commented 3 years ago

@Miniland1333 cool! and shouldn't take long at all. I suggest bringing your PR in, and then i'll port the few test I have to your jest setup and then we can build on that. sound good?

Miniland1333 commented 3 years ago

That does sound like a plan. @stevejpurves would you be able to review the PR #274 since you’ve used Jest before in case I made any obvious mistakes with the config or the tests?

stevejpurves commented 3 years ago

@Miniland1333 I just checked out and ran the tests on your branch. It all looks good for a starting point config additions will likely come later a we build in more tests.

Let's get your PR in 👍 and I'll port

Miniland1333 commented 3 years ago

@Miniland1333 I just checked out and ran the tests on your branch. It all looks good for a starting point config additions will likely come later a we build in more tests.

Let's get your PR in 👍 and I'll port

@stevejpurves #274 has been merged! Feel free to start porting your tests to Jest.

stevejpurves commented 3 years ago

@Miniland1333 yes, school holidays kicked in there hence the delay. I'm having a look this week. I see the puppeteer tests in better detail now. Seems like that those will give us more "wrap around" integration style tests as it basically simulates page load which is great but also there is space for more targeted unit level tests on the js code itself. I'm going to explore that aspect and bring in the jupyter server too, which seems like i'll need more jest config with webpack, but anyways i'll look at that aspect.

Miniland1333 commented 3 years ago

Also, Jest is interesting in that it can be run selectively based on the names of the files. As an example, if we decide to name files xxx.unit.js or xxx.integration.js, we can then run jest unit.js or jest integration.js to run the subset for unit or integration tests. This is because the argument to jest uses automatic pattern-matching of the filenames.

If you all think this is a good idea, I think this practice would help organization as the testbase expands.

stevejpurves commented 3 years ago

@Miniland1333 I've now got jest running on my initial tests. I've read back up the thread a little and opted to (a) split out a separate config fo the unit tests (b) leaned on the @jupyterlab/testutils repo for that config.

The comments from @blink1073 prompted that as it seems their work led them down separate tooling for e2e and unit tests, and the jest config as a bit more awkward with puppeteer in the mix (for me anyways). Anyway, both are running and https://github.com/executablebooks/thebe/pull/282 and it can be changed in future if needed.

cc @minrk