Password Manager testing automation

components/test/data/password_manager/environment.py

Index: components/test/data/password_manager/environment.py
diff --git a/components/test/data/password_manager/environment.py b/components/test/data/password_manager/environment.py
new file mode 100644
index 0000000000000000000000000000000000000000..e7024bc7050a6422e71fc7811e41480d1c5199c0
--- /dev/null
+++ b/components/test/data/password_manager/environment.py
@@ -0,0 +1,179 @@
+"""The testing Environment class."""
+
+
+import time
+from selenium import webdriver
+from selenium.webdriver.chrome.options import Options
+import xml.etree.ElementTree as ElementTree
+
+
+class Environment:
+  """Handles the testing Environment. """

[vabr (Chromium), 2014/05/14 15:16:09]

I don't understand what is meant by "Handles" here.
Maybe "Sets up"?

[rchtara, 2014/05/15 09:39:11]

Done.

+
+  def __init__(self, chrome_path, chromedriver_path,
+               profile_path, passwords_path):
+    """Creates a new testing Environment.
+
+    Args:
+      chrome_path: The chrome binary file.
+      chromedriver_path: The chromedriver binary file.
+      profile_path: The testing profile folder.
+      passwords_path: The usernames and passwords file.
+    """
+    options = Options()
+    options.add_argument("enable-automatic-password-saving")
+    # Chrome path.
+    options.binary_location = chrome_path
+    # Testing profile path.
+    options.add_argument("user-data-dir=%s" % profile_path)
+    print "user-data-dir=%s" % profile_path

[vabr (Chromium), 2014/05/14 15:16:09]

Please do not leave debugging statements in the code to be committed.
Also on other places in the scripts.

[rchtara, 2014/05/15 09:39:11]

Done.

+    # The webdriver.
+    self.driver = webdriver.Chrome(chromedriver_path, 0, options)

[vabr (Chromium), 2014/05/14 15:16:09]

optional nit: would be nice to explain the "0" passed to webdriver. It's not
necessary, but could be helpful later.

[rchtara, 2014/05/15 09:39:11]

Done.

+    # The password internals window.
+    self.internals_window = self.driver.current_window_handle
+    # The Website window.
+    self.websitewindow = None
+    # The Websites list.
+    self.websites = []
+    # An xml tree filled with logins and passwords.
+    self.passwords_tree = None
+    if passwords_path != None:
+      self.passwords_tree = ElementTree.parse(passwords_path).getroot()
+    # The Websites list for which we expect the test to be working.
+    self.working_tests = []
+
+  def AddWebsite(self,
+                 website,
+                 failing=False):
+    """Adds a Website to the testing Environment.
+
+    Args:
+      name: The name of the website.

[vabr (Chromium), 2014/05/14 15:16:09]

There is no 'name' among the arguments. Did you mean 'website'?
Also note that it is not clear what a "name of a website" is. Is that URL? Or
just some user-defined identifier to use during the tests?

[rchtara, 2014/05/15 09:39:11]

Done.

+      username_not_auto: Don't check that the state of the DOM is equal to the

[vabr (Chromium), 2014/05/14 15:16:09]

This is also not in the arguments list.

[rchtara, 2014/05/15 09:39:11]

Done.

+      expected value for the username.
+      failing: Test is expected to fail.

[vabr (Chromium), 2014/05/14 15:16:09]

Does this rather mean that the test should not be run, because it fails?
In that case, I propose renaming 'failing' to 'disabled'. The concept of
disabled tests as test currently failing and not being run is widely known
within Chromium. Instead, 'failing', would imply that the test should be run and
should fail, and if it runs and does not fail, then something is wrong. I don't
think that's the case, but please correct me if I'm wrong.

[rchtara, 2014/05/15 09:39:11]

Done.

+
+    Returns:

[vabr (Chromium), 2014/05/14 15:16:09]

The function does not appear to return anything.

[rchtara, 2014/05/15 09:39:11]

Done.

+      A new new Website.

[vabr (Chromium), 2014/05/14 15:16:09]

typo: new new

[rchtara, 2014/05/15 09:39:11]

Done.

+    """
+    website.test_environment = self
+    website.driver = self.driver
+    if self.passwords_tree != None:

[vabr (Chromium), 2014/05/14 15:16:09]

I believe you can leave out '!= None', as 'None' translates to 'false' in
conditions.
Same also below, including 'X == None' replaced with 'not X'.

[rchtara, 2014/05/15 09:39:11]

I got this warning when I tied to replace this None.
/usr/local/google/home/rchtara/chrome/src/components/test/data/password_manager/environment.py:63:
FutureWarning: The behavior of this method will change in future versions.  Use
specific 'len(elem)' or 'elem is not None' test instead.
  if self.passwords_tree:
For all the others, I got nothing.
So I replaced all the other and I kept only this one

+      if website.username == None:
+        username_tag = (
+            self.passwords_tree.find(
+                ".//*[@name='%s']/username" % website.name))
+        if (username_tag != None):
+          website.username = username_tag.text
+      if website.password == None:
+        password_tag = (
+            self.passwords_tree.find(
+                ".//*[@name='%s']/password" % website.name))
+        if (password_tag != None):
+          website.password = password_tag.text
+
+    self.websites.append(website)
+    if failing == False:

[vabr (Chromium), 2014/05/14 15:16:09]

'failing == False' -> 'not failing'

[rchtara, 2014/05/15 09:39:11]

Done.

+      self.working_tests.append(website.name)
+
+  def RemoveAllPasswords(self, websites):
+    """Removes the stored passwords for all Websites.
+
+    Args:
+      websites: The Websites which the passwords are going to be removed.
+    """
+    print "\nRemoveAllPasswords\n"
+    self.driver.get("chrome://settings/passwords")
+    self.driver.switch_to_frame("settings")
+
+    for website in websites:
+      urls = self.GetURLs()

[vabr (Chromium), 2014/05/14 15:16:09]

I think you can pull
urls = self.GetURLs()
before the for cycle, because it does not depend on 'website'.

[rchtara, 2014/05/15 09:39:11]

The urls list is changing every time.

+      website.RemoveAllPasswords(urls)
+
+  def GetURLs(self):
+    """Gets all URLs of the saved passwords in the chrome://settings/passwords.
+
+    Returns:
+      A list of the URLs.
+    """
+    arr = self.driver.find_elements_by_css_selector(

[vabr (Chromium), 2014/05/14 15:16:09]

What does 'arr' mean? Could you please use something more descriptive? For
example: deletable_passwords?

[rchtara, 2014/05/15 09:39:11]

Done.

+        "#saved-passwords-list .deletable-item")
+    urls = []
+    for entry in arr:
+      urls.append(entry.find_element_by_class_name("url").text)
+    return urls
+
+  def NewWindow(self):
+    """Opens a new tab and switch to it."""
+    if self.websitewindow == None:
+      self.driver.get("https://google.com")
+      # There is no straightforward way to open a new tab with chromedriver.
+      # One of work-around is to go to a website, insert a link that is going

[vabr (Chromium), 2014/05/14 15:16:09]

nit: One of -> One

[rchtara, 2014/05/15 09:39:11]

Done.

+      # to be opened in a new tab, click on it. To avoid that the chrome popup
+      # blocker blocks the new tab, we need to choose a trusted website which
+      # is google.com.
+      # Links for local resources (chrome://*) are blocked. So this why the

[vabr (Chromium), 2014/05/14 15:16:09]

nit: this -> this is

[rchtara, 2014/05/15 09:39:11]

Done.

+      # new tab is going to be for google.com and then the browser is asked
+      # via chromedriver to go to chrome://settings/passwords .

[vabr (Chromium), 2014/05/14 15:16:09]

Seems out of date: this function does not open chrome://settings/passwords.
You can still keep the explanation for why the new tab starts with google.com,
though.

[rchtara, 2014/05/15 09:39:11]

Done.

+      a = self.driver.execute_script(
+          "var d=document;var "

[vabr (Chromium), 2014/05/14 15:16:09]

Please make the line wrapping be one line per statement, if possible. And please
also use spaces around binary operators, to make reading the JavaScript easier.

[rchtara, 2014/05/15 09:39:11]

Done.

+          "a=d.createElement('a');a.target='_blank';a.href=arguments[0];"
+          "a.innerHTML='.';d.body.appendChild(a);return"
+          " a;",
+          "https://google.com")
+
+      a.click()
+      time.sleep(1)
+
+      self.websitewindow = self.driver.window_handles[-1]
+
+      self.driver.switch_to_window(self.websitewindow)
+
+  def TestList(self, websites):
+    """Runs the tests on many Websites.
+    Args:
+      websites: A list of Websites that are going to be tested.
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.RemoveAllPasswords(websites)
+    self.NewWindow()
+    for website in websites:
+      website.WrongLoginTest()
+      website.SuccessfulLoginTest()
+      website.SuccessfulLoginWithAutofilledPasswordTest()
+
+    self.RemoveAllPasswords(websites)
+    for website in websites:
+      website.SuccessfulLoginAfterDeletionTest()
+
+  def AllTests(self):
+    """Runs the tests on all the websites.
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.TestList(self.websites)
+
+  def WorkingTests(self):
+    """Runs the tests on all the working websites.
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.Test(self.working_tests)
+
+  def Test(self, tests):
+    """Runs the tests on many websites.
+    Args:
+      tests: A list of the names of the websites that are going to be tested.
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    websites = []
+    for website in self.websites:
+      if website.name in tests:
+        websites.append(website)
+    self.TestList(websites)
+
+  def Quit(self):
+    """Close the chromedriver."""
+    self.driver.quit()