[FIX, TEST] Fix create_comm in ThebeManager (reopening PR#311)

[celine168]

Reverts the commit, "Revert "Fix create_comm in ThebeManager"". This reverts commit 4f09ba2 (PR #329).

This is a reopening of PR #311 to bring up a local Jupyter server that was suggested, rather than using an external Binder session.

[moorepants]

I'd like to get this in the 0.6.0 release.

[moorepants]

Is there an example of firing up the Jupyterserver yet?

[moorepants]

I guess there isn't. I'm not seeing it anywhere.

[moorepants]

There is this in the top level of the repo: https://github.com/executablebooks/thebe/blob/master/jupyter-server.js

[celine168]

I'd like to get this in the 0.6.0 release.

We can release a minor version after we fix the test?

Is there an example of firing up the Jupyterserver yet?

It was added in #257, I'll play around later to see if I could figure out how to get it to work. I'm not so well-versed in Javascript though so any advice is appreciated.

[moorepants]

We can release a minor version after we fix the test?

Yes.

[celine168]

If we used a local Jupyter server, this particular test requires the server to have the bqplot package since it loads Thebe. I'm not sure if JupyterServer allows for custom images/packages to be installed with the server.

Another way to test that @narrator0 and I were considering was to add a unit test instead, but we don't think that suits this PR since _create_comm wraps around the createComm function from JupyterLab and the kernel doesn't exist (unless we mock it).

Another idea: creating a local Binder service (in a machine or Docker container) for testing, which would solve the package problem, while avoiding mybinder.org. But this may also lead to timeout issues.

We still think that an integration test is the best way to test this kind of logic out though in practice.

[celine168]

@narrator0 extended the timeouts for Puppeteer (which were causing the errors in the first place) and committed it to this PR.

[narrator0]

@moorepants I think we didn't have this fix in 0.6.0. I tested the release and bqplot is still not working.

[celine168]

It's not in release 0.6.0 since no changes relating to this issue got merged. There's not a clear way on how we should approach testing without timeouts (although we did extend the Puppeteer timeout issue in this PR, which allowed the tests to pass this time).

Maybe we could merge the change in without the test (the change in function name is documented here), and open another PR with the test?

[moorepants]

Maybe we could merge the change in without the test (the change in function name is documented here), and open another PR with the test?

Yes, this is fine with me.

[celine168]

I'm sorry for the delay. I've removed the previous e2e test and HTML page for the bqplot to work. The commit history probably looks a bit messy right now, please let me know if you would like for me to clean it up!

[moorepants]

@sandertyu, @mandeepika, @TimStewartJ, @pmackle can you all verify that this works manually. Testing that each example still works in the docs for thebe would give us some sense of whether this breaks anything. I still haven't seen any confirmation on that. Thanks. Once confirmed here that it works and doesn't seem to break anything, I'll merge.

[sandertyu]

bqplot completely fails to display using the same repository and code as on the example page;

Ipyleaflet is similarly broken;

All other widgets besides plotly (see below) fail with similar errors using the exact repositories from the Thebe example pages. The common theme is that Cannot read property error in htmlmanager.js:72. I believe that it is jupyter code, and it's imported here

Plotly fails both in Thebe development version with a slightly different error. I also saw it fail on the example page itself, but it's working now (???) and I didn't screenshot it. The error was same as below though;

Furthermore, I am testing this with Thebe 0.6.0 rather than 0.5.1 which I believe this PR was originally built on (??).

[TimStewartJ]

In my testing, I got results equivalent to the docs as long as I put the required scripts in the html I used for testing. When I didn't include these scripts, I got errors similar to what Noah's, leading me to believe that he simply forgot to include these scripts in testing.
On an unrelated note, the example in the docs for ipycytoscape seems to not be working. Not sure if this is a fluke or not, but I can replicate the error in a local instance as well. I think the example code may need to be changed or updated for that.

[sandertyu]

Tim is right, I forgot to load the proper javascript libraries in development.html. I also see that ipycytoscape is not properly displaying on the documentation page, and the same thing happens if I run that repository in a local Thebe instance or barebones-ish HTML page. However, I can confirm that QuantStack/ipycytoscape 1.2.0 does work in local Thebe, so we should try updating the repo for that documentation page most likely.

I've tested bqplot now, and the buttons do not seem to work as this fix was intended to do.

[TimStewartJ]

That's odd, the buttons are operational on this PR in my testing:

[sandertyu]

That's odd, the buttons are operational on this PR in my testing:

Yeah you're right. I cloned Celine's repo but it seems I never moved off of the master branch. I see that the buttons function properly.

[moorepants]

Ok, both have tested this and we find it working. So merging. Thanks!