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.

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."
         )

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: