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..2a17225a160a7b2e1c0291d05264cc2e0885c8a5
--- /dev/null
+++ b/components/test/data/password_manager/environment.py
@@ -0,0 +1,242 @@
+"""The testing Environment class."""
+
+import shutil
+import time
+
+from selenium import webdriver
+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,
+               log_to_screen, log_file):
+    """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.
+      log_to_screen: If set, the tests log is going to be shown on the screen.
+      log_file: The file where to store the log.
+    """
+    # Cleaning the profile folder.
+    try:
+      shutil.rmtree(profile_path)
+    except Exception, e:
+      self.Log(e)

[vabr (Chromium), 2014/05/16 09:36:00]

This deservers a comment on why is it OK to proceed even if rmtree() fails, and
what could be the consequences.
You can actually mention the consequences in the log message.

[rchtara, 2014/05/20 08:24:47]

Done.

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

[vabr (Chromium), 2014/05/16 09:36:00]

nit: Since yesterday, this is no more needed. :)

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

Yes, i have an old version of code, so i need it. :)
I will removed before landing the cl.

+    # 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.websitewindow = None
+    # The Websites list.
+    self.websites = []
+    # An xml tree filled with logins and passwords.
+    self.passwords_tree = None
+    if passwords_path:
+      self.passwords_tree = ElementTree.parse(passwords_path).getroot()
+    # The Websites list for which we expect the test to be working.
+    self.working_tests = []
+    # The number of the save operations done through  all the tests.

[vabr (Chromium), 2014/05/16 09:36:00]

nit: "through  all" contains 2 spaces instead of 1

[rchtara, 2014/05/20 08:24:47]

Done.

+    self.save_count = 0
+
+    self.log_to_screen = log_to_screen
+    self.log_file = None
+    if log_file:
+      self.log_file = open(log_file,'w')
+
+  def AddWebsite(self,
+                 website,
+                 disabled=False):
+    """Adds a Website to the testing Environment.

[vabr (Chromium), 2014/05/16 09:36:00]

Actually, what do you think of renaming Website to WebsiteTest?
To me, WebsiteTest looks like a more precise description of what the class
represents.
If the class Website gets reamed to WebsiteTest, all the variables and method
names related to it should also be renamed accordingly.

[rchtara, 2014/05/20 08:24:47]

Done.

+
+    Args:
+      website: The Website instance that's going to be added to the testing
+          Environment.
+      disabled: Test is disabled.
+    """
+    website.environment = self
+    website.driver = self.driver
+    if self.passwords_tree is not None:

[vabr (Chromium), 2014/05/16 09:36:00]

nit: Just use
if self.passwords_tree:

[rchtara, 2014/05/20 08:24:47]

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.

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

Fair enough. Your test for "is not None" is actually correct here, as being
equal to None means exactly that the tree was not constructed based on the
external XML file, whereas "if self.passwords_tree" tests if the tree is there
and non-empty (and this will break in the future, given the warning). It's fine
to leave the current version, sorry for the noise.

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

No problem

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

[vabr (Chromium), 2014/05/16 09:36:00]

nit: Just use:
if not disabled:

[rchtara, 2014/05/20 08:24:47]

Done.

+      self.working_tests.append(website.name)
+
+  def RemoveAllPasswords(self, websites):
+    """Removes the stored passwords for all Websites.

[vabr (Chromium), 2014/05/16 09:36:00]

Why don't you just flush all the stored passwords, without involving the
websites one after each other, and letting them delete what they hope is their
password entry? The current way looks unnecessarily fragile and complex.

[rchtara, 2014/05/20 08:24:47]

Done.

+
+    Args:
+      websites: The Websites which the passwords are going to be removed.
+    """
+    self.Log("\nRemoveAllPasswords\n")
+    self.driver.get("chrome://settings/passwords")
+    self.driver.switch_to_frame("settings")
+    for website in websites:
+      urls = self.GetURLs()
+      website.RemoveAllPasswords(urls)
+
+  def GetURLs(self):
+    """Gets all URLs of the saved passwords in the chrome://settings/passwords.
+
+    Returns:
+      A list of the URLs.
+    """
+    deletable_passwords = self.driver.find_elements_by_css_selector(
+        "#saved-passwords-list .deletable-item")
+    urls = []
+    for entry in deletable_passwords:
+      urls.append(entry.find_element_by_class_name("url").text)
+    return urls
+
+  def OpenTabAndGoInternal(self):

[vabr (Chromium), 2014/05/16 09:36:00]

nit: Please rename to OpenTabAndGoToInternals
("go internal" sounds confusing)

[rchtara, 2014/05/20 08:24:47]

Done.

+    """Opens a new tab and navigates to passwords internals page in the first

[vabr (Chromium), 2014/05/16 09:36:00]

The comment does not seem to match the code exactly:
If there is a self.websitewindow, nothing happens?
Is that expected? Then the comment should say so.
If that's not expected (i.e., this method should not be called when
self.websitewindow exists), then please make it an error.

[rchtara, 2014/05/20 08:24:47]

Done.

+    tab."""
+    if not self.websitewindow:
+      self.driver.get("https://google.com")

[vabr (Chromium), 2014/05/16 09:36:00]

Would chrome://newtab work instead of google.com?
Every time we depend on an external page loading, we introduce a source of
flakiness. In particular here the flakiness comes in the form of the
time.sleep(1) -- if the network hiccups, 1 second may be too short for
google.com, and the test is screwed.

[rchtara, 2014/05/20 08:24:47]

I updated the test to go to the url of the first website.
I think i was wrong about the new tab.
If I use this script 
var a = document.createElement('a');
a.target = '_blank';
a.href = "chrome://newtab";
a.innerHTML = '.';
document.body.appendChild(a);
a.click()
The pop blocker blocks the tab.
If I remove the last line.
And the ask the driver to perform the click, every thing work fine

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

Great, thanks for fixing this!

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

Thanks :)

+      # 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 avoid that the chrome popup
+      # blocker blocks the new tab, we need to choose a trusted website which
+      # is google.com.
+      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;",
+          "https://google.com")
+
+      a.click()
+      time.sleep(1)

[vabr (Chromium), 2014/05/16 09:36:00]

It's a bit unfortunate that this sleep() does not contribute to the max_duration
decrement of the calling Website here and in CheckPromptIsNotShown.

You could make OpenTabAndGoInternal and CheckPromptIsNotShown return the total
time spent waiting, and let the Website subtract that from its max_duration, and
check the result is still positive.

[rchtara, 2014/05/20 08:24:47]

I don't think it's make sense to do that.max_duration is related to only one
site. And this sleep is made once for all tests.
The duration of the tests is not something very important. It's not even shown.
Some of the tests have an infinite loop in them, for example the following loop:
*click on a menu button
*wait for 1 second
*if the submenu button is shown-> break
*otherwise continue the loop.
max_duration is only created to prevent infinite loop in this cases. 

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

OK, makes sense, I misunderstood the function of max_duration. I suggested a
comment change at the definition remaining_time_to_wait below, to make this
easier to understand.

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

Done.

+
+      self.websitewindow = self.driver.window_handles[-1]
+      self.driver.get(self.internals_page)
+      self.driver.switch_to_window(self.websitewindow)
+
+  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.websitewindow)
+
+  def CheckPromptIsNotShown(self, expected, msg, timeout=10):
+    """Checks if the prompt was shown: If the prompt is shown when it shouldn't

[vabr (Chromium), 2014/05/16 09:36:00]

nit: CheckPromptIsNotShown ... Checks if the prompt was shown
That's a bit confusing: was not shown or was shown?
Also, the function name indicates "not shown" but the argument |expected| allows
to check for both "not shown" and "shown".
So I suggest renaming the method to just CheckPrompt, |expected| to
|prompt_should_show_up|, and start the comment with something like "Detects
whether the save password prompt is shown, and raises an exception if the result
does not match the expectation."

[rchtara, 2014/05/20 08:24:47]

Done.

+    be or is not shown when it should be, raise an exception.
+
+    Args:
+      expected: Whether or not the prompt is expected to be shown.
+      msg: Error message for the exception.
+      timeout: There is some delay between the login and the password
+          internals update. In case nothing is detected, the method is going to
+          wait and execute itself recursively until the time will end.

[vabr (Chromium), 2014/05/16 09:36:00]

This is not clear (and also does not mention the time units again). I suggest
replacing the "In case...will end" sentence with
"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."

In general, I also recommend making the units part of the variable; here it
would mean something like |timeout_sec|.

[rchtara, 2014/05/20 08:24:47]

Done.

+
+    Raises:
+      Exception: An exception is raised in case there a problem.
+    """
+    if expected:
+      self.save_count += 1
+    log = self.driver.find_element_by_css_selector("#log-entries")
+    count = log.text.count("Message: Decision: SAVE the password")

[vabr (Chromium), 2014/05/16 09:36:00]

This is not correct.
The message actually says that the prompt was not shown, instead the password
was saved automatically.

Now, I understand that that's because of the --enable-automatic-password-saving
flag you passed. But this way we won't find out if the prompt would actually be
shown without the flag. For example, if there was a bug and Chrome started to
save everything automatically, the test would no know.

Do you think you could actually split the tests, and do on run with
--enable-automatic-password-saving, and one without? In the run with the flag,
you can ignore whether the prompt is shown. In the run without the flag, you can
only test if the prompt is shown as expected (the message to look for is then
"ASK the user"), and disregard the rest of the tests.

[rchtara, 2014/05/20 08:24:47]

Done.

+
+    if count < self.save_count and timeout > 0:
+      time.sleep(1)
+      self.CheckPromptIsNotShown(False, msg, timeout - 1)
+
+    elif not self.save_count == count:
+      raise Exception(msg)
+
+  def Log(self, text):

[vabr (Chromium), 2014/05/16 09:36:00]

Please use the standard Python logging support instead of this:
https://docs.python.org/2/howto/logging.html

[rchtara, 2014/05/20 08:24:47]

Done.

+    """Writes to the log.
+    Args:
+      text: Text to be written.
+    """
+    if self.log_to_screen:
+      print text
+    if self.log_file:
+      self.log_file.write("%s \n" % text)
+
+  def TestList(self, websites):
+    """Runs the tests on many Websites.

[vabr (Chromium), 2014/05/16 09:36:00]

nit: "many Websites" is unnecessarily vague. Just say "|websites|".

[rchtara, 2014/05/20 08:24:47]

Done.

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

That's not what I meant: you still kept "many". I commented in the new code.

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

sorry :)

+    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.OpenTabAndGoInternal()
+    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):
+    """Closes the tests."""
+    # Close the log file if it's open.
+    if self.log_file:
+      self.log_file.close()
+    # Close the webdriver.
+    self.driver.quit()