[3335] Convert JS tests to Selenium #3601
Conversation
arovit
commented
May 7, 2018
arovit
commented
- 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 there 3. added a generalized method "trigger_keystrokes" to send keystrokes to browser
|
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. |
|
@takluyver can I please get your attention to this PR. |
|
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. |
|
Apologies for pushing. I understand you must be swamped with PR reviews and own work. |
|
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
| @@ -184,6 +184,11 @@ def add_cell(self, index=-1, cell_type="code", content=""): | |||
| if cell_type != 'code': | |||
| self.convert_cell_type(index=new_index, cell_type=cell_type) | |||
|
|
|||
| def delete_cell(self, index): | |||
| self.focus_cell(index) | |||
| self.to_command_mode | |||
There was a problem hiding this comment.
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.
my bad, I should have noticed. I will check on this.
notebook/tests/selenium/utils.py
Outdated
| for each_key_combination in keys: | ||
| keys = each_key_combination.split('-') | ||
| if len(keys) > 1: # key has modifiers eg. control, alt, shift | ||
| modifiers_keys = list(map(lambda x:getattr(Keys, x.upper()), keys[:-1])) |
There was a problem hiding this comment.
list(map(lambda is a pretty good sign that a list comprehension would be clearer. ;-)
There was a problem hiding this comment.
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') | ||
| assert remove_cells(notebook); |
There was a problem hiding this comment.
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.
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
| def test_empty_arrows_keys(notebook): | ||
| # delete all the cells | ||
| notebook.trigger_keydown('up') | ||
| notebook.trigger_keydown('down') |
There was a problem hiding this comment.
Let's get rid of the up/down, since they're not really testing anything, rename the function to test_delete_all_cells, and maybe move it into test_deletecell.py
…to test_deletecell.py
|
Thanks for reviewing, removed the up and down arrow movement and moved the logic into test_deletecell.py |
|
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') | ||
| def remove_cells(notebook): |
There was a problem hiding this comment.
Let's call this remove_all_cells, just to be clear what it's doing.
There was a problem hiding this comment.
Also, I think the implementation could be more efficient. If you look at what notebook.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 how many cells to delete. Then we can do it by deleting the first cell N times.
There was a problem hiding this comment.
makes sense. The current way is quite inefficient
| @@ -56,3 +55,6 @@ def test_delete_cells(notebook): | |||
| notebook.current_cell.send_keys('cv') | |||
| assert len(notebook.cells) == 2 | |||
| assert cell_is_deletable(notebook, 1) | |||
|
|
|||
| remove_cells(notebook) | |||
There was a problem hiding this comment.
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.
You are right. Sorry, did not notice as I just moved the deletion test to this file.
…nly cell to false
|
Thanks for reviewing, changed the code according to the comments. |
mpacer
changed the title
