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

Bug 1132700 - Convert Mozmill testSafeBrowsingNotificationBar #91

Merged
merged 1 commit into from Mar 6, 2015

Conversation

Projects
None yet
3 participants
@galgeek
Copy link
Collaborator

galgeek commented Feb 18, 2015

This PR updates Chris Manchester's PR #30 for recent library updates.

@galgeek galgeek force-pushed the galgeek:safeBrowsingNotification branch 3 times, most recently from d34d21e to d0b84cf Feb 18, 2015

@galgeek galgeek self-assigned this Feb 19, 2015

@galgeek galgeek force-pushed the galgeek:safeBrowsingNotification branch 3 times, most recently from 7896c3b to 41a88ac Feb 20, 2015

("safebrowsing.notAnAttackButton.label",
"www.stopbadware.org",
"https://www.itisatrap.org/firefox/its-an-attack.html"),
]

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Those are not all URLs. I would suggest that in this case you name the variable self.test_data, and keep the dict and not use an array.


self.tabbar = self.browser.tabbar

# TODO? workaround for bug 1084289, mozmill: "aModule.browserWindow.maximize()

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Necessary for some locales where the button is not clickable inside the notification. It might be different for Marionette?

This comment has been minimized.

@galgeek

galgeek Feb 25, 2015

Author Collaborator

I can still reproduce in the latest nightly the clipping described here: https://bugzilla.mozilla.org/show_bug.cgi?id=1084289#c8

Is there a way to specify the browser window size for firefox-ui-tests?

This comment has been minimized.

@whimboo

whimboo Feb 26, 2015

Contributor

Can we press escape or so to not have to work with the close button for the moment? We could revert that once the underlying problem has been fixed.

self.prefs.restore_all_prefs
try:
self.windows.close_all([self.browser])
finally:

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Same as I mentioned on the other PR for safebrowsing.

self.prefs.set_pref('browser.safebrowsing.enabled', True)
self.prefs.set_pref('browser.safebrowsing.malware.enabled', True)

self.tabbar = self.browser.tabbar

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

nit: I don't see a real benefit for it.


self.marionette.set_context('chrome')
self.check_x_button()
self.marionette.set_context('content')

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Do the context switch inside of check_x_button with the with expression.

except:
return False

self.wait_for_condition(find_main_feature_el)

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

You should be able to do it like self.wait_for_condition(expected.element_present(by, locator)). The expected module you can find here: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/expected.py


self.wait_for_condition(find_main_feature_el)
self.assertRaises(NoSuchElementException, self.marionette.find_element,
'id', 'ignoreWarningButton')

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Always By.ID please.

buttons = element.find_elements('tag name', 'button')
found_button = None
for button in buttons:
if button.get_attribute('label') == label:

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

You should be able to use find_element with an anon attribute method here.

button_label = self.browser.get_property(label)
with self.marionette.using_context('chrome'):
tab_count = len(self.tabbar.tabs)
blocked_badware_page = (self.marionette.find_element(By.ID, 'content')

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

The variable name is confusing here. This should be the notification, right?

This comment has been minimized.

@galgeek

galgeek Feb 25, 2015

Author Collaborator

I agree. This appears to find the notification bar element.

I'm not sure that it's necessary to find the notification bar before finding its buttons. On the other hand, I'm not so sure that the get me out of here button that my current code finds is the one in the notification bar, not in the page, but I also wasn't sure about that with the find_button code.

This comment has been minimized.

@whimboo

whimboo Feb 26, 2015

Contributor

Please see the tabs_tabPanel code in the tabs.js module (line 673 ff) for the mozmill-tests. The panel is hard to get, but if you have the tab as reference you can get the panel id which itself can be used to retrieve the panel for this specific tab.

blocked_badware_page = (self.marionette.find_element(By.ID, 'content')
.find_element('anon attribute',
{'value': 'blocked-badware-page'}))
self.find_button(blocked_badware_page, button_label).click()

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Please always separate actions on retrieved elements.


self.wait_for_condition(lambda _: len(self.tabbar.tabs) == tab_count + 1)
self.wait_for_condition(lambda _: report_page in self.browser.navbar.locationbar.value)
self.tabbar.close_tab()

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Make sure to close all tabs in tearDown.


def check_get_me_out_of_here_button(self):
label = self.browser.get_property(
'safebrowsing.getMeOutOfHereButton.label')

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

This should fit into a single line. We have 100 chars available.

blocked_badware_page = (self.marionette.find_element(By.ID, 'content')
.find_element('anon attribute',
{'value': 'blocked-badware-page'}))
self.find_button(blocked_badware_page, label).click()

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Same as above.

self.find_button(blocked_badware_page, label).click()

expected_homepage = self.prefs.get_pref('browser.startup.homepage',
interface='nsIPrefLocalizedString')

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

This needs a property most likely on the BrowserWindow class to get the default homepage.

x_button = (self.marionette.find_element(By.ID, 'content')
.find_element('anon attribute', {'value': 'blocked-badware-page'})
.find_element('anon attribute',
{'class': 'messageCloseButton close-icon tabbable'}))

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Is messageCloseButton not enough here?

This comment has been minimized.

@galgeek

galgeek Feb 25, 2015

Author Collaborator

It’s not.

except:
return False

self.wait_for_condition(x_button_gone)

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

Why retrieving all elements again here? If you would operate on the already retrieved button, it should throw a StaleElementException, right?

@galgeek galgeek force-pushed the galgeek:safeBrowsingNotification branch 2 times, most recently from 4fd1970 to bcffcf4 Feb 25, 2015

label = self.browser.get_property('safebrowsing.getMeOutOfHereButton.label')
with self.marionette.using_context('chrome'):
# is this the button in the notification bar, or the button on the page?
button = (self.marionette.find_element(By.ID, 'content')

This comment has been minimized.

@chmanchester

chmanchester Feb 25, 2015

Contributor

This is the button on the notification bar.

This comment has been minimized.

@whimboo

whimboo Feb 26, 2015

Contributor

Lets call it notification panel for consistency and to not mix it up with any toolbar in the browser window.

# Give the browser a little time, because SafeBrowsing.jsm takes a
# while between start up and adding the example urls to the db.
# https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1194
# viewed 2015-02-24

This comment has been minimized.

@chmanchester
.find_element('anon attribute', {'label': label}))
button.click()

self.wait_for_condition(lambda _: len(self.browser.tabbar.tabs) == tab_count + 1)

This comment has been minimized.

@chmanchester

chmanchester Feb 25, 2015

Contributor

I think this can use our tabs library now with a callback clicking the button.

with self.marionette.using_context('chrome'):
self.marionette.execute_script("""
Cu.import("resource://gre/modules/Services.jsm");
Services.perms.remove(arguments[0], arguments[1]);

This comment has been minimized.

@chmanchester

chmanchester Feb 25, 2015

Contributor

Indent the js two more spaces.

"""Remove permission for host.
:param host: Web host, e.g., www.itisatrap.org
:param permission: Permission type, e.g., 'safe-browsing'

This comment has been minimized.

@whimboo

whimboo Feb 26, 2015

Contributor

We might want to improve the documentation later for that part to make it easier for testers to use it. No-one knows what to use e.g. as permission type.

This comment has been minimized.

@galgeek

galgeek Feb 26, 2015

Author Collaborator

I've added a little.

Permissions are defined in a variety of places, and I haven't found a master list.

@@ -413,6 +415,11 @@ def __init__(self, *args, **kwargs):
self._tabbar = None

@property
def default_homepage(self):
"""Returns the value of the preference browser.startup.homepage."""

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

Just say The default homepage as used by the current locale. Also please add the :returns line.

@@ -475,6 +482,17 @@ def callback(win):

BaseWindow.close(self, callback, force)

def get_loaded_url(self, url):

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

The name is misleading and not clear. I would call it get_final_url().

@@ -475,6 +482,17 @@ def callback(win):

BaseWindow.close(self, callback, force)

def get_loaded_url(self, url):
"""Loads the page at `url` and returns the resulting url.
This function enables testing redirects.

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

nit: Add a blank line between the summary and the more details explanation.

@@ -1,3 +1,4 @@
[test_security_notification.py]
[test_unknown_issuer.py]
[test_ssl_disabled_error_page.py]
[test_safe_browsing_notification.py]

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

please sort correctly.

from marionette_driver import (
By,
expected
)

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

With only two imports lets keep it on a single line like we do in all the other tests.

self.prefs.set_pref('browser.safebrowsing.malware.enabled', True)

# workaround for bug 1084289; in smaller windows, clipping causes X button to disapper
self.marionette.maximize_window()

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

This is not doing what you expect. I would suggest we kill that line and disable the button test. See bug 1139369

self.test_data = [
# Phishing URL info
{
'button_label': 'safebrowsing.notAForgeryButton.label',

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

This is not the label but a property. Please adjust the name.

button = self.marionette.find_element(By.ID, 'ignoreWarningButton')

# Wait for the DOM to receive events
time.sleep(1)

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

I may have asked this earlier but why do we need this is it is not an about:error page? If we do I would like to see it right next to the call to navigate().

label = self.browser.get_property(button_label)
button = (self.marionette.find_element(By.ID, 'content')
.find_element('anon attribute', {'label': label}))
button.click()

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

Define those elements outside of opener, and simply have a lambda as callback which is doing the click. That is way better to read.

def opener(tab):
label = self.browser.get_property(button_label)
button = (self.marionette.find_element(By.ID, 'content')
.find_element('anon attribute', {'label': label}))

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

This is operating on a notification bar. Here we need a new class for handling that. Please add a TODO item and file a bug for it.


# TODO: better workaround for timeout exceptions (especially stopbadware.org)
self.browser.tabbar.open_tab(opener)
self.wait_for_condition(lambda _: report_page in self.browser.navbar.locationbar.value)

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

Does the page load at all? Or what are you seeing? What about increasing the timeout to something larger? It's not something we do in Mozmill tests where a timeout of 30s works pretty good.

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

Also why not checking for marionette.get_url() here?

This comment has been minimized.

@galgeek

galgeek Mar 4, 2015

Author Collaborator

I always see the page begin to load, and I'm not seeing timeout errors today, for whatever that's worth.

Note, the timeout defined here is 5s, I believe, not 30s.

We are in chrome context here; marionette.get_url() requires content context.

def check_get_me_out_of_here_button(self):
with self.marionette.using_context('chrome'):
label = self.browser.get_property('safebrowsing.getMeOutOfHereButton.label')
button = (self.marionette.find_element(By.ID, 'content')

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

A TODO comment here too for the notificationbar class.


def check_x_button(self):
with self.marionette.using_context('chrome'):
button = (self.marionette.find_element(By.ID, 'content')

This comment has been minimized.

@whimboo

whimboo Mar 4, 2015

Contributor

Same here again.

@galgeek galgeek force-pushed the galgeek:safeBrowsingNotification branch 2 times, most recently from 8eb2880 to f84bbb2 Mar 4, 2015

@galgeek

This comment has been minimized.

Copy link
Collaborator Author

galgeek commented Mar 4, 2015

Thank you for the feedback!

This test loads not about:error but about:blocked, with has similar issues. I've moved the time.sleep() lines.

I am not seeing any timeout issues today.

Regarding marionette.get_url(), that requires content context, and for the method you asked about, we are otherwise always in chrome context, including closing the tab after the url check.

@galgeek galgeek force-pushed the galgeek:safeBrowsingNotification branch from f84bbb2 to 2a05ded Mar 4, 2015

button.click()

self.wait_for_condition(expected.element_present(By.ID, 'main-feature'))
self.wait_for_condition(expected.element_stale(button))

This comment has been minimized.

@whimboo

whimboo Mar 5, 2015

Contributor

I would flip those two checks. The button will become stale first.

self.assertEquals(self.marionette.get_url(), self.browser.get_final_url(unsafe_page))

# Clean up here since the permission gets set in this function
self.utils.remove_perms('www.itisatrap.org', 'safe-browsing')

This comment has been minimized.

@whimboo

whimboo Mar 5, 2015

Contributor

Make sure this is also part of tearDown, otherwise it will not be reset in case an assert above fails.

label = self.browser.get_property(button_property)
button = (self.marionette.find_element(By.ID, 'content')
.find_element('anon attribute', {'label': label}))
# TODO: better workaround for timeout exceptions (especially stopbadware.org)

This comment has been minimized.

@whimboo

whimboo Mar 5, 2015

Contributor

nit: add a blank line so we can separate the declaration block from actions.

This comment has been minimized.

@whimboo

whimboo Mar 5, 2015

Contributor

I assume the comment can also be removed? I don't see why we have to carry it around if all works now.

.find_element('anon attribute', {'label': label}))
# TODO: better workaround for timeout exceptions (especially stopbadware.org)
self.browser.tabbar.open_tab(lambda _: button.click())
self.wait_for_condition(lambda _: report_page in self.browser.navbar.locationbar.value)

This comment has been minimized.

@whimboo

whimboo Mar 5, 2015

Contributor

We should use that or mn.get_url(). I would propse the latter.

.find_element('anon attribute', {'label': label}))
button.click()

self.wait_for_condition(lambda mn: self.browser.default_homepage in mn.get_url())

This comment has been minimized.

@whimboo

whimboo Mar 5, 2015

Contributor

Good idea! That way we do not have to wait until the final page has been loaded.

button = (self.marionette.find_element(By.ID, 'content')
.find_element('anon attribute', {'value': 'blocked-badware-page'})
.find_element('anon attribute',
{'class': 'messageCloseButton close-icon tabbable'}))

This comment has been minimized.

@whimboo

whimboo Mar 5, 2015

Contributor

nit: This should align with the rounded opening bracket of line 115.

@galgeek galgeek force-pushed the galgeek:safeBrowsingNotification branch from 1d95172 to 1a6adf5 Mar 6, 2015

@whimboo whimboo merged commit 1a6adf5 into mozilla:master Mar 6, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment