thebe icon indicating copy to clipboard operation
thebe copied to clipboard

improve test coverage

Open minrk opened this issue 4 years ago • 14 comments

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:

  • loading config from the page
  • transforming divs to cells
  • connecting to a kernel (local or with Binder? Binder will probably be a lot slower)
  • rendering output, especially latex
  • widgets
  • syntax highlighting for multiple languages
  • permutations of preloaded output, etc.

minrk avatar Jul 17 '20 09:07 minrk

We have a testutils package that has among other things:

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

  • coreutils shows a fairly minimal example of the config and testing setup
  • notebook is a kitchen sink: it uses all of the above mentioned utilities

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 😉

blink1073 avatar Jul 17 '20 09:07 blink1073

@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.

minrk avatar Aug 14 '20 07:08 minrk

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

blink1073 avatar Aug 14 '20 16:08 blink1073

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 avatar Oct 17 '20 11:10 stevejpurves

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:

  • faster tests
  • no single entrypoint, no webpack on test code and we can have jest look after test execution order
  • jest.mock is better then chai-spies and we may be able to use that to mock the thebelab module effectively where chai-spies are currently failing (I think due to the current export pattern)
  • using jest means we could drop in the wrapped jupyter server like the folks over at jupyterlab/debugger are.

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

stevejpurves avatar Oct 19 '20 14:10 stevejpurves

@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!

minrk avatar Oct 20 '20 07:10 minrk

@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.

Miniland1333 avatar Oct 20 '20 19:10 Miniland1333

@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?

stevejpurves avatar Oct 21 '20 08:10 stevejpurves

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?

Miniland1333 avatar Oct 21 '20 16:10 Miniland1333

@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 avatar Oct 23 '20 06:10 stevejpurves

@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.

Miniland1333 avatar Oct 28 '20 16:10 Miniland1333

@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.

stevejpurves avatar Nov 02 '20 12:11 stevejpurves

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.

Miniland1333 avatar Nov 02 '20 22:11 Miniland1333

@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

stevejpurves avatar Nov 09 '20 12:11 stevejpurves