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

Update selenium tests #3412

Merged
merged 8 commits into from Mar 23, 2018
Merged

Update selenium tests #3412

merged 8 commits into from Mar 23, 2018

Conversation

@mpacer
Copy link
Member

@mpacer mpacer commented Mar 9, 2018

This PR comes from the same pair programming session with @takluyver that #3411 came from.

This also depends upon #3411.

current_items = get_list_items(authenticated_browser)
current_items_links = [item["link"] for item in current_items]
stored_items = visited_dict.pop(authenticated_browser.current_url)
stored_items_links = [item["link"] for item in stored_items]
Copy link
Member

@takluyver takluyver Mar 13, 2018

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.

Loading

items = get_list_items(authenticated_browser)
visited_dict[authenticated_browser.current_url] = items
print(authenticated_browser.current_url, len(items))
if len(items)>1:
Copy link
Member

@takluyver takluyver Mar 13, 2018

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.

Loading

Copy link
Member Author

@mpacer mpacer Mar 16, 2018

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?

Loading

Copy link
Member

@takluyver takluyver Mar 20, 2018

Choose a reason for hiding this comment

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

Yep, that's right.

Loading

Copy link
Member Author

@mpacer mpacer Mar 20, 2018

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

Loading

@takluyver takluyver added this to the 5.5 milestone Mar 13, 2018
raise("You currently ")
wait_for_selector(browser, '.item_link')
items = get_list_items(browser)
return [i for i in items if 'tree' in i['link'] and i['label'] != '..']
Copy link
Member

@takluyver takluyver Mar 22, 2018

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 has tree in the name. Maybe that's unlikely, but I think it's worth doing it a bit more thoroughly.

Loading

Copy link
Member Author

@mpacer mpacer Mar 22, 2018

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 (with url_in_tree)

Loading

"""
try:
assert 'tree' in browser.current_url
except PageError:
Copy link
Member

@takluyver takluyver Mar 22, 2018

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 writing if '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.

Loading

Copy link
Member Author

@mpacer mpacer Mar 22, 2018

Choose a reason for hiding this comment

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

fair enough — changed.

Loading

@takluyver takluyver merged commit dc1eeab into jupyter:master Mar 23, 2018
4 checks passed
Loading
@takluyver
Copy link
Member

@takluyver takluyver commented Mar 23, 2018

🎉

Loading

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 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