From df8a5480120866cd4ffe4749e9a8f18efce37637 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Thu, 31 Jan 2019 11:47:45 +0000 Subject: [PATCH 1/3] Some improvements to promise handling when saving notebook --- notebook/static/notebook/js/notebook.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/notebook/static/notebook/js/notebook.js b/notebook/static/notebook/js/notebook.js index 8c125f3468..fad772a194 100644 --- a/notebook/static/notebook/js/notebook.js +++ b/notebook/static/notebook/js/notebook.js @@ -2742,6 +2742,8 @@ define([ $.proxy(that.save_notebook_success, that, start), function (error) { that.events.trigger('notebook_save_failed.Notebook', error); + // This hasn't handled the error, so propagate it up + return Promise.reject(error); } ); }; @@ -2845,6 +2847,7 @@ define([ this.create_checkpoint(); this._checkpoint_after_save = false; } + return data; }; Notebook.prototype.save_notebook_as = function() { @@ -3309,7 +3312,7 @@ define([ */ Notebook.prototype.save_checkpoint = function () { this._checkpoint_after_save = true; - this.save_notebook(true); + return this.save_notebook(true); }; /** From e00a86c4cb22f15ed4593ccf02bb92fe5e284b84 Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Thu, 31 Jan 2019 11:48:10 +0000 Subject: [PATCH 2/3] Throw clearer error if no new window handles found --- notebook/tests/selenium/utils.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/notebook/tests/selenium/utils.py b/notebook/tests/selenium/utils.py index 7b26ab04b7..0c03d4dc68 100644 --- a/notebook/tests/selenium/utils.py +++ b/notebook/tests/selenium/utils.py @@ -324,9 +324,11 @@ def new_window(browser, selector=None): """ initial_window_handles = browser.window_handles yield - new_window_handle = next(window for window in browser.window_handles - if window not in initial_window_handles) - browser.switch_to.window(new_window_handle) + new_window_handles = [window for window in browser.window_handles + if window not in initial_window_handles] + if not new_window_handles: + raise Exception("No new windows opened during context") + browser.switch_to.window(new_window_handles[0]) if selector is not None: wait_for_selector(browser, selector) From bc3a8cbe797dd1181c51cd54aab4315ddc30c1be Mon Sep 17 00:00:00 2001 From: Thomas Kluyver Date: Thu, 31 Jan 2019 11:49:06 +0000 Subject: [PATCH 3/3] Convert test for saving with complex name to Selenium --- notebook/tests/notebook/save.js | 112 --------------------------- notebook/tests/selenium/test_save.py | 65 ++++++++++++++++ 2 files changed, 65 insertions(+), 112 deletions(-) delete mode 100644 notebook/tests/notebook/save.js create mode 100644 notebook/tests/selenium/test_save.py diff --git a/notebook/tests/notebook/save.js b/notebook/tests/notebook/save.js deleted file mode 100644 index 0e5665250b..0000000000 --- a/notebook/tests/notebook/save.js +++ /dev/null @@ -1,112 +0,0 @@ -// -// Test saving a notebook with escaped characters -// - -casper.notebook_test(function () { - // don't use unicode with ambiguous composed/decomposed normalization - // because the filesystem may use a different normalization than literals. - // This causes no actual problems, but will break string comparison. - var nbname = "has#hash and space and unicø∂e.ipynb"; - - this.append_cell("s = '??'", 'code'); - - this.thenEvaluate(function (nbname) { - require(['base/js/events'], function (events) { - IPython.notebook.set_notebook_name(nbname); - IPython._save_success = IPython._save_failed = false; - events.on('notebook_saved.Notebook', function () { - IPython._save_success = true; - }); - events.on('notebook_save_failed.Notebook', - function (event, error) { - IPython._save_failed = "save failed with " + error; - }); - IPython.notebook.save_notebook(); - }); - }, {nbname:nbname}); - - this.waitFor(function () { - return this.evaluate(function(){ - return IPython._save_failed || IPython._save_success; - }); - }); - - this.then(function(){ - var success_failure = this.evaluate(function(){ - return [IPython._save_success, IPython._save_failed]; - }); - this.test.assertEquals(success_failure[1], false, "Save did not fail"); - this.test.assertEquals(success_failure[0], true, "Save OK"); - - var current_name = this.evaluate(function(){ - return IPython.notebook.notebook_name; - }); - this.test.assertEquals(current_name, nbname, "Save with complicated name"); - var current_path = this.evaluate(function(){ - return IPython.notebook.notebook_path; - }); - this.test.assertEquals(current_path, nbname, "path OK"); - }); - - this.thenEvaluate(function(){ - IPython._checkpoint_created = false; - require(['base/js/events'], function (events) { - events.on('checkpoint_created.Notebook', function (evt, data) { - IPython._checkpoint_created = true; - }); - }); - IPython.notebook.save_checkpoint(); - }); - - this.waitFor(function () { - return this.evaluate(function(){ - return IPython._checkpoint_created; - }); - }); - - this.then(function(){ - var checkpoints = this.evaluate(function(){ - return IPython.notebook.checkpoints; - }); - this.test.assertEquals(checkpoints.length, 1, "checkpoints OK"); - }); - - this.then(function(){ - this.open_dashboard(); - }); - - this.then(function(){ - var notebook_url = this.evaluate(function(nbname){ - var escaped_name = encodeURIComponent(nbname); - var return_this_thing = null; - $("a.item_link").map(function (i,a) { - if (a.href.indexOf(escaped_name) >= 0) { - return_this_thing = a.href; - return; - } - }); - return return_this_thing; - }, {nbname:nbname}); - this.test.assertNotEquals(notebook_url, null, "Escaped URL in notebook list"); - // open the notebook - this.open(notebook_url); - }); - - // wait for the notebook - this.waitFor(this.kernel_running); - - this.waitFor(function() { - return this.evaluate(function () { - return IPython && IPython.notebook && true; - }); - }); - - this.then(function(){ - // check that the notebook name is correct - var notebook_name = this.evaluate(function(){ - return IPython.notebook.notebook_name; - }); - this.test.assertEquals(notebook_name, nbname, "Notebook name is correct"); - }); - -}); diff --git a/notebook/tests/selenium/test_save.py b/notebook/tests/selenium/test_save.py new file mode 100644 index 0000000000..2ce609a5a7 --- /dev/null +++ b/notebook/tests/selenium/test_save.py @@ -0,0 +1,65 @@ +"""Test saving a notebook with escaped characters +""" + +from urllib.parse import quote +from .utils import wait_for_selector, new_window + +promise_js = """ +var done = arguments[arguments.length - 1]; +%s.then( + data => { done(["success", data]); }, + error => { done(["error", error]); } +); +""" + +def execute_promise(js, browser): + state, data = browser.execute_async_script(promise_js % js) + if state == 'success': + return data + raise Exception(data) + + +def test_save(notebook): + # don't use unicode with ambiguous composed/decomposed normalization + # because the filesystem may use a different normalization than literals. + # This causes no actual problems, but will break string comparison. + nbname = "has#hash and space and unicø∂e.ipynb" + escaped_name = quote(nbname) + + notebook.edit_cell(index=0, content="s = '??'") + + notebook.browser.execute_script("Jupyter.notebook.set_notebook_name(arguments[0])", nbname) + + model = execute_promise("Jupyter.notebook.save_notebook()", notebook.browser) + assert model['name'] == nbname + + current_name = notebook.browser.execute_script("return Jupyter.notebook.notebook_name") + assert current_name == nbname + + current_path = notebook.browser.execute_script("return Jupyter.notebook.notebook_path") + assert current_path == nbname + + displayed_name = notebook.browser.find_element_by_id('notebook_name').text + assert displayed_name + '.ipynb' == nbname + + execute_promise("Jupyter.notebook.save_checkpoint()", notebook.browser) + + checkpoints = notebook.browser.execute_script("return Jupyter.notebook.checkpoints") + assert len(checkpoints) == 1 + + notebook.browser.find_element_by_css_selector('#ipython_notebook a').click() + hrefs_nonmatch = [] + for link in wait_for_selector(notebook.browser, 'a.item_link'): + href = link.get_attribute('href') + if escaped_name in href: + print("Opening", href) + notebook.browser.get(href) + wait_for_selector(notebook.browser, '.cell') + break + hrefs_nonmatch.append(href) + else: + raise AssertionError("{!r} not found in {!r}" + .format(escaped_name, hrefs_nonmatch)) + + current_name = notebook.browser.execute_script("return Jupyter.notebook.notebook_name") + assert current_name == nbname