From 3cfbdcac055a11975181b64b4804cb9c6615cec4 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Wed, 19 Jun 2019 16:36:18 -0400 Subject: [PATCH] Add safe send_keys methods to FunctionalTest. Stop implicit wait. Add methods to more reliably send key events to elements. This seems to have fixed one of the most pernicious flaky tests, test_journalist_interface_ui_with_modal, allowing removal of the hideous retry hack that could make the tests take minutes longer. Use them to replace other usages of send_keys, including some that have definitely been flaky, though not as bad as that one. Also simplify the safe_click* methods, as WebDriverWait.until returns the result of the wait condition, and some of the other element manipulations were unnecessary, given the expected conditions. Stop setting a default implicit wait in the web drivers. If an element might not be present, we should use explicit waits instead of adding this potential delay to every element-locating call. There's also ample Selenium folklore that mixing implicit and explicit waits leads to unpredictable timeouts and possible bad behavior. Rename _alert_wait and _alert_accept. Double the timeout in alert_wait. This helped with some timeouts waiting for confirmation popups. --- .../tests/functional/functional_test.py | 121 ++++++++++++------ .../functional/journalist_navigation_steps.py | 86 ++++--------- .../functional/source_navigation_steps.py | 31 ++--- .../tests/functional/test_journalist.py | 19 +-- 4 files changed, 124 insertions(+), 133 deletions(-) diff --git a/securedrop/tests/functional/functional_test.py b/securedrop/tests/functional/functional_test.py index a6c720d318..711d9f0de1 100644 --- a/securedrop/tests/functional/functional_test.py +++ b/securedrop/tests/functional/functional_test.py @@ -24,7 +24,6 @@ from selenium import webdriver from selenium.common.exceptions import NoAlertPresentException from selenium.common.exceptions import WebDriverException -from selenium.webdriver.common.action_chains import ActionChains from selenium.webdriver.common.by import By from selenium.webdriver.remote.remote_connection import LOGGER from selenium.webdriver.support import expected_conditions @@ -56,6 +55,7 @@ class FunctionalTest(object): session_expiration = 30 secret_message = "These documents outline a major government invasion of privacy." timeout = 10 + poll_frequency = 0.1 accept_languages = None driver = None @@ -124,7 +124,6 @@ def create_firefox_driver(self): ) self.firefox_driver.set_window_position(0, 0) self.firefox_driver.set_window_size(1024, 768) - self.firefox_driver.implicitly_wait(self.timeout) logging.info("Created Firefox web driver") break except Exception as e: @@ -149,21 +148,7 @@ def drivers(self): self.create_firefox_driver() self.switch_to_torbrowser_driver() - - # Polls the DOM to wait for elements. To read more about why - # this is necessary: - # - # http://www.obeythetestinggoat.com/how-to-get-selenium-to-wait-for-page-load-after-a-click.html - # - # A value of 5 is known to not be enough in some cases, when - # the machine hosting the tests is slow, reason why it was - # raised to 10. Setting the value to 60 or more would surely - # cover even the slowest of machine. However it also means - # that a test failing to find the desired element in the DOM - # will only report failure after 60 seconds which is painful - # for quickly debuging. - # - self.driver.implicitly_wait(self.timeout) + self.driver.implicitly_wait(0) yield @@ -200,7 +185,7 @@ def sd_servers(self): self.source_app = source_app.create_app(config) self.journalist_app = journalist_app.create_app(config) - self.journalist_app.config['WTF_CSRF_ENABLED'] = True + self.journalist_app.config["WTF_CSRF_ENABLED"] = True self.__context = self.journalist_app.app_context() self.__context.push() @@ -296,45 +281,107 @@ def wait_for(self, function_with_assertion, timeout=None): try: return function_with_assertion() except (AssertionError, WebDriverException): - time.sleep(0.1) + time.sleep(self.poll_frequency) # one more try, which will raise any errors if they are outstanding return function_with_assertion() def safe_click_by_id(self, element_id): - WebDriverWait(self.driver, self.timeout).until( + """ + Clicks the element with the given ID attribute. + + Returns: + el: The element, if found. + + Raises: + selenium.common.exceptions.TimeoutException: If the element cannot be found in time. + + """ + el = WebDriverWait(self.driver, self.timeout, self.poll_frequency).until( expected_conditions.element_to_be_clickable((By.ID, element_id)) ) - - el = self.wait_for(lambda: self.driver.find_element_by_id(element_id)) - el.location_once_scrolled_into_view - ActionChains(self.driver).move_to_element(el).click().perform() + el.click() + return el def safe_click_by_css_selector(self, selector): - WebDriverWait(self.driver, self.timeout).until( + """ + Clicks the first element with the given CSS selector. + + Returns: + el: The element, if found. + + Raises: + selenium.common.exceptions.TimeoutException: If the element cannot be found in time. + + """ + el = WebDriverWait(self.driver, self.timeout, self.poll_frequency).until( expected_conditions.element_to_be_clickable((By.CSS_SELECTOR, selector)) ) - - el = self.wait_for(lambda: self.driver.find_element_by_css_selector(selector)) - el.location_once_scrolled_into_view - ActionChains(self.driver).move_to_element(el).click().perform() + el.click() + return el def safe_click_all_by_css_selector(self, selector, root=None): + """ + Clicks each element that matches the given CSS selector. + + Returns: + els (list): The list of elements that matched the selector. + + Raises: + selenium.common.exceptions.TimeoutException: If the element cannot be found in time. + + """ if root is None: root = self.driver els = self.wait_for(lambda: root.find_elements_by_css_selector(selector)) for el in els: - el.location_once_scrolled_into_view - self.wait_for(lambda: el.is_enabled() and el.is_displayed()) - ActionChains(self.driver).move_to_element(el).click().perform() + clickable_el = WebDriverWait(self.driver, self.timeout, self.poll_frequency).until( + expected_conditions.element_to_be_clickable((By.CSS_SELECTOR, selector)) + ) + clickable_el.click() + return els + + def safe_send_keys_by_id(self, element_id, text): + """ + Sends the given text to the element with the specified ID. - def _alert_wait(self, timeout=None): + Returns: + el: The element, if found. + + Raises: + selenium.common.exceptions.TimeoutException: If the element cannot be found in time. + + """ + el = WebDriverWait(self.driver, self.timeout, self.poll_frequency).until( + expected_conditions.element_to_be_clickable((By.ID, element_id)) + ) + el.send_keys(text) + return el + + def safe_send_keys_by_css_selector(self, selector, text): + """ + Sends the given text to the first element with the given CSS selector. + + Returns: + el: The element, if found. + + Raises: + selenium.common.exceptions.TimeoutException: If the element cannot be found in time. + + """ + el = WebDriverWait(self.driver, self.timeout, self.poll_frequency).until( + expected_conditions.element_to_be_clickable((By.CSS_SELECTOR, selector)) + ) + el.send_keys(text) + return el + + def alert_wait(self, timeout=None): if timeout is None: - timeout = self.timeout - WebDriverWait(self.driver, timeout).until( + timeout = self.timeout * 2 + WebDriverWait(self.driver, timeout, self.poll_frequency).until( expected_conditions.alert_is_present(), "Timed out waiting for confirmation popup." ) - def _alert_accept(self): + def alert_accept(self): # adapted from https://stackoverflow.com/a/34795883/837471 def alert_is_not_present(object): """ Expect an alert to not be present.""" @@ -346,6 +393,6 @@ def alert_is_not_present(object): return True self.driver.switch_to.alert.accept() - WebDriverWait(self.driver, self.timeout).until( + WebDriverWait(self.driver, self.timeout, self.poll_frequency).until( alert_is_not_present, "Timed out waiting for confirmation popup to disappear." ) diff --git a/securedrop/tests/functional/journalist_navigation_steps.py b/securedrop/tests/functional/journalist_navigation_steps.py index 8bb12f1151..f8f8ef45b2 100644 --- a/securedrop/tests/functional/journalist_navigation_steps.py +++ b/securedrop/tests/functional/journalist_navigation_steps.py @@ -69,14 +69,9 @@ def return_downloaded_content(self, url, cookies): def _input_text_in_login_form(self, username, password, token): self.driver.get(self.journalist_location + "/login") - username_field = self.driver.find_element_by_css_selector('input[name="username"]') - username_field.send_keys(username) - - password_field = self.driver.find_element_by_css_selector('input[name="password"]') - password_field.send_keys(password) - - token_field = self.driver.find_element_by_css_selector('input[name="token"]') - token_field.send_keys(token) + self.safe_send_keys_by_css_selector('input[name="username"]', username) + self.safe_send_keys_by_css_selector('input[name="password"]', password) + self.safe_send_keys_by_css_selector('input[name="token"]', token) def _try_login_user(self, username, password, token): self._input_text_in_login_form(username, password, token) @@ -270,9 +265,7 @@ def _admin_updates_logo_image(self): dir_name = dirname(dirname(dirname(os.path.abspath(__file__)))) image_path = os.path.abspath(os.path.join(dir_name, "static/i/logo.png")) - logo_upload_input = self.wait_for(lambda: self.driver.find_element_by_id("logo-upload")) - logo_upload_input.location_once_scrolled_into_view - logo_upload_input.send_keys(image_path) + self.safe_send_keys_by_id("logo-upload", image_path) self.safe_click_by_id("submit-logo-update") @@ -285,22 +278,17 @@ def updated_image(): self.wait_for(updated_image, timeout=self.timeout * 6) def _add_user(self, username, first_name="", last_name="", is_admin=False, hotp=None): - username_field = self.driver.find_element_by_css_selector('input[name="username"]') - username_field.send_keys(username) + self.safe_send_keys_by_css_selector('input[name="username"]', username) if first_name: - first_name_field = self.driver.find_element_by_id("first_name") - first_name_field.send_keys(first_name) + self.safe_send_keys_by_id("first_name", first_name) if last_name: - last_name_field = self.driver.find_element_by_id("last_name") - last_name_field.send_keys(last_name) + self.safe_send_keys_by_id("last_name", last_name) if hotp: - hotp_checkbox = self.driver.find_element_by_css_selector('input[name="is_hotp"]') - hotp_checkbox.click() - hotp_secret = self.driver.find_element_by_css_selector('input[name="otp_secret"]') - hotp_secret.send_keys(hotp) + self.safe_click_all_by_css_selector('input[name="is_hotp"]') + self.safe_send_keys_by_css_selector('input[name="otp_secret"]', hotp) if is_admin: self.safe_click_by_css_selector('input[name="is_admin"]') @@ -310,8 +298,7 @@ def _add_user(self, username, first_name="", last_name="", is_admin=False, hotp= self.wait_for(lambda: self.driver.find_element_by_id("check-token")) def _admin_adds_a_user(self, is_admin=False, new_username=""): - self.wait_for(lambda: self.driver.find_element_by_id("add-user").is_enabled()) - self.safe_click_by_css_selector("button#add-user") + self.safe_click_by_id("add-user") self.wait_for(lambda: self.driver.find_element_by_id("username")) @@ -342,8 +329,7 @@ def _admin_adds_a_user(self, is_admin=False, new_username=""): self.create_new_totp(shared_secret) # Verify the two-factor authentication - token_field = self.driver.find_element_by_css_selector('input[name="token"]') - token_field.send_keys(str(self.new_totp.now())) + self.safe_send_keys_by_css_selector('input[name="token"]', str(self.new_totp.now())) self.safe_click_by_css_selector("button[type=submit]") def user_token_added(): @@ -361,8 +347,8 @@ def _admin_deletes_user(self): for i in range(3): try: self.safe_click_by_css_selector(".delete-user") - self._alert_wait() - self._alert_accept() + self.alert_wait() + self.alert_accept() break except TimeoutException: # Selenium has failed to click, and the confirmation @@ -389,9 +375,7 @@ def test_alert_sent(): def _logout(self): # Click the logout link - logout_link = self.driver.find_element_by_id("link-logout") - logout_link.send_keys(" ") - logout_link.click() + self.safe_click_by_id("link-logout") self.wait_for(lambda: self.driver.find_element_by_css_selector(".login-form")) # Logging out should redirect back to the login page @@ -530,10 +514,8 @@ def can_edit_user(): new_characters = "2" new_username = self.new_user["username"] + new_characters - username_field = self.driver.find_element_by_css_selector('input[name="username"]') - username_field.send_keys(new_characters) - update_user_btn = self.driver.find_element_by_css_selector("button[type=submit]") - update_user_btn.click() + self.safe_send_keys_by_css_selector('input[name="username"]', new_characters) + self.safe_click_by_css_selector("button[type=submit]") def user_edited(): if not hasattr(self, "accept_languages"): @@ -686,7 +668,7 @@ def _journalist_composes_reply(self): "Thanks for the documents. Can you submit more " "information about the main program?" ) self.wait_for(lambda: self.driver.find_element_by_id("reply-text-field")) - self.driver.find_element_by_id("reply-text-field").send_keys(reply_text) + self.safe_send_keys_by_id("reply-text-field", reply_text) def _journalist_sends_reply_to_source(self): self._journalist_composes_reply() @@ -728,13 +710,8 @@ def _visit_edit_secret(self, otp_type, tooltip_text=''): alert.accept() def _set_hotp_secret(self): - def find_hotp_input(): - return self.driver.find_element_by_css_selector('input[name="otp_secret"]') - - hotp_input = self.wait_for(find_hotp_input) - hotp_input.send_keys("123456") - submit_button = self.driver.find_element_by_css_selector("button[type=submit]") - submit_button.click() + self.safe_send_keys_by_css_selector('input[name="otp_secret"]', "123456") + self.safe_click_by_css_selector("button[type=submit]") def _visit_edit_hotp_secret(self): self._visit_edit_secret( @@ -786,8 +763,8 @@ def _admin_visits_reset_2fa_hotp(self): if not hasattr(self, "accept_languages"): assert tip_text == "Reset 2FA for hardware tokens like Yubikey" self.safe_click_by_id("button-reset-two-factor-hotp") - self._alert_wait() - self._alert_accept() + self.alert_wait() + self.alert_accept() break except TimeoutException: # Selenium has failed to click, and the confirmation @@ -816,8 +793,8 @@ def _admin_visits_reset_2fa_totp(self): if not hasattr(self, "accept_languages"): assert tip_text == "Reset 2FA for mobile apps such as FreeOTP or Google Authenticator" self.safe_click_by_id("button-reset-two-factor-totp") - self._alert_wait() - self._alert_accept() + self.alert_wait() + self.alert_accept() def _admin_creates_a_user(self, hotp): self.safe_click_by_id("add-user") @@ -885,21 +862,12 @@ def _journalist_fail_login_many(self): self._try_login_user(self.user, "worse", "mocked") def _admin_enters_journalist_account_details_hotp(self, username, hotp_secret): - username_field = self.driver.find_element_by_css_selector('input[name="username"]') - username_field.send_keys(username) - - hotp_secret_field = self.driver.find_element_by_css_selector('input[name="otp_secret"]') - hotp_secret_field.send_keys(hotp_secret) - - hotp_checkbox = self.driver.find_element_by_css_selector('input[name="is_hotp"]') - hotp_checkbox.click() + self.safe_send_keys_by_css_selector('input[name="username"]', username) + self.safe_send_keys_by_css_selector('input[name="otp_secret"]', hotp_secret) + self.safe_click_by_css_selector('input[name="is_hotp"]') def _journalist_uses_js_filter_by_sources(self): - self.wait_for(lambda: self.driver.find_element_by_id("filter"), timeout=self.timeout * 30) - - filter_box = self.driver.find_element_by_id("filter") - filter_box.send_keys("thiswordisnotinthewordlist") - + filter_box = self.safe_send_keys_by_id("filter", "thiswordisnotinthewordlist") sources = self.driver.find_elements_by_class_name("code-name") assert len(sources) > 0 for source in sources: diff --git a/securedrop/tests/functional/source_navigation_steps.py b/securedrop/tests/functional/source_navigation_steps.py index e921beecb4..ad712138b9 100644 --- a/securedrop/tests/functional/source_navigation_steps.py +++ b/securedrop/tests/functional/source_navigation_steps.py @@ -7,14 +7,10 @@ class SourceNavigationStepsMixin: def _is_on_source_homepage(self): - return self.wait_for( - lambda: self.driver.find_element_by_id("source-index") - ) + return self.wait_for(lambda: self.driver.find_element_by_id("source-index")) def _is_logged_in(self): - return self.wait_for( - lambda: self.driver.find_element_by_id("logout") - ) + return self.wait_for(lambda: self.driver.find_element_by_id("logout")) def _is_on_lookup_page(self): return self.wait_for(lambda: self.driver.find_element_by_id("upload")) @@ -27,7 +23,7 @@ def _source_checks_instance_metadata(self): self.driver.get(self.source_location + "/metadata") j = json.loads(self.driver.find_element_by_tag_name("body").text) assert j["server_os"] == "16.04" - assert j["sd_version"] == self.source_app.jinja_env.globals['version'] + assert j["sd_version"] == self.source_app.jinja_env.globals["version"] assert j["gpg_fpr"] != "" def _source_clicks_submit_documents_on_homepage(self): @@ -102,9 +98,7 @@ def _source_hits_cancel_at_login_page(self): assert self._is_on_source_homepage() def _source_proceeds_to_login(self): - codename_input = self.driver.find_element_by_id("login-with-existing-codename") - codename_input.send_keys(self.source_name) - + self.safe_send_keys_by_id("login-with-existing-codename", self.source_name) self.safe_click_by_id("login") # Check that we've logged in @@ -114,8 +108,9 @@ def _source_proceeds_to_login(self): assert len(replies) == 1 def _source_enters_codename_in_login_form(self): - codename_input = self.driver.find_element_by_id("login-with-existing-codename") - codename_input.send_keys("ascension hypertext concert synopses") + self.safe_send_keys_by_id( + "login-with-existing-codename", "ascension hypertext concert synopses" + ) def _source_hits_cancel_at_submit_page(self): self.driver.find_element_by_id("cancel").click() @@ -153,13 +148,12 @@ def submit_page_loaded(): def _source_submits_a_file(self): with tempfile.NamedTemporaryFile() as file: - file.write(self.secret_message.encode('utf-8')) + file.write(self.secret_message.encode("utf-8")) file.seek(0) filename = file.name - file_upload_box = self.driver.find_element_by_css_selector("[name=fh]") - file_upload_box.send_keys(filename) + self.safe_send_keys_by_css_selector("[name=fh]", filename) submit_button = self.driver.find_element_by_id("submit-doc-button") ActionChains(self.driver).move_to_element(submit_button).perform() @@ -199,8 +193,7 @@ def message_submitted(): time.sleep(self.timeout) def _source_enters_text_in_message_field(self): - text_box = self.driver.find_element_by_css_selector("[name=msg]") - text_box.send_keys(self.secret_message) + self.safe_send_keys_by_css_selector("[name=msg]", self.secret_message) def _source_clicks_submit_button_on_submission_page(self): submit_button = self.driver.find_element_by_id("submit-doc-button") @@ -235,9 +228,7 @@ def reply_deleted(): self.wait_for(reply_deleted) def _source_logs_out(self): - logout = self.driver.find_element_by_id("logout") - logout.send_keys(" ") - logout.click() + self.safe_click_by_id("logout") self.wait_for(lambda: ("Submit for the first time" in self.driver.page_source)) def _source_not_found(self): diff --git a/securedrop/tests/functional/test_journalist.py b/securedrop/tests/functional/test_journalist.py index c4ed615493..450370ef1e 100644 --- a/securedrop/tests/functional/test_journalist.py +++ b/securedrop/tests/functional/test_journalist.py @@ -15,9 +15,6 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . # -import logging - -from selenium.common.exceptions import NoSuchElementException from . import functional_test from . import journalist_navigation_steps @@ -68,20 +65,8 @@ def test_journalist_interface_ui_with_modal(self): self._source_submits_a_file() self._source_logs_out() - tries = 10 - for i in range(tries): - try: - self._journalist_logs_in() - self._journalist_uses_js_filter_by_sources() - break - except NoSuchElementException: - if i < tries: - logging.error( - "Journalist home page JS stymied Selenium again. Retrying login." - ) - else: - raise - + self._journalist_logs_in() + self._journalist_uses_js_filter_by_sources() self._journalist_source_selection_honors_filter() self._journalist_selects_all_sources_then_selects_none() self._journalist_selects_the_first_source()