[Password manager Python tests] Re-arrange tests

components/test/data/password_manager/automated_tests/environment.py

Index: components/test/data/password_manager/automated_tests/environment.py
diff --git a/components/test/data/password_manager/automated_tests/environment.py b/components/test/data/password_manager/automated_tests/environment.py
index 45cec1cc4a59780c8ae0e429ca9711b62ee329d1..4c4fd14fce01fbdda06acfd918c2ddf508a76a60 100644
--- a/components/test/data/password_manager/automated_tests/environment.py
+++ b/components/test/data/password_manager/automated_tests/environment.py
@@ -2,7 +2,12 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-"""The testing Environment class."""
+"""The testing Environment class.
+
+It holds the WebsiteTest instances, provides them with credentials,
+provides clean browser environment, runs the tests, and gathers the
+results.
+"""
 
 import os
 import shutil
@@ -13,34 +18,18 @@ from selenium import webdriver
 from selenium.webdriver.chrome.options import Options
 
 
-# Message strings to look for in chrome://password-manager-internals
+# Message strings to look for in chrome://password-manager-internals.
 MESSAGE_ASK = "Message: Decision: ASK the user"
 MESSAGE_SAVE = "Message: Decision: SAVE the password"
 
-
-class TestResult:
-  """Stores the information related to a test result. """
-  def __init__(self, name, test_type, successful, message):
-    """Creates a new TestResult.
-
-    Args:
-      name: The tested website name.
-      test_type: The test type.
-      successful: Whether or not the test was successful.
-      message: The error message of the test.
-    """
-    self.name = name
-    self.test_type = test_type
-    self.successful = successful
-    self.message = message
-
+INTERNALS_PAGE_URL = "chrome://password-manager-internals/"
 
 class Environment:
   """Sets up the testing Environment. """
 
   def __init__(self, chrome_path, chromedriver_path, profile_path,
                passwords_path, enable_automatic_password_saving):
-    """Creates a new testing Environment.
+    """Creates a new testing Environment, starts Chromedriver.
 
     Args:
       chrome_path: The chrome binary file.
@@ -58,39 +47,35 @@ class Environment:
     # Cleaning the chrome testing profile folder.
     if os.path.exists(profile_path):
       shutil.rmtree(profile_path)
+
     options = Options()
-    self.enable_automatic_password_saving = enable_automatic_password_saving
     if enable_automatic_password_saving:
       options.add_argument("enable-automatic-password-saving")
-    # Chrome path.
+    # TODO(vabr): show_prompt is used in WebsiteTest for asserting that
+    # Chrome set-up corresponds to the test type. Remove that knowledge
+    # about Environment from the WebsiteTest.
+    self.show_prompt = not enable_automatic_password_saving
     options.binary_location = chrome_path
-    # Chrome testing profile path.
     options.add_argument("user-data-dir=%s" % profile_path)
 
     # The webdriver. It's possible to choose the port the service is going to
     # run on. If it's left to 0, a free port will be found.
     self.driver = webdriver.Chrome(chromedriver_path, 0, options)
-    # The password internals window.
+
+    # Password internals page tab/window handle.
     self.internals_window = self.driver.current_window_handle
-    if passwords_path:
-      # An xml tree filled with logins and passwords.
-      self.passwords_tree = ElementTree.parse(passwords_path).getroot()
-    else:
-      raise Exception("Error: |passwords_path| needs to be provided if"
-          "|chrome_path| is provided, otherwise the tests could not be run")
-    # Password internals page.
-    self.internals_page = "chrome://password-manager-internals/"
-    # The Website window.
-    self.website_window = None
-    # The WebsiteTests list.
+
+    # An xml tree filled with logins and passwords.
+    self.passwords_tree = ElementTree.parse(passwords_path).getroot()

[msramek, 2015/03/27 11:37:06]

This can throw IOError or ParseError. Why don't we handle the former anymore?
Also, please add to the "Raises:" section of the docstring.

[vabr (Chromium), 2015/03/27 12:14:43]

This is intentional. The tests cannot be run, and the original error raised by
ElementTree gives enough context to fix that.
Done.

+
+    self.website_window = self._OpenNewTab()
+
     self.websitetests = []
+
     # Map messages to the number of their appearance in the log.
     self.message_count = { MESSAGE_ASK: 0, MESSAGE_SAVE: 0 }
-    # The tests needs two tabs to work. A new tab is opened with the first
-    # GoTo. This is why we store here whether or not it's the first time to
-    # execute GoTo.
-    self.first_go_to = True
-    # List of all tests results.
+
+    # A list of (test_name, test_type, test_success, failure_log).
     self.tests_results = []
 
   def AddWebsiteTest(self, websitetest):
@@ -110,51 +95,78 @@ class Environment:
     # TODO(vabr): Make driver a property of WebsiteTest.
     websitetest.driver = self.driver
     if not websitetest.username:
-      username_tag = (
-          self.passwords_tree.find(
-              ".//*[@name='%s']/username" % websitetest.name))
+      username_tag = (self.passwords_tree.find(
+          ".//*[@name='%s']/username" % websitetest.name))
       websitetest.username = username_tag.text
     if not websitetest.password:
-      password_tag = (
-          self.passwords_tree.find(
-              ".//*[@name='%s']/password" % websitetest.name))
+      password_tag = (self.passwords_tree.find(
+          ".//*[@name='%s']/password" % websitetest.name))
       websitetest.password = password_tag.text
     self.websitetests.append(websitetest)
 
-  def ClearCache(self, clear_passwords):
-    """Clear the browser cookies. If |clear_passwords| is true, clear all the
-    saved passwords too.
+  def _ClearBrowserDataInit(self):
+    """Opens and resets the chrome://settings/clearBrowserData dialog.
 
-    Args:
-      clear_passwords : Clear all the passwords if the bool value is true.
+    It unchecks all checkboxes, and sets the time range to the "beginning of
+    time".
     """
+
     self.driver.get("chrome://settings/clearBrowserData")
     self.driver.switch_to_frame("settings")
-    script = (
-        "if (!document.querySelector('#delete-cookies-checkbox').checked)"
-        "  document.querySelector('#delete-cookies-checkbox').click();"
-        )
-    negation = ""
-    if clear_passwords:
-      negation = "!"
-    script += (
-        "if (%sdocument.querySelector('#delete-passwords-checkbox').checked)"
-        "  document.querySelector('#delete-passwords-checkbox').click();"
-        % negation)
-    script += "document.querySelector('#clear-browser-data-commit').click();"
-    self.driver.execute_script(script)
+
+    time_range_selector = "#clear-browser-data-time-period"
+    # TODO(vabr): Wait until time_range_selector is displayed instead.
     time.sleep(2)
-    # Every time we do something to the cache let's enable password saving.
+    set_time_range = (
+        "var range = document.querySelector('{0}');".format(
+            time_range_selector) +
+        "range.value = 4"  # 4 == the beginning of time
+        )
+    self.driver.execute_script(set_time_range)
+
+    all_cboxes_selector = (
+        "#clear-data-checkboxes > * > * >[type=\"checkbox\"]")

[msramek, 2015/03/27 11:37:07]

Nit: Why not just

#clear-data-checkboxes [type="checkbox"] ?

I think the semantics are clearer (all checkboxes in this section) as opposed to
unnecessary structural information (there are two <div>s inbetween).

[vabr (Chromium), 2015/03/27 12:14:43]

Agreed and done.
(Learning CSS selectors on the fly, thanks for teaching me!)

+    uncheck_all = (
+        "var checkboxes = document.querySelectorAll('{0}');".format(
+            all_cboxes_selector ) +
+        "for (var i = 0; i < checkboxes.length; ++i) {"
+        "  if (checkboxes[i].checked)"

[msramek, 2015/03/27 11:37:06]

Nit:

checkboxes[i].checked = false

[vabr (Chromium), 2015/03/27 12:14:43]

Done.

+        "    checkboxes[i].click();"
+        "}"
+        )
+    self.driver.execute_script(uncheck_all)
+
+  def _ClearDataForCheckbox(self, selector):
+    """Causes the data associated with |selector| to be cleared.
+
+    Opens chrome://settings/clearBrowserData, unchecks all checkboxes, then
+    checks the one described by |selector|, then clears the corresponding
+    browsing data for the full time range.
+
+    Args:
+      selector: describes the checkbox through which to delete the data.
+    """
+
+    self._ClearBrowserDataInit()
+    check_cookies_and_submit = (
+        "var cbox = document.querySelector('{0}');".format(selector) +
+        "if (!cbox.checked)"

[msramek, 2015/03/27 11:37:07]

Nit:

document.querySelector().checked = true

[vabr (Chromium), 2015/03/27 12:14:43]

Done.

+        "  cbox.click();"
+        "document.querySelector('#clear-browser-data-commit').click();"
+        )
+    self.driver.execute_script(check_cookies_and_submit)
+
+  def _EnablePasswordSaving(self):
+    """Make sure that password manager is enabled."""
+
     # TODO(melandory): We should check why it's off in a first place.
     # TODO(melandory): Investigate, maybe there is no need to enable it that
     # often.
-    self.EnablePasswordsSaving()
-
-  def EnablePasswordsSaving(self):
     self.driver.get("chrome://settings")
     self.driver.switch_to_frame("settings")
     script = "document.getElementById('advanced-settings-expander').click();"
     self.driver.execute_script(script)
+    # TODO(vabr): Wait until element is displayed instead.
     time.sleep(2)
     script = (
         "if (!document.querySelector('#password-manager-enabled').checked)"

[msramek, 2015/03/27 11:37:06]

Nit: As above.

[vabr (Chromium), 2015/03/27 12:14:43]

Done.

@@ -162,196 +174,122 @@ class Environment:
     self.driver.execute_script(script)
     time.sleep(2)
 
-  def OpenTabAndGoToInternals(self, url):
-    """If there is no |self.website_window|, opens a new tab and navigates to
-    |url| in the new tab. Navigates to the passwords internals page in the
-    first tab. Raises an exception otherwise.
-
-    Args:
-      url: Url to go to in the new tab.
+  def _OpenNewTab(self):
+    """Open a new tab, and loads the internals page in the old tab.
 
-    Raises:
-      Exception: An exception is raised if |self.website_window| already
-          exists.
+    Returns:
+      A handle to the new tab.
     """
-    if self.website_window:
-      raise Exception("Error: The window was already opened.")
 
-    self.driver.get("chrome://newtab")
+    number_old_tabs = len(self.driver.window_handles)
     # There is no straightforward way to open a new tab with chromedriver.
     # One work-around is to go to a website, insert a link that is going
-    # to be opened in a new tab, click on it.
+    # to be opened in a new tab, and click on it.
+    self.driver.get("about:blank")
     a = self.driver.execute_script(
         "var a = document.createElement('a');"
         "a.target = '_blank';"
-        "a.href = arguments[0];"
+        "a.href = 'about:blank';"
         "a.innerHTML = '.';"
         "document.body.appendChild(a);"
-        "return a;",
-        url)
-
+        "return a;")
     a.click()
-    time.sleep(1)
+    while number_old_tabs == len(self.driver.window_handles):
+      time.sleep(1)  # Wait until the new tab is opened.
 
-    self.website_window = self.driver.window_handles[-1]
-    self.driver.get(self.internals_page)
-    self.driver.switch_to_window(self.website_window)
+    new_tab = self.driver.window_handles[-1]
+    self.driver.get(INTERNALS_PAGE_URL)
+    self.driver.switch_to_window(new_tab)
+    return new_tab
 
-  def SwitchToInternals(self):
-    """Switches from the Website window to internals tab."""
-    self.driver.switch_to_window(self.internals_window)
+  def _DidStringAppearUntilTimeout(self, strings, timeout):
+    """Checks whether some of |strings| appeared in the current page.
 
-  def SwitchFromInternals(self):
-    """Switches from internals tab to the Website window."""
-    self.driver.switch_to_window(self.website_window)
-
-  def _DidMessageAppearUntilTimeout(self, log_message, timeout):
-    """Checks whether the save password prompt is shown.
+    Waits for up to |timeout| seconds until at least one of |strings| is
+    shown in the current page. Updates self.message_count with the current
+    number of occurrences of the shown string. Assumes that at most
+    one of |strings| is newly shown.
 
     Args:
-      log_message: Log message to look for in the password internals.
-      timeout: There is some delay between the login and the password
-          internals update. The method checks periodically during the first
-          |timeout| seconds if the internals page reports the prompt being
-          shown. If the prompt is not reported shown within the first
-          |timeout| seconds, it is considered not shown at all.
+      strings: A list of strings to look for.
+      timeout: If any such string does not appear within the first |timeout|
+        seconds, it is considered a no-show.
 
     Returns:
-      True if the save password prompt is shown.
-      False otherwise.
+      True if one of |strings| is observed until |timeout|, False otherwise.
     """
-    log = self.driver.find_element_by_css_selector("#log-entries")
-    count = log.text.count(log_message)
 
-    if count > self.message_count[log_message]:
-      self.message_count[log_message] = count
-      return True
-    elif timeout > 0:
+    log = self.driver.find_element_by_css_selector("#log-entries")
+    while timeout:

[msramek, 2015/03/27 11:37:06]

Why don't we just sleep for |timeout| and then count the strings? Do they
disappear over time?

[vabr (Chromium), 2015/03/27 12:14:43]

No, but in the case when they appear, they mostly appear after the first second
or so, so waiting for the whole timeout is wasteful. This delay is particularly
inconvenient when doing interactive testing (i.e., with a person observing the
test, as opposed to the stuff run by cron).

+      for string in strings:
+        count = log.text.count(string)
+        if count > self.message_count[string]:
+          self.message_count[string] = count
+          return True
       time.sleep(1)
-      return self._DidMessageAppearUntilTimeout(log_message, timeout - 1)
-    else:
-      return False
+      timeout -= 1
+    return False
 
-  def CheckForNewMessage(self, log_message, message_should_show_up,
-                         error_message, timeout=15):
-    """Detects whether the save password prompt is shown.
+  def CheckForNewString(self, strings, string_should_show_up, error):
+    """Checks that |strings| show up on the internals page as it should.
 
-    Args:
-      log_message: Log message to look for in the password internals. The
-          only valid values are the constants MESSAGE_* defined at the
-          beginning of this file.
-      message_should_show_up: Whether or not the message is expected to be
-          shown.
-      error_message: Error message for the exception.
-      timeout: There is some delay between the login and the password
-          internals update. The method checks periodically during the first
-          |timeout| seconds if the internals page reports the prompt being
-          shown. If the prompt is not reported shown within the first
-          |timeout| seconds, it is considered not shown at all.
-
-    Raises:
-      Exception: An exception is raised in case the result does not match the
-          expectation
-    """
-    if (self._DidMessageAppearUntilTimeout(log_message, timeout) !=
-        message_should_show_up):
-      raise Exception(error_message)
-
-  def AllTests(self, prompt_test):
-    """Runs the tests on all the WebsiteTests.
-
-    TODO(vabr): Currently, "all tests" always means one.
+    Switches to the internals page and looks for a new instances of |strings|
+    being shown up there. It checks that |string_should_show_up| is true if
+    and only if at leas one string from |strings| shows up, and throws an
+    Exception if that check fails.
 
     Args:
-      prompt_test: If True, tests caring about showing the save-password
-          prompt are going to be run, otherwise tests which don't care about
-          the prompt are going to be run.
+      strings: A list of strings to look for in the internals page.
+      string_should_show_up: Whether or not at least one string from |strings|
+          is expected to be shown.
+      error: Error message for the exception.
 
     Raises:
-      Exception: An exception is raised if the tests fail.
+      Exception: (See above.)
     """
-    if prompt_test:
-      self.PromptTestList(self.websitetests)
-    else:
-      self.TestList(self.websitetests)
 
-  def Test(self, tests, prompt_test):
-    """Runs the tests on websites named in |tests|.
+    self.driver.switch_to_window(self.internals_window)
+    try:
+      if (self._DidStringAppearUntilTimeout(strings, 15) !=
+          string_should_show_up):
+        raise Exception(error)
+    finally:
+      self.driver.switch_to_window(self.website_window)
 
-    Args:
-      tests: A list of the names of the WebsiteTests that are going to be
-          tested.
-      prompt_test: If True, tests caring about showing the save-password
-          prompt are going to be run, otherwise tests which don't care about
-          the prompt are going to be executed.
+  def DeleteCookies(self):
+    """Deletes cookies via the settings page."""
 
-    Raises:
-      Exception: An exception is raised if the tests fail.
-    """
-    websitetests = []
-    for websitetest in self.websitetests:
-      if websitetest.name in tests:
-        websitetests.append(websitetest)
+    self._ClearDataForCheckbox("#delete-cookies-checkbox")
 
-    if prompt_test:
-      self.PromptTestList(websitetests)
-    else:
-      self.TestList(websitetests)
+  def RunTestsOnSites(self, test_type):
+    """Runs the specified test on the known websites.
 
-  def TestList(self, websitetests):
-    """Runs the tests on the websites in |websitetests|.
+    Also saves the test results in the environment. Note that test types
+    differ in their requirements on whether the save password prompt
+    should be displayed. Make sure that such requirements are consistent
+    with the enable_automatic_password_saving argument passed to |self|
+    on construction.
 
     Args:
-      websitetests: A list of WebsiteTests that are going to be tested.
-
-    Raises:
-      Exception: An exception is raised if the tests fail.
+      test_type: A test identifier understood by WebsiteTest.run_test().
     """
-    self.ClearCache(True)
-
-    for websitetest in websitetests:
-      successful = True
-      error = ""
-      try:
-        websitetest.was_run = True
-        websitetest.WrongLoginTest()
-        websitetest.SuccessfulLoginTest()
-        self.ClearCache(False)
-        websitetest.SuccessfulLoginWithAutofilledPasswordTest()
-        self.ClearCache(True)
-        websitetest.SuccessfulLoginTest()
-        self.ClearCache(True)
-      except Exception as e:
-        successful = False
-        error = e.message
-      self.tests_results.append(TestResult(websitetest.name, "normal",
-                                           successful, error))
 
+    self.DeleteCookies()
+    self._ClearDataForCheckbox("#delete-passwords-checkbox")
+    self._EnablePasswordSaving()
 
-  def PromptTestList(self, websitetests):
-    """Runs the prompt tests on the websites in |websitetests|.
-
-    Args:
-      websitetests: A list of WebsiteTests that are going to be tested.
-
-    Raises:
-      Exception: An exception is raised if the tests fail.
-    """
-    self.ClearCache(True)
-
-    for websitetest in websitetests:
+    for websitetest in self.websitetests:
       successful = True
       error = ""
       try:
-        websitetest.was_run = True
-        websitetest.PromptTest()
+        websitetest.RunTest(test_type)
       except Exception as e:
         successful = False
         error = e.message
-      self.tests_results.append(TestResult(websitetest.name, "prompt",
-                                           successful, error))
+      self.tests_results.append(
+          (websitetest.name, test_type, successful, error))
 
   def Quit(self):
-    """Closes the tests."""
-    # Close the webdriver.
+    """Shuts down the driver."""
+
     self.driver.quit()