Skip to content
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

[3335] Convert JS tests to Selenium #3601

Merged
merged 6 commits into from Jun 10, 2018
Merged

Conversation

@arovit
Copy link
Contributor

@arovit arovit commented May 7, 2018

  1. Converting 'empty_arrows_keys.js' into selenium based testcase
  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
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
@arovit
Copy link
Contributor Author

@arovit arovit commented May 7, 2018

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.

Loading

@arovit
Copy link
Contributor Author

@arovit arovit commented May 9, 2018

@takluyver can I please get your attention to this PR.

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented May 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.

Loading

@arovit
Copy link
Contributor Author

@arovit arovit commented May 21, 2018

Apologies for pushing. I understand you must be swamped with PR reviews and own work.

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented May 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?".

Loading

@@ -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
Copy link
Member

@takluyver takluyver May 21, 2018

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.

Loading

Copy link
Contributor Author

@arovit arovit May 21, 2018

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.

Loading

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]))
Copy link
Member

@takluyver takluyver May 21, 2018

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. ;-)

Loading

Copy link
Contributor Author

@arovit arovit May 21, 2018

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.

Loading

# delete all the cells
notebook.trigger_keydown('up')
notebook.trigger_keydown('down')
assert remove_cells(notebook);
Copy link
Member

@takluyver takluyver May 21, 2018

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.

Loading

Copy link
Contributor Author

@arovit arovit May 21, 2018

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.

Loading

…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')
Copy link
Member

@takluyver takluyver May 27, 2018

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 to test_delete_all_cells, and maybe move it into test_deletecell.py

Loading

@arovit
Copy link
Contributor Author

@arovit arovit commented May 31, 2018

Thanks for reviewing, removed the up and down arrow movement and moved the logic into test_deletecell.py

Loading

@arovit
Copy link
Contributor Author

@arovit arovit commented Jun 1, 2018

Please let me know if anything else needs to be worked on here.

Loading

notebook.focus_cell(index)
notebook.to_command_mode
notebook.current_cell.send_keys('dd')
def remove_cells(notebook):
Copy link
Member

@takluyver takluyver Jun 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this remove_all_cells, just to be clear what it's doing.

Loading

Copy link
Member

@takluyver takluyver Jun 2, 2018

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 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.

Loading

Copy link
Contributor Author

@arovit arovit Jun 8, 2018

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

Loading

@@ -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)
Copy link
Member

@takluyver takluyver Jun 2, 2018

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.

Loading

Copy link
Contributor Author

@arovit arovit Jun 8, 2018

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.

Loading

@arovit
Copy link
Contributor Author

@arovit arovit commented Jun 8, 2018

Thanks for reviewing, changed the code according to the comments.

Loading

@arovit arovit closed this Jun 8, 2018
@arovit arovit reopened this Jun 8, 2018
@takluyver takluyver added this to the 5.6 milestone Jun 10, 2018
@takluyver takluyver merged commit ebe0176 into jupyter:master Jun 10, 2018
4 checks passed
Loading
@mpacer mpacer changed the title [WIP] [3335] Convert JS tests to Selenium [3335] Convert JS tests to Selenium Jun 13, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants