Selenium utils + markdown rendering tests #3458

mpacer · 2018-03-22T19:17:28Z

This PR introduces a Notebook class as a helper for running selenium functions. It still needs work to document it more completely, but… it works and makes it a lot easier to create the relevant tests inside notebooks.

I'm interested to hear people's reviews.

I think @takluyver @Carreau @minrk and @rgbkrk would be particularly interested in this.

rgbkrk

Awesome

rgbkrk · 2018-03-22T22:37:26Z

I'm happy these at least make it easy for Python people to write tests for the JS. 😅

…ively

…ew page

mpacer · 2018-03-23T00:47:52Z

This is now rebased on #3412 to take into account the issues that arise when you create a new notebook in the first place.

__getitem__ also accepts slices also introduces __len__

takluyver · 2018-03-23T08:14:24Z

notebook/tests/selenium/quick_selenium.py

+    except IndexError as e:
+        e.message = 'You need a server running before you can run this command'
+    driver = Firefox()
+    auth_url = f'{server["url"]}?token={server["token"]}'


f strings are great, but we're still supporting Python 3.4 and 3.5 for now, so we'll need to stick with the explicit .format() method for now.

takluyver

Looks good, just a few points about clarity.

takluyver · 2018-03-23T08:17:03Z

notebook/tests/selenium/utils.py

+        """Gets all cells once they are visible.
+
+        """
+        wait_for_selector(self.browser, ".cell")


I don't like having a wait inside a property - the rule of thumb for properties is that anything they need to do should be near-instant. If it needs to do something that's not, let's make it a regular method - the call gives people reading the code a hint that there's something going on underneath.

I was wrong in my belief that it was needed here. One of the reasons I really didn't like it was that it couldn't handle notebooks with no cells; so this always seemed wrong. So it's gone!

takluyver · 2018-03-23T08:20:53Z

notebook/tests/selenium/utils.py

+
+@contextmanager
+def new_window(browser, selector=None):
+    """Creates new window, switches you to that window, waits for selector if set.


Neat! The docstring needs some work, though - this doesn't create a new window, rather it expects that code called in its context will create a new window, and then it switches to that on leaving the context.

This is worth explaining clearly, because most context managers do the interesting stuff when you create/enter them, and exiting just cleans something up. Here the interesting stuff is on exit.

I think I took care of this one.

takluyver · 2018-03-23T08:22:55Z

notebook/tests/selenium/utils.py

+        # initial_window_handles = browser.window_handles
+        with new_window(browser, selector=".cell"):
+            select_kernel(browser, kernel_name=kernel_name)
+        browser.execute_script("Jupyter.notebook.set_autosave_interval(0)")


Is it OK that this is in new_notebook() but remove_safety_check() is called from __init__? I'd expect they'd go together.

yea… I think you're right on that account.

takluyver · 2018-03-23T08:23:43Z

notebook/tests/selenium/utils.py

+
+
+    @classmethod
+    def new_notebook(cls, browser, kernel_name='kernel-python3'):


Docstring! It should mention that this expects to be called with the browser on the dashboard page. You could use your other utilities to verify that and throw an error if it's not.

takluyver · 2018-03-23T08:25:33Z

notebook/tests/selenium/utils.py

+    def current_cell_index(self):
+        return self.cells.index(self.current_cell)
+
+    def remove_safety_check(self):


'safety' is rather vague. remove_unsaved_check, maybe? Or base it on the hook name: remove_onbeforeunload.

takluyver · 2018-03-23T08:28:26Z

notebook/tests/selenium/utils.py

+        if visible:
+            conditional = EC.visibility_of_all_elements_located
+        else:
+            conditional = EC.presence_of_all_elements_located


This is steadily heading towards replicating the complexity of the underlying Selenium API. It's fine for now, but we should keep in mind that it may be better to use the underlying APIs directly in some cases.

Agreed — however, I think this covers all the cases where we wanted it and it hides a tonne of the selenium boilerplate so I think it's still ok.

mpacer · 2018-03-23T19:13:37Z

I think this now stands on its own. It's not fully handling everything that the markdown tests are doing, but it handles all the basic tests of our markdown rendering.

The mathjax rendering is a different story — however, I think we might be able to test that even more directly in pure javascript code as it's not really about what's rendered on the page but is entirely about what is parsed on the javascript side.

takluyver · 2018-03-25T07:50:33Z

notebook/tests/selenium/utils.py

+    """Contextmanager for switching to & waiting for a window created. 
+
+    This context manager gives you the ability to create a new window inside 
+    the created context and it will switch you to that new window.abs


takluyver · 2018-03-25T07:51:39Z

notebook/tests/selenium/utils.py

+    def index(self, cell):
+        return self.cells.index(cell)
+
+    def remove_safety_check(self):


Let's find a more representative name for this.

takluyver · 2018-03-25T07:54:17Z

notebook/tests/selenium/test_markdown.py

+                         '<pre><code class="cm-s-ipython language-aaaa">x = 1\n</code></pre>'
+                         ]
+    for i, cell in enumerate(nb):
+        nb.append(*cell_text, cell_type="markdown")


I don't think this needs to be in a loop now - the .append() call adds all the cells in one go.

takluyver · 2018-03-25T07:57:46Z

notebook/tests/selenium/quick_selenium.py

@@ -0,0 +1,43 @@
+from selenium.webdriver import Firefox


Let's have a module level docstring to explain that these utilities are not used by the tests, but they're intended for interactive exploration while writing tests.

I had intended to add this.

takluyver · 2018-03-27T19:11:29Z

The last few things on this are all docstring and naming points. If you've got other things to do, let me know and I can tidy them up.

takluyver · 2018-03-28T10:15:46Z

@mpacer sorry to take over the PR, but I wanted to get it merged in quickly.

rgbkrk · 2018-03-28T15:09:50Z

Thanks @mpacer and @takluyver!

hroncok · 2018-05-11T15:46:58Z

notebook/tests/selenium/utils.py

+        self.add_cell(index, cell_type="markdown")
+        self.edit_cell(index=index, content=content, render=render)
+
+    def append(self, *values, cell_type="code"):


This is invalid syntax on Python 2. Are those files intended to be supported on all the Pythons as notebook itself?

Notebook development is Python3 only now. Your runtime / kernel will always be allowed to be in Python2.

I'm planning to get python2-notebook out of Fedora, however I was wondering what's the upstream status on this. The docs say Python 2.7 is supported. And except the selenium tests it seems to be.

mpacer force-pushed the selenium_utils branch from 51c2d27 to 1889e56 Compare March 22, 2018 22:24

rgbkrk approved these changes Mar 22, 2018

View reviewed changes

mpacer force-pushed the selenium_utils branch from 1889e56 to 715d9e3 Compare March 22, 2018 22:42

mpacer added 18 commits March 22, 2018 17:34

add utils module and wait_for_selector

9d4cf94

add quick_selenium script for quickly starting up a selenium interact…

7c659f5

…ively

Introduce Notebook class and give usage example in quick_selenium

0994db2

add basic cell functionality to Notebook class

f9dd7c1

add body property

c9a8504

add edit_cell and execute_cell methods; remove focus_cell edit parameter

3d3086e

Add CellTypeError and convert_cell_type and wait_for_stale_cell methods

4b6a0a7

Add add_cell and add_markdown_cell methods

2b7f549

add method that allows you to close the notebook without confirming

e2243d6

add first markdown test and a notebook fixture for new notebook

86ae162

move sauce driver logic into isolated function, simplify selenium_driver

bf39dec

make authenticated browser module scope fixture for permission reasons

e0ed2c4

use wait_for_selector from utils module in test_dashboard_nav

c220215

add wait_for_selector to the cells property and to_command_mode

3092800

enhance wait_for_selector to handle returning single elements

3615d4a

add new_window contextmanager for creating, switching, & waiting on n…

d634f1d

…ew page

add select_kernel function for clicking "new" & selecting a kernel

a112ab6

create new_notebook classmethod creating/switching to new Notebook page

ebef7ba

mpacer force-pushed the selenium_utils branch from 715d9e3 to ebef7ba Compare March 23, 2018 00:46

mpacer added 2 commits March 22, 2018 19:34

small changes to naming things

5d19785

introduce cell __iter__, & __setitem__/__getitem__ for cells & indices

3571f16

__getitem__ also accepts slices also introduces __len__

takluyver reviewed Mar 23, 2018

View reviewed changes

mpacer added 2 commits March 23, 2018 08:33

enrich signature for execute_cell to accept both index and cell directly

e183fc1

use new rich container __*__ methods to make adding & executing nicer

c16dac2

mpacer added 7 commits March 23, 2018 11:01

fix add_cell logic, add content param, edit_cell default not render

02e0ac3

use index method to get cell index in execute_cell

98c09f8

add append, extend & coerce_to_cell methods

5e43458

add run_all method

d9dd5d8

add get_rendered_contents helper function

33ca649

use new utilities, enable more markdown cells to be added easily

0999798

make append actually append to the end of cell list

7808a89

mpacer added 5 commits March 23, 2018 12:18

add other markdown string conversion examples for test

c081af5

switch fstring to format string

d598ef5

Take into account lab as a potential endpoint for the driver

3c4596b

add quick_notebook utility and give docstring reminder to quit browsers

79603b4

Improve docstrings & comments; Remove unused code; Relocate script

74af79c

mpacer force-pushed the selenium_utils branch from 64a2619 to 74af79c Compare March 23, 2018 19:18

mpacer changed the title ~~Selenium utils + 1 markdown test~~ Selenium utils + markdown tests Mar 23, 2018

mpacer changed the title ~~Selenium utils + markdown tests~~ Selenium utils + markdown rendering tests Mar 23, 2018

takluyver reviewed Mar 25, 2018

View reviewed changes

takluyver mentioned this pull request Mar 25, 2018

Convert JS tests to Selenium #3335

Closed

nicer error and use append directly do not iterate over cells

515f8e2

takluyver added 3 commits March 28, 2018 11:07

Docstrings and naming clarity

c23ba2a

No need for copy

a6f604a

Remove unused variant of append for now

7266fd5

takluyver merged commit faa0cab into jupyter:master Mar 28, 2018

takluyver added this to the 5.5 milestone Mar 28, 2018

hroncok reviewed May 11, 2018

View reviewed changes

github-actions bot added the status:resolved-locked label Apr 1, 2021

github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selenium utils + markdown rendering tests #3458

Selenium utils + markdown rendering tests #3458

mpacer commented Mar 22, 2018

rgbkrk left a comment

rgbkrk commented Mar 22, 2018

mpacer commented Mar 23, 2018

takluyver Mar 23, 2018

mpacer Mar 23, 2018

takluyver left a comment

takluyver Mar 23, 2018

mpacer Mar 23, 2018

takluyver Mar 23, 2018

mpacer Mar 23, 2018

takluyver Mar 23, 2018

mpacer Mar 23, 2018

takluyver Mar 23, 2018

takluyver Mar 23, 2018

takluyver Mar 23, 2018

mpacer Mar 23, 2018

mpacer commented Mar 23, 2018

takluyver Mar 25, 2018

takluyver Mar 25, 2018

takluyver Mar 25, 2018

takluyver Mar 25, 2018

mpacer Mar 30, 2018

takluyver commented Mar 27, 2018

takluyver commented Mar 28, 2018

rgbkrk commented Mar 28, 2018

hroncok May 11, 2018

rgbkrk May 11, 2018

hroncok May 11, 2018



		@classmethod
		def new_notebook(cls, browser, kernel_name='kernel-python3'):

Selenium utils + markdown rendering tests #3458

Selenium utils + markdown rendering tests #3458

Conversation

mpacer commented Mar 22, 2018

rgbkrk left a comment

Choose a reason for hiding this comment

rgbkrk commented Mar 22, 2018

mpacer commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpacer commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver commented Mar 27, 2018

takluyver commented Mar 28, 2018

rgbkrk commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment