Convert display_id JS test to selenium (#3335) #5394
Conversation
notebook/tests/selenium/utils.py
Outdated
| @@ -256,14 +260,15 @@ def edit_cell(self, cell=None, index=0, content="", render=False): | |||
| for line_no, line in enumerate(content.splitlines()): | |||
| if line_no != 0: | |||
| self.current_cell.send_keys(Keys.ENTER, "\n") | |||
| self.current_cell.send_keys(Keys.ENTER, Keys.HOME) # Avoid auto-indent. | |||
There was a problem hiding this comment.
Before I discovered the prefill_notebook fixture, I was having trouble with auto-indentation when setting cell content to a multiline string that ended in a colon. This resets the cursor to the start of the line before sending the line content.
It is now redundant because the display_id test uses prefill_notebook, which doesn't have that problem, but I figured it's a useful fix regardless.
There was a problem hiding this comment.
Hm, looks like this doesn't quite work as I intended, so I'll remove it for now.
|
Ugh, apologies for the whitespace spam in the commits for |
| notebook.browser.execute_script(textwrap.dedent("""\ | ||
| Jupyter.notebook.insert_cell_at_index("code", 3); | ||
| var cell = Jupyter.notebook.get_cell(3); | ||
| cell.set_text([ | ||
| "display_with_id(5, 'here')", | ||
| "display_with_id(6, 'here', update=True)", | ||
| ].join('\\n')); | ||
| cell.execute(); | ||
|
|
||
| var cell = IPython.notebook.get_cell(3); | ||
| var kernel = IPython.notebook.kernel; | ||
| var msg_id = cell.last_msg_id; | ||
| var callback_id = 'mycallbackid' | ||
| cell.iopub_messages = []; | ||
| var add_msg = function(msg) { | ||
| msg.content.output_type = msg.msg_type; | ||
| cell.iopub_messages.push(msg.content); | ||
| }; | ||
| kernel.set_callbacks_for_msg(callback_id, { | ||
| iopub: { | ||
| output: add_msg, | ||
| clear_output: add_msg, | ||
| } | ||
| }, false); | ||
| kernel.output_callback_overrides_push(msg_id, callback_id); | ||
| """)) |
There was a problem hiding this comment.
I ended up keeping most of this test in JavaScript. Why?
- Starting with the second half (L. 92-108), seems to me like these callbacks have to be set from JS, but this is the first time I've come across iopub, so maybe I'm missing something?
- As for the first bit (L. 84-90) - adding a cell and setting its contents - this could be done in selenium, via the UI. However, when I tried that, I started getting intermittent test failures. I played around with this in the browser and it turns out that adding just a slight delay (tens of milliseconds) on L. 91 causes the cell's
iopub_messagesarray to remain empty.
It should be obvious from my comments that I don't know what I'm doing or what's going here! :)
If there's a better way to approach this or some documentation I could be reading to understand it better, I'd be all ears.
| notebook = prefill_notebook(INITIAL_CELLS) | ||
| notebook.execute_cell(0) | ||
|
|
||
| #TODO describe test 1 |
There was a problem hiding this comment.
Since this is all one big, long test, it'd be nice to have a short description of what each part is testing. I'm not really sure what it is that is being tested here. Could someone who knows more maybe suggest a short summary?
| assert cell1_outputs[1]['transient']['display_id'] == 'here', 'has transient display_id' | ||
| assert cell1_outputs[1]['data']['text/plain'] == '1', 'display_with_id produces output' | ||
|
|
||
| #TODO describe test 2 |
| assert cell3_iopub_messages[1]['data']['text/plain'] == '6', 'value' | ||
| assert cell3_iopub_messages[1]['transient']['display_id'] == 'here', 'display id 1' | ||
|
|
||
| #TODO describe test 4 |
| def _get_outputs(cell_idx): | ||
| return notebook.browser.execute_script(f"""\ | ||
| var cell = Jupyter.notebook.get_cell({cell_idx}); | ||
| return cell.output_area.outputs; |
There was a problem hiding this comment.
The JS version converts the outputs first to JSON and then loads them back, I'm not sure why. I skipped this in the Python version (also because selenium probably does something similar anyway).
| display('below') | ||
| """)) | ||
| notebook.wait_for_cell_output(1) | ||
|
|
There was a problem hiding this comment.
Here and at various other points in the test, the JS version calls wait_for_idle, which checks if IPython._status == "idle". I tried replicating this in the Python version, but it fails because there's no _status on the IPython object? I'm not sure what's going on here, but the tests don't seem affected by this.
| from IPython.display import display | ||
| def display_with_id(obj, display_id, update=False, execute_result=False): | ||
| iopub = ip.kernel.iopub_socket | ||
| session = get_ipython().kernel.session |
There was a problem hiding this comment.
I'm guessing it's just an oversight by the original author that this line uses get_ipython() instead of the ip variable (like the other lines)?
| session = get_ipython().kernel.session | |
| session = ip.kernel.session |
Hi @takluyver! This is my first time on this project, so please do let me know if anything about this PR isn't in line with how things are done here - and apologies in advance if so!
I was looking for a way to help out and found issue #3335, where I picked a test that seems not to have been converted yet.
The selenium test is passing and all assertions from the JS original are there, but I still have some questions, which I'll post as comments.