- Notifications
You must be signed in to change notification settings - Fork5.5k
[3335] Convert JS tests to Selenium#3601
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
arovit commentedMay 7, 2018
- Converting 'empty_arrows_keys.js' into selenium based testcase
- Moved "delete_cell" method to utils.py and modified references to use it from there
- Added a generalized method "trigger_keystrokes" to send keystrokes to browser
2. Moved "delete_cell" method to utils.py and modified references to use it from there3. added a generalized method "trigger_keystrokes" to send keystrokes to browser
arovit commentedMay 7, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The selenium test "test_dualmode_insertcell.py" fails here. I am inclined towards saying my changes don't affect this testcase but let me see if I can fix it. |
arovit commentedMay 9, 2018
@takluyver can I please get your attention to this PR. |
takluyver commentedMay 9, 2018
Please don't pester me, especially just one day after opening it. I have other things to do as well! I'm also not the only person who can review PRs, at least in theory. |
arovit commentedMay 21, 2018
Apologies for pushing. I understand you must be swamped with PR reviews and own work. |
takluyver commentedMay 21, 2018
No worries. I'll have a look now. For future reference, it's OK to give a PR a friendly ping if it goes a couple of weeks without anyone looking at it. It might have been forgotten, or the maintainers might think you're going to do something else. But "can I please get your attention" comes across as a bit demanding. One way I sometimes do this is to ask the maintainers "is there anything more I should do on this?". |
notebook/tests/selenium/utils.py Outdated
| defdelete_cell(self,index): | ||
| self.focus_cell(index) | ||
| self.to_command_mode |
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 should have() at the end, otherwise it's not doing anything. I can see it's just moved from another file, but it's worth checking while we're looking at it.
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.
my bad, I should have noticed. I will check on this.
notebook/tests/selenium/utils.py Outdated
| foreach_key_combinationinkeys: | ||
| keys=each_key_combination.split('-') | ||
| iflen(keys)>1:# key has modifiers eg. control, alt, shift | ||
| modifiers_keys=list(map(lambdax:getattr(Keys,x.upper()),keys[:-1])) |
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.
list(map(lambda is a pretty good sign that a list comprehension would be clearer. ;-)
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.
Ah, muscle memory of using maps in Python2.
You are right, "list comprehension" would be much clearer.
| # delete all the cells | ||
| notebook.trigger_keydown('up') | ||
| notebook.trigger_keydown('down') | ||
| assertremove_cells(notebook); |
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.
The original JS version of the test removes the cells before doing the up/down keys. So it's testing up/down inside an empty notebook. But to be honest, I have no idea what that test is meant to achieve. You can't even really have an empty notebook - if you delete the last cell, it creates a new one for you. Like Nature, Jupyter abhors a vacuum.
Maybe it would be more use to test that behaviour instead - delete all cells, check there's a new one there.
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.
Hmm, I agree that test was testing minimal behavior. Yes, Sure, I can add do this check.
…dded assert to check presence of cell and remove 'return True' from remove_cells
| deftest_empty_arrows_keys(notebook): | ||
| # delete all the cells | ||
| notebook.trigger_keydown('up') | ||
| notebook.trigger_keydown('down') |
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 get rid of the up/down, since they're not really testing anything, rename the function totest_delete_all_cells, and maybe move it intotest_deletecell.py
…to test_deletecell.py
arovit commentedMay 31, 2018
Thanks for reviewing, removed the up and down arrow movement and moved the logic into test_deletecell.py |
arovit commentedJun 1, 2018
Please let me know if anything else needs to be worked on here. |
| notebook.focus_cell(index) | ||
| notebook.to_command_mode | ||
| notebook.current_cell.send_keys('dd') | ||
| defremove_cells(notebook): |
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 call thisremove_all_cells, just to be clear what it's doing.
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.
Also, I think the implementation could be more efficient. If you look at whatnotebook.index() does, it's actually querying the browser for the full list of cells to find out where the given cell is. We don't need to know that, we just need to know howmany cells to delete. Then we can do it by deleting the first cell N times.
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.
makes sense. The current way is quite inefficient
| assertlen(notebook.cells)==2 | ||
| assertcell_is_deletable(notebook,1) | ||
| remove_cells(notebook) |
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.
Careful! If you look at the code just above this, it's making one of the cells undeletable. So we're not actually deleting all the cells 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.
You are right. Sorry, did not notice as I just moved the deletion test to this file.
arovit commentedJun 8, 2018
Thanks for reviewing, changed the code according to the comments. |