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

Add find replace test #3630

Merged
merged 6 commits into from Jun 10, 2018
Merged

Add find replace test #3630

merged 6 commits into from Jun 10, 2018

Conversation

@arovit
Copy link
Contributor

@arovit arovit commented May 21, 2018

Adding a selenium based test for finding and replacing in selected cell applied on all cells

def esc(browser, k):
"""Send key combination esc+(k)"""
ActionChains(browser)\
.key_down(Keys.ESCAPE).send_keys(k).key_up(Keys.ESCAPE).perform()
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.

This isn't necessary - Esc isn't used as a modifier key that you have to hold down while pressing other keys. Just call self.to_command_mode() and then send the other keys as normal.

Loading

Copy link
Contributor Author

@arovit arovit May 31, 2018

Choose a reason for hiding this comment

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

I see okay. Makes sense, will call self.to_command_mode()

Loading

def find_and_replace(self, index=0, find_txt='', replace_txt='', replace_all=False):
self.focus_cell(index)
esc(self.browser, 'F')
time.sleep(1) # TODO: find better way to fill the find and replace form
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.

Are there any specific elements on the form? We can wait for an element to appear.

Loading

Copy link
Contributor Author

@arovit arovit May 31, 2018

Choose a reason for hiding this comment

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

Hmm, I don't want to comment now before trying again (I rem trying the wait logic while working on this, but couldn't get the wait working because I couldn't find any specific element, but let me retry)

Loading

esc(self.browser, 'F')
time.sleep(1) # TODO: find better way to fill the find and replace form
self.browser.find_elements_by_css_selector("button.btn.btn-default.btn-sm")[2].click()
JS = "document.getElementsByClassName('form-control input-sm')[0].setAttribute('value', '%s')"%find_txt
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.

We shouldn't need to construct JS just to set the value of a form field like this, I think.

Loading

Copy link
Contributor Author

@arovit arovit May 31, 2018

Choose a reason for hiding this comment

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

right, let me see how I can make it better.

Loading

self.focus_cell(index)
esc(self.browser, 'F')
time.sleep(1) # TODO: find better way to fill the find and replace form
self.browser.find_elements_by_css_selector("button.btn.btn-default.btn-sm")[2].click()
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.

What's this doing?

Loading

Copy link
Contributor Author

@arovit arovit May 31, 2018

Choose a reason for hiding this comment

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

This code is basically for clicking double "upside down" arrow (third button) on the find and replace form to enable global replace. It's kinda hacky way of doing it, since the buttons don't have any specific ids on them which made it harder to fetch them directly. But let me will revisit this commit to see if we can make it more robust. Thanks !

Loading

Copy link
Member

@takluyver takluyver May 31, 2018

Choose a reason for hiding this comment

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

We could add an ID, so long as that's not going to cause problems. If you do, try closing and reopening the dialog a few times so it's created several successive buttons with the same ID, and check that the button still works.

Loading

@arovit
Copy link
Contributor Author

@arovit arovit commented Jun 1, 2018

Thanks, added 'ids' in the find_and_replace form. Code looks much clearer now.

Loading

@@ -163,6 +163,7 @@ define([

var allCellsButton = $('<button/>')
.append($('<i/>').addClass('fa fa-arrows-v'))
.attr('id', 'allcells_id')
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.

IDs don't need to have a _id suffix, but they do need to be unique on the whole page - that includes IDs that might come from extensions or from output produced in a notebook. So let's make them a bit more specific, like findreplace_allcells_btn - and likewise for the other ones added here.

Loading

@@ -100,6 +100,16 @@ def focus_cell(self, index=0):
self.to_command_mode()
self.current_cell = cell

def find_and_replace(self, index=0, find_txt='', replace_txt='', replace_all=False):
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.

Looks like the replace_all parameter doesn't do anything. It's fine to drop it for now - it can always be added back when there's a test that wants to use it.

Loading

@@ -0,0 +1,25 @@
import os
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 the file test_find_and_replace.py - we'll probably later want to add tests that aren't doing apply all.

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Jun 2, 2018

It is looking much clearer :-)

Loading

…me of the testcase as mentioned in the comments
@arovit
Copy link
Contributor Author

@arovit arovit commented Jun 8, 2018

Thanks for reviewing, renamed the ids, removed the replace_all parameter and changed the name of testcase.

Loading

@takluyver takluyver added this to the 5.6 milestone Jun 10, 2018
@takluyver takluyver merged commit 702b67d into jupyter:master Jun 10, 2018
4 checks passed
Loading
@takluyver
Copy link
Member

@takluyver takluyver commented Jun 10, 2018

Thanks!

Loading

@mpacer mpacer changed the title [WIP: 838] Add find replace test Add find replace test 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