Fix create_comm in ThebeManager #311

celine168 · 2020-11-17T17:15:54Z

Intends to fix #302

In this _create_comm, this.kernel returns undefined as the variable is never set. I set the variable in registerWithKernel. I also could not find the function connectToComm in the KernelConnection class.

  _create_comm(target_name, model_id, data, metadata) {
    const comm = this.kernel.connectToComm(target_name, model_id);
    if (data || metdata) {
      comm.open(data, metadata);
    }
    return Promise.resolve(new ShimmedComm(comm));
  }

HTML page used to test:

<!DOCTYPE html>
<html>
    <head>
        <!-- Configure and load Thebe !-->
        <!-- two config sections: first binder, second local
        switch which one has type="text/x-thebe-config"
        to activate it
      -->
      <!-- script that allows the line plots to be displayed, otherwise it gives an error -->
      <script src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.4/require.min.js"></script>

      <!-- script for awesome fonts 4.0 needed for navigation buttons -->
      <script src="https://use.fontawesome.com/3e02a3c909.js"></script>

        <script type="text/x-thebe-config">
          {
            requestKernel: true,
            binderOptions: {
              repo: "bqplot/bqplot",
              ref: "0.12.18",
              codeMirrorConfig: {
                "theme": "abcdef"
              },
            },
          }
        </script>
        <!-- use this one (and set the token!) for local testing (much faster)
        e.g.
        jupyter notebook \
          --NotebookApp.token=thebe-test-secret \
          --NotebookApp.allow_origin='http://127.0.0.1:8080'
        -->
        <script type="text/x-thebe-config-disabled">
          {
            requestKernel: true,
            kernelOptions: {
              name: "python3",
              path: ".",
              // notebook server configuration; not needed with binder
              serverSettings: {
                "baseUrl": "http://127.0.0.1:8888",
                "token": "thebe-test-secret",
              },
            },
            codeMirrorConfig: {
              "theme": "abcdef"
            },
          }
        </script>
        <script src="./lib/index.js"></script>
    </head>
    <body>
        <div class="container">
            <button id="activateButton">Activate</button>
            <script>
                var bootstrapThebe = function() {
                    thebelab.bootstrap();
                }

                document.querySelector("#activateButton").addEventListener('click', bootstrapThebe)
            </script>

            <pre data-executable="true" data-language="python" class="pre-element">print("Hello!")</pre>
            <pre data-executable="true" data-language="python" class="pre-element">
from IPython.display import Math
Math("$x = 5$")
            </pre>
            <pre data-executable="true" data-language="python" class="pre-element">
import numpy as np
from bqplot import pyplot as plt

size = 100
np.random.seed(0)
x_data = np.arange(size)
y_data = np.cumsum(np.random.randn(size)  * 100.0)

plt.figure(title='My First Plot')
plt.plot(x_data, y_data)
plt.show()
            </pre>
        </div>
    </body>
    <style>
        body {
            margin: 0;
        }
        .container {
            display: flex;
            flex-direction: column;
            justify-content: center;
            align-items: center;
            height: 100vh;
        }
        #activateButton {
            width: 130px;
            height: 50px;
            font-size: 1.25em;
            border: 1px solid black;
            border-radius: 5px;
            background-color: lightblue;
            text-transform: uppercase;
        }
        .pre-element {
            width: 50vw;
            height: 20vh;
            border: 2px solid #aaa;
            border-radius: 4px;
            margin-top: 8px;
            margin-bottom: 8px;
        }
        .thebelab-cell {
            width: 50vw;
        }
    </style>
</html>

celine168 · 2020-11-26T02:01:38Z

@narrator0 and I just added a Jest test testing bqplot's pan/zoom button.

moorepants · 2020-11-26T08:04:50Z

Do other tests load a binder like this one? I worry that too much time is required to run the tests with that. But if others are doing that, I guess it is fine.

celine168 · 2020-11-26T22:07:57Z

The read-only test that was added does load binder but I believe it only checks whether a specific tag exists.

This test requires actually running the code block and testing whether the pan/zoom button will work without any error.

Miniland1333 · 2020-12-01T19:22:37Z

Maybe wait on #282 to be merged in first

stevejpurves · 2020-12-04T07:34:16Z

@moorepants to your comment, plan is to soon have the test runner fire up a local jupyter server around the tests automatically making that connection fast. My plan was to do that once #282 is merged (or i'll extend that PR if it stays unmerged) as #282 has changes to the test layout it would be good to have that in first as @Miniland1333 suggests.

moorepants · 2020-12-09T11:13:07Z

@celine168 Can you update this to work with #282?

celine168 · 2020-12-13T20:50:45Z

Just moved the files to the e2e directory! Hoping that works.

celine168 · 2020-12-22T18:46:07Z

@stevejpurves @Miniland1333 Just checking, is it proper to move this test to the e2e directory like this?

moorepants · 2020-12-22T19:18:28Z

We just reviewed this in our meeting and feel it is good to go. Merging.

moorepants · 2020-12-22T20:12:17Z

The tests were fine here but seem to be failing on master...

moorepants · 2020-12-22T20:13:47Z

I'm seeing:

PASS e2e/readonly.test.js
FAIL e2e/bqplot.test.js (32.212 s)
  ● Test bqplot › cells are default editable › case-bqplot-pan

    expect(received).resolves.toBeTruthy()

    Received promise rejected instead of resolved
    Rejected to value: [TimeoutError: waiting for selector `button.widget-toggle-button` failed: timeout 30000ms exceeded]

      45 |   describe("cells are default editable", () => {
      46 |     it("case-bqplot-pan", async () => {
    > 47 |       await expect(editBQPlot()).resolves.toBeTruthy();
         |             ^
      48 |     }, 100000000);
      49 |   });
      50 | });

      at expect (node_modules/expect/build/index.js:134:15)
      at Object.<anonymous>.global.expect (node_modules/expect-puppeteer/lib/index.js:118:12)
      at Object.<anonymous> (e2e/bqplot.test.js:47:13)

Test Suites: 1 failed, 1 passed, 2 total
Tests:       1 failed, 16 passed, 17 total
Snapshots:   0 total
Time:        36.79 s
Ran all test suites matching /test.js/i.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] test:e2e: `jest test.js --config=./jest.e2e.config.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] test:e2e script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

moorepants · 2020-12-22T20:14:34Z

Would this happen because binder takes too long to load?

moorepants · 2020-12-22T20:16:38Z

I opened #329 to revert, which I may do.

moorepants · 2020-12-22T20:20:04Z

I reverted the changes. I think we need to think more carefully about trying to run a binder in a js test. It isn't going to be reliable.

As I mentioned before, we need to test if the kernel attribute is present as expected, not check if bqplot runs properly. We can do that manually with the example in the docs.

stevejpurves · 2020-12-22T21:09:28Z

Can we reopen the original PR / branch?. I think we should bring up the local test Jupyter server that was discussed, in order to reliably to connect to in this type of test. You can assign that one to me.

celine168 · 2020-12-23T01:02:25Z

I apologize for suggesting merging this PR so early, especially considering the binder session in the jest test!

I have reopened the PR at #330!

celine168 and others added 5 commits December 13, 2020 12:15

Fix create_comm in ThebeManager

65f147c

Add test

81c8aec

Fix test

85d2751

Clean up files and names

5aac370

Update directories of tests to match PR#282

0ad7a9e

celine168 force-pushed the fix/create_comm branch from bc3beb7 to 0ad7a9e Compare December 13, 2020 20:48

moorepants merged commit 07ad367 into jupyter-book:master Dec 22, 2020

moorepants mentioned this pull request Dec 22, 2020

Revert "Fix create_comm in ThebeManager" #329

Merged

celine168 mentioned this pull request Dec 23, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix create_comm in ThebeManager #311

Fix create_comm in ThebeManager #311

celine168 commented Nov 17, 2020 •

edited

Loading

celine168 commented Nov 26, 2020

moorepants commented Nov 26, 2020

celine168 commented Nov 26, 2020

Miniland1333 commented Dec 1, 2020

stevejpurves commented Dec 4, 2020

moorepants commented Dec 9, 2020

celine168 commented Dec 13, 2020

celine168 commented Dec 22, 2020 •

edited

Loading

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

stevejpurves commented Dec 22, 2020

celine168 commented Dec 23, 2020

Fix create_comm in ThebeManager #311

Fix create_comm in ThebeManager #311

Conversation

celine168 commented Nov 17, 2020 • edited Loading

celine168 commented Nov 26, 2020

moorepants commented Nov 26, 2020

celine168 commented Nov 26, 2020

Miniland1333 commented Dec 1, 2020

stevejpurves commented Dec 4, 2020

moorepants commented Dec 9, 2020

celine168 commented Dec 13, 2020

celine168 commented Dec 22, 2020 • edited Loading

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

moorepants commented Dec 22, 2020

stevejpurves commented Dec 22, 2020

celine168 commented Dec 23, 2020

celine168 commented Nov 17, 2020 •

edited

Loading

celine168 commented Dec 22, 2020 •

edited

Loading