[Password manager Python tests] Re-arrange tests

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

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # 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
 import time
 from xml.etree import ElementTree
 
 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.
       chromedriver_path: The chromedriver binary file.
       profile_path: The chrome testing profile folder.
       passwords_path: The usernames and passwords file.
       enable_automatic_password_saving: If True, the passwords are going to be
           saved without showing the prompt.
 
     Raises:
       Exception: An exception is raised if |profile_path| folder could not be
       removed.
     """
 
     # 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):
     """Adds a WebsiteTest to the testing Environment.
 
     TODO(vabr): Currently, this is only called at most once for each
     Environment instance. That is because to run all tests efficiently in
     parallel, each test gets its own process spawned (outside of Python).
     That makes sense, but then we should flatten the hierarchy of calls
     and consider making the 1:1 relation of environment to tests more
     explicit.
 
     Args:
       websitetest: The WebsiteTest instance to be added.
     """
     websitetest.environment = self
     # 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.
+
+    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")
+
+    time_range_selector = "#clear-browser-data-time-period"
+    # TODO(vabr): Wait until time_range_selector is displayed instead.
+    time.sleep(2)
+    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:
-      clear_passwords : Clear all the passwords if the bool value is true.
+      selector: describes the checkbox through which to delete the data.
     """
-    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();"
+
+    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();"
         )
-    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.sleep(2)
-    # Every time we do something to the cache let's enable password saving.
+    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.

         "{ document.querySelector('#password-manager-enabled').click();}")
     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.
+  def _OpenNewTab(self):
+    """Open a new tab, and loads the internals page in the old tab.
 
-    Args:
-      url: Url to go to in the new tab.
+    Returns:
+      A handle to the new tab.
+    """
 
-    Raises:
-      Exception: An exception is raised if |self.website_window| already
-          exists.
-    """
-    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()
+    while number_old_tabs == len(self.driver.window_handles):
+      time.sleep(1)  # Wait until the new tab is opened.
 
-    a.click()
-    time.sleep(1)
+    new_tab = self.driver.window_handles[-1]
+    self.driver.get(INTERNALS_PAGE_URL)
+    self.driver.switch_to_window(new_tab)
+    return new_tab
 
-    self.website_window = self.driver.window_handles[-1]
-    self.driver.get(self.internals_page)
-    self.driver.switch_to_window(self.website_window)
+  def _DidStringAppearUntilTimeout(self, strings, timeout):
+    """Checks whether some of |strings| appeared in the current page.
 
-  def SwitchToInternals(self):
-    """Switches from the Website window to internals tab."""
-    self.driver.switch_to_window(self.internals_window)
-
-  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)
+    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)
+      timeout -= 1
+    return False
 
-    if count > self.message_count[log_message]:
-      self.message_count[log_message] = count
-      return True
-    elif timeout > 0:
-      time.sleep(1)
-      return self._DidMessageAppearUntilTimeout(log_message, timeout - 1)
-    else:
-      return False
+  def CheckForNewString(self, strings, string_should_show_up, error):
+    """Checks that |strings| show up on the internals page as it should.
 
-  def CheckForNewMessage(self, log_message, message_should_show_up,
-                         error_message, timeout=15):
-    """Detects whether the save password prompt is shown.
+    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:
-      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.
+      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 in case the result does not match the
-          expectation
+      Exception: (See above.)
     """
-    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.
+    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)
 
-    TODO(vabr): Currently, "all tests" always means one.
+  def DeleteCookies(self):
+    """Deletes cookies via the settings page."""
+
+    self._ClearDataForCheckbox("#delete-cookies-checkbox")
+
+  def RunTestsOnSites(self, test_type):
+    """Runs the specified test on the known websites.
+
+    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:
-      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.
+      test_type: A test identifier understood by WebsiteTest.run_test().
+    """
 
-    Raises:
-      Exception: An exception is raised if the tests fail.
-    """
-    if prompt_test:
-      self.PromptTestList(self.websitetests)
-    else:
-      self.TestList(self.websitetests)
+    self.DeleteCookies()
+    self._ClearDataForCheckbox("#delete-passwords-checkbox")
+    self._EnablePasswordSaving()
 
-  def Test(self, tests, prompt_test):
-    """Runs the tests on websites named in |tests|.
-
-    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.
-
-    Raises:
-      Exception: An exception is raised if the tests fail.
-    """
-    websitetests = []
     for websitetest in self.websitetests:
-      if websitetest.name in tests:
-        websitetests.append(websitetest)
-
-    if prompt_test:
-      self.PromptTestList(websitetests)
-    else:
-      self.TestList(websitetests)
-
-  def TestList(self, websitetests):
-    """Runs the 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:
       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)
+        websitetest.RunTest(test_type)
       except Exception as e:
         successful = False
         error = e.message
-      self.tests_results.append(TestResult(websitetest.name, "normal",
-                                           successful, error))
-
-
-  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:
-      successful = True
-      error = ""
-      try:
-        websitetest.was_run = True
-        websitetest.PromptTest()
-      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()