- Notifications
You must be signed in to change notification settings - Fork5.5k
Add find replace test#3630
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
notebook/tests/selenium/utils.py Outdated
| defesc(browser,k): | ||
| """Send key combination esc+(k)""" | ||
| ActionChains(browser)\ | ||
| .key_down(Keys.ESCAPE).send_keys(k).key_up(Keys.ESCAPE).perform() |
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.
This isn't necessary -Esc isn't used as a modifier key that you have to hold down while pressing other keys. Just callself.to_command_mode() and then send the other keys as normal.
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 see okay. Makes sense, will call self.to_command_mode()
notebook/tests/selenium/utils.py Outdated
| deffind_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 |
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.
Are there any specific elements on the form? We can wait for an element to appear.
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 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)
notebook/tests/selenium/utils.py Outdated
| 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 |
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.
We shouldn't need to construct JS just to set the value of a form field like this, I think.
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.
right, let me see how I can make it better.
notebook/tests/selenium/utils.py Outdated
| 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() |
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.
What's this 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.
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 !
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.
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.
arovit commentedJun 1, 2018
Thanks, added 'ids' in the find_and_replace form. Code looks much clearer now. |
| varallCellsButton=$('<button/>') | ||
| .append($('<i/>').addClass('fa fa-arrows-v')) | ||
| .attr('id','allcells_id') |
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.
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, likefindreplace_allcells_btn - and likewise for the other ones added here.
notebook/tests/selenium/utils.py Outdated
| self.to_command_mode() | ||
| self.current_cell=cell | ||
| deffind_and_replace(self,index=0,find_txt='',replace_txt='',replace_all=False): |
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.
Looks like thereplace_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.
| @@ -0,0 +1,25 @@ | |||
| importos | |||
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 the filetest_find_and_replace.py - we'll probably later want to add tests that aren't doing apply all.
takluyver commentedJun 2, 2018
It is looking much clearer :-) |
…me of the testcase as mentioned in the comments
arovit commentedJun 8, 2018
Thanks for reviewing, renamed the ids, removed the replace_all parameter and changed the name of testcase. |
takluyver commentedJun 10, 2018
Thanks! |
Adding a selenium based test for finding and replacing in selected cell applied on all cells