- Notifications
You must be signed in to change notification settings - Fork5.5k
Update selenium tests#3412
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
Bug found during pair programming with@mpacer
Originally written by@takluyver while pair programming
| current_items=get_list_items(authenticated_browser) | ||
| current_items_links= [item["link"]foritemincurrent_items] | ||
| stored_items=visited_dict.pop(authenticated_browser.current_url) | ||
| stored_items_links= [item["link"]foriteminstored_items] |
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.
Neat, I like the comparison of what's here now vs what was here before.
| items=get_list_items(authenticated_browser) | ||
| visited_dict[authenticated_browser.current_url]=items | ||
| print(authenticated_browser.current_url,len(items)) | ||
| iflen(items)>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.
Why does it not work with 1 item? Does it catch the link to go up a directory?
I think it would be good to filter out which items are directories, so that this doesn't accidentally load a notebook or another file.
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.
For the first, I was using that to filter out.. but I see now that that actually introduces weird behaviour at the root.
For the second, I gather that I can distinguish that based on the link target being on the/tree vs./notebook or/file endpoints?
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.
Yep, that's right.
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.
Oops! i forgot about this simple fix and have been focused on getting the notebook navigation stuff working on the markdown stuff. I'll fix this quickly
| raise("You currently ") | ||
| wait_for_selector(browser,'.item_link') | ||
| items=get_list_items(browser) | ||
| return [iforiinitemsif'tree'ini['link']andi['label']!='..'] |
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 will go wrong for any file that hastree in the name. Maybe that's unlikely, but I think it's worth doing it a bit more thoroughly.
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.
ok addressed by using an explicit check for whether urls point to something in the tree (withurl_in_tree)
| """ | ||
| try: | ||
| assert'tree'inbrowser.current_url | ||
| exceptPageError: |
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 is a convoluted way of writingif 'tree' not in browser.current_url: , even if it was catching the right error. Let's stick to the simple if statement rather than using assert to achieve the same thing.
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.
fair enough — changed.
takluyver commentedMar 23, 2018
🎉 |
This PR comes from the same pair programming session with@takluyver that#3411 came from.
This also depends upon#3411.