Password Manager testing automation

components/test/data/password_manager/environment.py

+"""The testing Environment class."""
+
+import logging
+import shutil
+import time
+
+from selenium import webdriver
+from selenium.common.exceptions import NoSuchElementException
+from selenium.common.exceptions import WebDriverException
+from selenium.webdriver.chrome.options import Options
+from xml.etree import ElementTree
+
+
+class Environment:
+  """Sets up the testing Environment. """
+
+  def __init__(self, chrome_path, chromedriver_path, profile_path,
+               passwords_path, enable_automatic_password_saving,
+               numeric_level, log_screen, log_file):

[vabr (Chromium), 2014/05/20 14:47:25]

nit: log_screen -> log_to_console

[vabr (Chromium), 2014/05/20 14:47:25]

Please provide default values for numeric_level, log_screen and log_file
arguments (None, False, and "", respectively). Logging is not the core function
of Environment, so whoever uses this API should not be forced to deduce from the
code which are the default values.

[rchtara, 2014/05/22 08:44:38]

Done.

[rchtara, 2014/05/22 08:44:38]

Done.

+    """Creates a new testing Environment.
+
+    Args:
+      chrome_path: The chrome binary file.y

[vabr (Chromium), 2014/05/20 14:47:25]

typo: "y" at the end of the line

[rchtara, 2014/05/22 08:44:38]

Done.

+      chromedriver_path: The chromedriver binary file.
+      profile_path: The testing profile folder.

[vabr (Chromium), 2014/05/20 14:47:25]

nit: "profile" -> "Chrome profile", just for clarity

[rchtara, 2014/05/22 08:44:38]

Done.

+      passwords_path: The usernames and passwords file.
+      enable_automatic_password_saving: If set, the passwords are going to be
+          saved without showing the prompt.
+      numeric_level: The log numeric level.

[vabr (Chromium), 2014/05/20 14:47:25]

nit: "log" -> "log verbosity"

[rchtara, 2014/05/22 08:44:38]

Done.

+      log_screen: If set, the tests log is going to be shown on the screen.

[vabr (Chromium), 2014/05/20 14:47:25]

nit: change "screen" to console also in the comment

[vabr (Chromium), 2014/05/20 14:47:25]

nit: "tests log is going to be" -> "debug logs will be"

[vabr (Chromium), 2014/05/20 14:47:25]

nit: "set" -> "true"

[rchtara, 2014/05/22 08:44:38]

Done.

[rchtara, 2014/05/22 08:44:38]

Done.

[rchtara, 2014/05/22 08:44:38]

Done.

+      log_file: The file where to store the log.

[vabr (Chromium), 2014/05/20 14:47:25]

nit: Comment on what happens if it is empty.

[rchtara, 2014/05/22 08:44:38]

Done.

+
+    Raises:
+      Exception: An exception is raised if |profile_path| folder could not be
+      removed.
+    """
+    # Setting up the login.
+    if numeric_level is not None:
+      if log_file:
+        # Set up logging to file.
+        logging.basicConfig(level=numeric_level,
+                            filename=log_file,
+                            filemode='w')
+
+        if log_screen:
+          console = logging.StreamHandler()
+          console.setLevel(numeric_level)
+          # Add the handler to the root logger.
+          logging.getLogger('').addHandler(console)
+
+      elif log_screen:
+        logging.basicConfig(level=numeric_level)
+
+    # Cleaning the profile folder.
+    try:
+      shutil.rmtree(profile_path)
+    except Exception, e:
+      # The tests execution can continue, but this make them less stable.
+      logging.error("""Error: %s.\n The tests execution is continuing. But

[vabr (Chromium), 2014/05/20 14:47:25]

I don't think you use """ correctly here. If you only need to wrap a string
literal over multiple lines, use single quotes and \ at the end of the open
lines:
"This spans \
two lines"
Note that the line break will not be part of the resulting string, i.e., the
above string is equal to
"This spans two lines".
If you use """, you don't have to write \, but the line break will be part of
the string:
"""A
B"""
is equal to "A\nB".

In particular, on this line, I suggest it would be better not to specify line
breaks (you don't know how wide the log console will be anyway). Therefore it
seems better to use single quotes, and also omit '\n'.

[rchtara, 2014/05/22 08:44:38]

Done.

+               the tests are going to be less stable.""" % e)

[vabr (Chromium), 2014/05/20 14:47:25]

Please be more specific: write what is the error, and what are the implications.
For example:
"Error: could not wipe profile directory (%s). This affects the stability of the
tests. Continuing to run tests."

[rchtara, 2014/05/22 08:44:38]

Done.

+    options = Options()
+    if enable_automatic_password_saving:
+      options.add_argument("enable-automatic-password-saving")
+    options.add_argument("enable-password-manager-internals-ui")

[vabr (Chromium), 2014/05/20 14:47:25]

You probably missed my comment earlier: enable-password-manager-internals-ui is
no more needed.

[rchtara, 2014/05/22 08:44:38]

I didn't update my chromium yet, so I will keep using the
enable-automatic-password-saving until and remove it at the end.

+    # Chrome path.
+    options.binary_location = chrome_path
+    # 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.
+    self.internals_window = self.driver.current_window_handle
+    # Password internals page.
+    self.internals_page = "chrome://password-manager-internals/"
+    # The Website window.
+    self.website_window = None
+    # The WebsiteTests list.
+    self.websitetests = []
+    # An xml tree filled with logins and passwords.
+    self.passwords_tree = None
+    if passwords_path:
+      self.passwords_tree = ElementTree.parse(passwords_path).getroot()
+    # The enabled WebsiteTests list.
+    self.working_tests = []
+    # The number of the save operations done through all the tests.
+    self.save_count = 0

[vabr (Chromium), 2014/05/20 14:47:25]

Note: I see you use this to deal with the internals page containing all the logs
so far.
That's fine, but soon it will be able to flush all the logs by refreshing the
page. If that would make your code easier, please let me know, and I'll
prioritise fixing that.

[rchtara, 2014/05/22 08:44:38]

No, it's going make it harder, because I will need to refresh every time i do
that.
I will need to know the right time to refresh too.
So actually it's even harder for me to do that. 

+    # Show whether or not it's the first time to execute GoTo.

[vabr (Chromium), 2014/05/20 14:47:25]

"Show"? It's not clear how and to whom this is shown.
Did you mean "store"? Also, could you briefly explain in the comment, what this
information is used for?

[rchtara, 2014/05/22 08:44:38]

Done.

+    self.first_go_to = True
+
+  def AddWebsiteTest(self, websitetest, disabled=False):
+    """Adds a WebsiteTest to the testing Environment.
+
+    Args:
+      websitetest: The WebsiteTest instance that's going to be added to the

[vabr (Chromium), 2014/05/20 14:47:25]

nit: You can simply write:
"The WebsiteTest instance to be added."
Concise comments are quicker and easier to read.

[rchtara, 2014/05/22 08:44:38]

Done.

+          testing Environment.
+      disabled: Test is disabled.

[vabr (Chromium), 2014/05/20 14:47:25]

nit: Whether test is disabled.

[rchtara, 2014/05/22 08:44:38]

Done.

+    """
+    websitetest.environment = self
+    websitetest.driver = self.driver
+    if self.passwords_tree is not None:
+      if not websitetest.username:
+        username_tag = (
+            self.passwords_tree.find(
+                ".//*[@name='%s']/username" % websitetest.name))
+        if username_tag.text:
+          websitetest.username = username_tag.text
+      if not websitetest.password:
+        password_tag = (
+            self.passwords_tree.find(
+                ".//*[@name='%s']/password" % websitetest.name))
+        if password_tag.text:
+          websitetest.password = password_tag.text
+    self.websitetests.append(websitetest)
+    if not disabled:
+      self.working_tests.append(websitetest.name)
+
+  def RemoveAllPasswords(self):
+    """Removes all the stored passwords."""
+    logging.info("\nRemoveAllPasswords\n")
+    self.driver.get("chrome://settings/passwords")
+    self.driver.switch_to_frame("settings")
+    while True:
+      try:
+        self.driver.execute_script("document.querySelector('"
+          "#saved-passwords-list .row-delete-button').click()")
+        time.sleep(1)
+      except NoSuchElementException:
+        break
+      except WebDriverException:
+        break
+
+  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.

[vabr (Chromium), 2014/05/20 14:47:25]

It's not clear what "otherwise" refers to.
I believe leaving the last sentence completely is fine, as you describe the
exception in the "Raises:" section, and repeating only clutters the comments.

[rchtara, 2014/05/22 08:44:38]

Done.

+
+    Args:
+      url: Url to go to in the new tab.
+
+    Raises:
+      Exception: An exception is raised if |self.website_window| already exists.
+    """
+    if not self.website_window:

[vabr (Chromium), 2014/05/20 14:47:25]

nit: Consider starting with
if self.website_window:
  raise (...)

Then you can add the rest of the code without being nested in an if-branch.

[rchtara, 2014/05/22 08:44:38]

Done.

+      self.driver.get("chrome://newtab")
+      # 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.
+      a = self.driver.execute_script(
+          "var a = document.createElement('a');"
+          "a.target = '_blank';"
+          "a.href = arguments[0];"
+          "a.innerHTML = '.';"
+          "document.body.appendChild(a);"
+          "return a;",
+          url)
+
+      a.click()
+      time.sleep(1)
+
+      self.website_window = self.driver.window_handles[-1]
+      self.driver.get(self.internals_page)
+      self.driver.switch_to_window(self.website_window)
+    else:
+      raise Exception("Error: The window was already opened.")
+
+  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 CheckPrompt(self, log_message, prompt_should_show_up,
+                  error_message, timeout=10):
+    """Detects whether the save password prompt is shown, and raises an
+    exception if the result does not match the expectation.
+
+    Args:
+      log_message: Log message to look for in the password internals.
+      prompt_should_show_up: Whether or not the prompt is expected to be shown.
+      error_message: Error message for the exception  .

[vabr (Chromium), 2014/05/20 14:47:25]

nit: Remove the two spaces at the end of the sentence.

[rchtara, 2014/05/22 08:44:38]

Done.

+      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 there is a problem.

[vabr (Chromium), 2014/05/20 14:47:25]

nit: "there is a problem" -> "the result does not match the expectation"
(The latter provides much better information, and is not much longer. Saying
just that "exception is raised if there's a problem" does not help much, as it
is true almost always. :))

[rchtara, 2014/05/22 08:44:38]

Done.

+    """
+    if prompt_should_show_up:
+      self.save_count += 1
+    log = self.driver.find_element_by_css_selector("#log-entries")
+    count = log.text.count(log_message)
+
+    if count < self.save_count and timeout > 0:
+      time.sleep(1)
+      self.CheckPrompt(log_message, False, error_message, timeout - 1)

[vabr (Chromium), 2014/05/20 14:47:25]

You hard-code False as the "prompt_should_show_up" argument here. I can see why
you do it (so that save_count does not get increased again), but it's rather
confusing.

Also, the code seems wrong for the case when prompt_should_show_up == False
since beginning:
In that case count == self.save_count, and so CheckPrompt immediately returns
with success, instead of waiting 10 seconds, to see whether the prompt is not
shown after all.

My suggestion:
Define a private helper method, which would wait until either time is out, or
the prompt appears, and will return false in the first case, and true in the
latter. Then CheckPrompt simply calls that helper method to see whether the
prompt was shown, compares that to the expectation, and raises an exception
unless they match.

[rchtara, 2014/05/22 08:44:38]

Done.

+
+    elif not self.save_count == count:
+      raise Exception(error_message)
+
+  def AllTests(self, prompt_test):
+    """Runs the tests on all the  WebsiteTests.

[vabr (Chromium), 2014/05/20 14:47:25]

typo: two consequent spaces between "the" and "WebsiteTests".

[rchtara, 2014/05/22 08:44:38]

Done.

+
+    Args:
+      prompt_test: Prompt tests or normal tests.

[vabr (Chromium), 2014/05/20 14:47:25]

Please explain what are "prompt tests". The most plausible English meaning would
be "fast tests", which is not what you mean here. I'm guessing you mean tests
caring about the save-password prompt, and tests which don't; but that's not
obvious (and still not clear what exactly means |prompt_test|, and what are the
values).

Same below.

[rchtara, 2014/05/22 08:44:38]

Done.

+
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    if prompt_test:
+      self.PromptTestList(self.websitetests)
+    else:
+      self.TestList(self.websitetests)
+
+  def WorkingTests(self, prompt_test):
+    """Runs the tests on all the enabled WebsiteTests.
+
+    Args:
+      prompt_test: Prompt tests or normal tests.
+
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.Test(self.working_tests, prompt_test)
+
+  def Test(self, tests, prompt_test):
+    """Runs the tests on many WebsiteTests.

[vabr (Chromium), 2014/05/20 14:47:25]

many WebsiteTests -> websites named in |tests|

Also below at TestList() and PromptTestList().

[rchtara, 2014/05/22 08:44:38]

Done.

+
+    Args:
+      tests: A list of the names of the WebsiteTests that are going to be
+          tested.
+      prompt_test: Prompt tests or normal tests.
+
+    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 many WebsiteTests.
+
+    Args:
+      websitetests: A list of WebsiteTests that are going to be tested.
+
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.RemoveAllPasswords()
+
+    for websitetest in websitetests:
+      websitetest.WrongLoginTest()
+      websitetest.SuccessfulLoginTest()
+      websitetest.SuccessfulLoginWithAutofilledPasswordTest()
+
+    self.RemoveAllPasswords()
+    for websitetest in websitetests:
+      websitetest.SuccessfulLoginTest()
+
+  def PromptTestList(self, websitetests):
+    """Runs the prompt tests on many WebsiteTests.
+
+    Args:
+      websitetests: A list of WebsiteTests that are going to be tested.
+
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.RemoveAllPasswords()
+
+    for websitetest in websitetests:
+      websitetest.PromptTest()
+
+  def Quit(self):
+    """Closes the tests."""
+    # Close the webdriver.
+    self.driver.quit()