Add dual mode JS tests #5290

jdfreder · 2014-03-07T22:20:08Z

This PR adds 403 tests for the notebook's new dual mode.

how it works

After each action, the states of the notebook cells, code mirror instances, notebook, and keyboard manager are all verified as the expected results.

closes #4887

ellisonbg · 2014-03-07T22:40:08Z

Other things to test:

shift+enter, ctrl+enter, alt+enter when the cell i) is the last one in the notebook, ii) is not the last one in the notebook, iii) when the cell starts out in edit mode and iv) when the cell starts out in command mode.
To code cell: y
Markdown cell: to and from, render using *+enter, to and from edit mode in both rendered+unrendered state.
Raw cell: to (r)
up/k: test in edit mode as well, at the top/bottom line of the edit cell it should jump to the next/prev cell and put it in edit mode.
down/j: same
ctrl+k: moving cells up/down should focus them.
ctrl+j: same
a
b
x/c/v/shift+v/z: various combinations of cut/copy/paste/undo
d: delete cell
split/merge above&below: very careful about which cell is selected and what its mode is in (it depends on the mode of the original cell).

ellisonbg · 2014-03-07T22:41:24Z

IPython/html/tests/notebook/dualmode.js

+
+
+    // Utility functions.
+    this.validate_state = function(message, mode, cell_index) {


Let's move this into utils.js.

Let's actually move most of these utility function to utils.js.

For some tests, especially those involving markdown or heading cells, we should test the rendered state as well.

ellisonbg · 2014-03-10T22:12:03Z

Closes #4887

jdfreder · 2014-03-10T22:18:45Z

jdfreder · 2014-03-12T01:55:46Z

@ellisonbg I've implemented all of the things you mentioned here and that we talked about in person, it's ready for another review. I think the only other thing I need to do before merge is add some comments to the new util.js methods.

jdfreder · 2014-03-12T01:57:55Z

Ahh and I forgot to mention, I removed the arrow keys test since the dualmode_arrows.js tests the exact same thing in much greater detail.

minrk · 2014-03-12T02:58:54Z

Can you remove the unused IPython.keyboard.trigger_keydown in this PR? It looks like you redefined the method where it belongs, in the test utils.

jdfreder · 2014-03-12T17:58:04Z

Can you remove the unused IPython.keyboard.trigger_keydown in this PR? It looks like you redefined the method where it belongs, in the test utils.

Yup, went ahead and removed it... Also jshinted the file since I was touching it anyways.

ellisonbg · 2014-03-13T20:49:28Z

IPython/html/tests/util.js

+};
+
+// Emulate a keydown in the notebook.
+casper.trigger_keydown = function() {


Because casper.trigger_keydown has to rely on calls to IPython.keyboard... and basically has to reimplement the logic of IPython.keyboard.trigger_keydown I actually think it makes sense to put IPython.keyboard.trigger_keydown back into place. If the casper.trigger_keydown version didn't need to make calls back into al the stuff in keyboard I would feel differently, but in this case I think this refactoring makes it less clear. Are you OK with this @minrk ?

I don't think so. IPython.keyboard.trigger_keydown never made sense - it is a utility exclusively for testing, so its definition belongs in tests.

That is fine.

ellisonbg · 2014-03-13T20:50:52Z

Only minor changes needed.

jdfreder · 2014-03-14T18:37:08Z

Changes addressed

ellisonbg · 2014-03-20T19:19:37Z

IPython/html/tests/notebook/merge_cells_api.js

@@ -9,7 +10,10 @@ casper.notebook_test(function() {
            var cell_one = IPython.notebook.get_selected_cell();
            cell_one.set_text('a = 5');

-            IPython.keyboard.trigger_keydown('b');
+            var element = $(document);
+            var event = IPython.keyboard.shortcut_to_event('b', 'keydown');


Can't you use this.trigger_keydown here?

Not without rewriting the entire test since it's nested in an evaluate... But that's okay, I think it makes more sense to rewrite the test.

ellisonbg · 2014-03-20T19:22:17Z

This looks good, but I am seeing a test failure:

Test file: /Users/bgranger/Documents/Computing/IPython/code/ipython/IPython/html/tests/notebook/tooltip.js
PASS tooltip token: C
PASS tooltip token: MyClass
PASS tooltip token: foo123
FAIL 530 tests executed in 104.283s, 529 passed, 1 failed, 0 dubious, 0 skipped.

Details for the 1 failed test:

In /Users/bgranger/Documents/Computing/IPython/code/ipython/IPython/html/tests/notebook/dualmode_arrows.js:41
  Untitled suite in /Users/bgranger/Documents/Computing/IPython/code/ipython/IPython/html/tests/notebook/dualmode_arrows.js
    assert: up; cell 2 is the only cell selected

jdfreder · 2014-03-20T20:54:19Z

a test failure

That particular failure is very concerning because it means you've encountered a situation where two cells are selected at once. 😦

jdfreder · 2014-03-20T21:26:08Z

@ellisonbg here is the human reproducable steps performed by the failing test. Can you try these and see if you can break your notebook's state?

Start in a notebook with four code cells, all executed, the first one left blank:

cell 2

print("a")

cell 3

print("b")

cell 4

print("c")

Select the last cell by clicking on it.
Click escape to enter command mode.
Hit this sequence of keys: j, down, up

Select the first cell by clicking on it.
Click escape to enter command mode.
Hit this sequence of keys: k, up, down

Select the last cell by clicking on it.
Stay in edit mode.
Hit this sequence of keys: down, up

FAILURE HERE

The test continues to try some more things...

ellisonbg · 2014-03-20T22:33:10Z

Tests pass for me locally, once Travis passes, lets merge.

minrk · 2014-03-20T23:07:28Z

IPython/html/static/base/js/keyboard.js

@@ -262,8 +262,7 @@ IPython.keyboard = (function (IPython) {
        normalize_key : normalize_key,
        normalize_shortcut : normalize_shortcut,
        shortcut_to_event : shortcut_to_event,
-        event_to_shortcut : event_to_shortcut,
-        trigger_keydown : trigger_keydown


you removed the export here, but not the actual function itself.

I am fine with trigger_keydown being removed from here, but definitely think that event_to_shortcut should stay.

Ahh, nevermind, it is still here...

I didn't mention anything about event_to_shortcut; that's actually used in the code. Just pointing out that trigger_keydown has just been made unreachable here, but the actual definition is what should be removed.

I think this was the result of a bad rebase...

in preparation to move them into the base utils.js

added new function that sets the codemirror instance cursor coords

also added a bunch of missing semicolons (jshint)

Made the method comments more pythonic by moving them within the method definitions.

jdfreder · 2014-03-21T01:47:15Z

I re-removed the trigger keydown method... It was un-removed during a bad rebase. erg

minrk · 2014-03-24T22:37:11Z

IPython/html/static/base/js/keyboard.js

@@ -210,7 +201,7 @@ IPython.keyboard = (function (IPython) {
        if (!suppress_help_update) {
            // update the keyboard shortcuts notebook help
            $([IPython.events]).trigger('rebuild.QuickHelp');
-        }
+    }


these indentation changes are not correct

jdfreder · 2014-03-24T22:44:20Z

I just fixed the broken indents

takluyver · 2014-03-27T19:14:59Z

In the dev meeting, we seemed to think this is ready to merge. Is there anything else we want to do/review with it before I press the green button?

minrk · 2014-03-27T20:43:00Z

Don't think so.

Add dual mode JS tests

ellisonbg reviewed Mar 7, 2014
View reviewed changes

ellisonbg reviewed Mar 13, 2014
View reviewed changes

ellisonbg reviewed Mar 20, 2014
View reviewed changes

minrk reviewed Mar 20, 2014
View reviewed changes

jdfreder added 9 commits March 20, 2014 16:10

Add dual mode JS tests

7a8ad5a

s/key_press/trigger_keydown

af5c87b

s/_is_cell_editor_focused/is_cell_editor_focused

472f166

s/_is_cell_on/is_cell_on

e73c568

Make a few more methods public,

dd38bea

in preparation to move them into the base utils.js

Move code into is_cell_editor_focused

09eb378

Add '_only' to only cell methods

2c9375c

sep. fucntions to move to util.js

5acc968

Made *+enter tests more complicated.

6477a1a

jdfreder and others added 20 commits March 20, 2014 16:11

Added split merge tests

ceb999e

Moved util functions into util.js

bda578e

Fixed problem with split tests,

5cb85e2

added new function that sets the codemirror instance cursor coords

HUGE speed improvements to dual mode tests

e9e745e

Move validate notebook state into utils

f6c9e99

Cleaned up test names and locations.

c5e9bff

Removed trigger keydown from keyboard.js,

35b0c18

also added a bunch of missing semicolons (jshint)

Add comments

80abfdb

demsemicolons

f158e96

Partial fix of problems b/c keydown move

36e09c7

Final fixes?

3ba9c75

Pythonize me captin'

9fe4cd5

Made the method comments more pythonic by moving them within the method definitions.

Call trigger_keydown in merge_cells_api test

ff5c11b

Note to self

bfa67c7

Set cursor pos in edit mode before attempting to jump cells.

2007bec

Plus to minus shortcuts rebase fixes

6041713

Fix split shortcut

dd92b3e

s/set_cell_text/set_cells_text

7414ad5

get_text() before returning results

d0ec96b

Actually remove the trigger keydown method

a84fc03

minrk reviewed Mar 24, 2014
View reviewed changes

Fixed keyboard.js indent break

433c125

ellisonbg added this to the 2.0 milestone Mar 26, 2014

takluyver added a commit that referenced this pull request Mar 27, 2014

Merge pull request #5290 from jdfreder/dual_tests

78455ba

Add dual mode JS tests

takluyver merged commit 78455ba into ipython:master Mar 27, 2014

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#5290 from jdfreder/dual_tests

b715869

Add dual mode JS tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dual mode JS tests #5290

Add dual mode JS tests #5290

jdfreder commented Mar 7, 2014

ellisonbg commented Mar 7, 2014

ellisonbg Mar 7, 2014

ellisonbg Mar 7, 2014

ellisonbg Mar 7, 2014

ellisonbg commented Mar 10, 2014

jdfreder commented Mar 10, 2014

jdfreder commented Mar 12, 2014

jdfreder commented Mar 12, 2014

minrk commented Mar 12, 2014

jdfreder commented Mar 12, 2014

ellisonbg Mar 13, 2014

minrk Mar 13, 2014

ellisonbg Mar 14, 2014

ellisonbg commented Mar 13, 2014

jdfreder commented Mar 14, 2014

ellisonbg Mar 20, 2014

jdfreder Mar 20, 2014

ellisonbg commented Mar 20, 2014

jdfreder commented Mar 20, 2014

jdfreder commented Mar 20, 2014

ellisonbg commented Mar 20, 2014

minrk Mar 20, 2014

ellisonbg Mar 20, 2014

ellisonbg Mar 20, 2014

minrk Mar 20, 2014

jdfreder Mar 21, 2014

jdfreder commented Mar 21, 2014

minrk Mar 24, 2014

jdfreder commented Mar 24, 2014

takluyver commented Mar 27, 2014

minrk commented Mar 27, 2014



		// Utility functions.
		this.validate_state = function(message, mode, cell_index) {

Add dual mode JS tests #5290

Add dual mode JS tests #5290

Conversation

jdfreder commented Mar 7, 2014

how it works

ellisonbg commented Mar 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellisonbg commented Mar 10, 2014

jdfreder commented Mar 10, 2014

jdfreder commented Mar 12, 2014

jdfreder commented Mar 12, 2014

minrk commented Mar 12, 2014

jdfreder commented Mar 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellisonbg commented Mar 13, 2014

jdfreder commented Mar 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellisonbg commented Mar 20, 2014

jdfreder commented Mar 20, 2014

jdfreder commented Mar 20, 2014

ellisonbg commented Mar 20, 2014

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

jdfreder commented Mar 21, 2014

Choose a reason for hiding this comment

jdfreder commented Mar 24, 2014

takluyver commented Mar 27, 2014

minrk commented Mar 27, 2014