New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add dual mode JS tests #5290
Add dual mode JS tests #5290
Conversation
Other things to test:
|
|
||
|
||
// Utility functions. | ||
this.validate_state = function(message, mode, cell_index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this into utils.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's actually move most of these utility function to utils.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some tests, especially those involving markdown or heading cells, we should test the rendered state as well.
Closes #4887 |
|
@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. |
Ahh and I forgot to mention, I removed the arrow keys test since the |
Can you remove the unused |
Yup, went ahead and removed it... Also jshinted the file since I was touching it anyways. |
}; | ||
|
||
// Emulate a keydown in the notebook. | ||
casper.trigger_keydown = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fine.
Only minor changes needed. |
Changes addressed |
@@ -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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you use this.trigger_keydown
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This looks good, but I am seeing a test failure:
|
That particular failure is very concerning because it means you've encountered a situation where two cells are selected at once. 😦 |
@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. Select the first cell by clicking on it. Select the last cell by clicking on it. FAILURE HERE The test continues to try some more things... |
Tests pass for me locally, once Travis passes, lets merge. |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you removed the export here, but not the actual function itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with trigger_keydown
being removed from here, but definitely think that event_to_shortcut
should stay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, nevermind, it is still here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I re-removed the trigger keydown method... It was un-removed during a bad rebase. erg |
@@ -210,7 +201,7 @@ IPython.keyboard = (function (IPython) { | |||
if (!suppress_help_update) { | |||
// update the keyboard shortcuts notebook help | |||
$([IPython.events]).trigger('rebuild.QuickHelp'); | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these indentation changes are not correct
I just fixed the broken indents |
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? |
Don't think so. |
Add dual mode JS tests
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