Password Manager testing automation

components/test/data/password_manager/website.py

+"""Website testing class."""
+
+
+import time
+
+from selenium.common.exceptions import ElementNotVisibleException
+from selenium.common.exceptions import NoSuchElementException
+from selenium.common.exceptions import StaleElementReferenceException
+from selenium.webdriver.common.action_chains import ActionChains
+from selenium.webdriver.common.keys import Keys
+
+
+def _IsOneSubstringOfAnother(s1, s2):

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

Please add a doc string, explaining what this function does.

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

Done.

+  return s1 in s2 or s2 in s1
+
+
+class Website:
+  """Handles a tested Website."""
+
+  class Mode:
+    """Test mode."""
+    # Password and username are expected to be autofilled.
+    Autofilled = 1

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

Please use all caps for consistency with the C++ enum naming:
AUTOFILLED
NOT_AUTOFILLED

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

Done.

+    # Password and username are not expected to be autofilled.
+    NotAutofilled = 2
+
+    def __init__(self):
+      pass
+
+  def __init__(
+      self, name, url, username=None, password=None,

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

Please remove |url|. You don't seem to use it, and it's always the same URL
which is passed as the GoTo() argument in the login steps.

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

Are you actually using the username and password arguments? If not, then please
remove them.

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

Yes, I use them to sign in

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

Done.

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

But you don't seem to use the __init__ arguments username and password, as
opposed to self.username, and self.password, which get assigned to in the
environment.py code.
I'll add a comment at the most recent patch set.

+      username_not_auto=False):
+    """Creates a new Website.
+
+    Args:
+      name: The Website name.
+      url:  The Website URL.
+      username: The Website username. If it's None, the username is going to be
+          replaced by the value in the usernames and passwords file.
+      password: The Website password. If it's None, the password is going to be
+          replaced by the value in the usernames and passwords file.
+      username_not_auto: Username inputs in some Websites (like wikipedia) are
+          sometimes filled with some messages and thus, the usernames are not
+          automatically autofilled. This flag handles that and disables us from
+          checking if the state of the DOM is the same as the username of
+          Website.
+    """
+    # Name of the Website
+    self.name = name
+    # URL of the Website
+    self.url = url
+    # Username of the Website.
+    self.username = username
+    # Password of the Website.
+    self.password = password
+    # Username is not automatically filled.
+    self.username_not_auto = username_not_auto

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

For pages like Wikipedia, where the username is not autofilled, because the page
fills in some trash, we still could do better in our test:

Ideally, we would clear the username input, and overwrite it with the correct
username. After that Chrome should autofill the password, and we should check
that it did so correctly.

Currently we give up, and fill the password ourselves, which leaves out
important things to check.

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

username_not_auto affects only the username, but the password is treated exactly
  
the same as for the normal website

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

You are correct, thanks for explanation.

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

you re welcome :)

+    # Autofilling mode.
+    self.mode = self.Mode.NotAutofilled
+    # Waiting duration before stopping the test.

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

Note: max_duration limits the total time spent in waiting, but does not include
the time spent running. You might want to make this clear in the comment.

Also, since max_duration gets decremented, a more appropriate name would be:
remaining_time_to_wait or something similar.

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

Done.

+    self.max_duration = 200
+    # The testing Environment.
+    self.environment = None
+    # The webdriver.
+    self.driver = None
+
+  # Mouse/Keyboard actions.
+
+  def Click(self, selector):
+    """Clicks on an element.
+
+    Args:
+      selector: The element CSS selector.
+    """
+    self.environment.Log("action: Click %s" % selector)
+    element = self.driver.find_element_by_css_selector(selector)
+    element.click()
+
+  def ClickIfVisible(self, selector):
+    """Clicks on an element, if it's available.

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

So, is this about visibility, or availability?
If the element is there, but has 'display: none' in its CSS, will it be clicked?

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

Done.

+
+    Args:
+      selector: The element CSS selector.
+    """
+    self.environment.Log("action: ClickIfVisible %s" % selector)
+    try:
+      element = self.driver.find_element_by_css_selector(selector)
+      element.click()
+    except NoSuchElementException:
+      return False

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

The method returns False if the element is not clicked, but does not return
anything when it is clicked.

Please unify this. It looks like you don't use the return value, so don't return
False. Use just "return" or "pass".

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

Done.

+    except StaleElementReferenceException:
+      return False
+
+  def GoTo(self, url):
+    """Navigates the main frame to a url.

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

nit: "a url" -> "|url|"
(It does not navigate to just some url, it navigates to the url specified in
|url|.)

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

Done.

+
+    Args:
+      url: The URL.
+    """
+    self.environment.Log("action: GoTo %s" % self.url)
+    self.driver.get(url)
+
+  def HoverOver(self, selector):
+    """Hovers over an element.
+
+    Args:
+      selector: The element CSS selector.
+    """
+    self.environment.Log("action: Hover %s" % selector)
+    element = self.driver.find_element_by_css_selector(selector)
+    hover = ActionChains(self.driver).move_to_element(element)
+    hover.perform()
+
+  def SendEnterTo(self, selector):
+    """Sends an enter key to an element.
+
+    Args:
+      selector: The element CSS selector.
+    """
+    self.environment.Log("action: SendEnterTo %s" % selector)
+    body = self.driver.find_element_by_tag_name("body")
+    body.send_keys(Keys.ENTER)
+
+  # Waiting/Displaying actions.
+
+  def IsDisplayed(self, selector):
+    """Checks if an element is displayed.
+
+    Args:
+      selector: The element CSS selector.
+    """
+    self.environment.Log("action: IsDisplayed %s" % selector)
+    try:
+      element = self.driver.find_element_by_css_selector(selector)
+      return element.is_displayed()
+    except NoSuchElementException:
+      return False
+    except StaleElementReferenceException:
+      return False
+
+  def Wait(self, duration):
+    """Wait for a duration.

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

Please specify time units.

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

Done.

+
+    Args:
+      duration: The element.
+    """
+    self.environment.Log("action: Wait %s" % duration)
+    time.sleep(duration)
+    self.max_duration -= 1
+    if self.max_duration < 0:
+      raise Exception("Tests took more time than expected for the following "
+                      "website : %s \n" % self.name)
+
+  def WaitUntilDisplayed(self, selector, timeout=10):
+    """Waits until an element is displayed.
+
+    Args:
+      selector: The element CSS selector.
+    """
+    if not self.IsDisplayed(selector):
+      time.sleep(1)

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

Please call Wait() instead of manually decrementing and checking max_duration
below.

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

Done.

+      timeout = timeout - 1
+      if (timeout <= 0):
+        raise Exception("Error: Element %s not shown before timeout is "
+                        "finished for the following website: %s"
+                        % (selector, self.name))
+      else:
+        self.WaitUntilDisplayed(selector, timeout)
+        self.max_duration -= 1
+        if self.max_duration < 0:
+          raise Exception("Tests took more time than expected for the "
+                          "following website : %s \n" % self.name)
+
+  # Form actions.
+
+  def FillPasswordInto(self, selector):
+    """If the testing mode is the Autofilled mode, compares the Website
+    password to the DOM state.
+    If the testing mode is the NotAutofilled mode, checks that the DOM state
+    is empty.
+    Then, fills the input with the Website password.
+
+    Args:
+      selector: The password input CSS selector.
+
+    Raises:
+      Exception: An exception is raised if the DOM value of the password is
+      different that the one we expected.
+    """
+    self.environment.Log("action: FillPasswordInto %s" % selector)
+    password_element = self.driver.find_element_by_css_selector(selector)
+    if self.mode == self.Mode.Autofilled:
+      # Chrome protects the password inputs and doesn't fill them until
+      # the user interacts with the page. To guarantee that, we just
+      # send a key to the password input. Clicking on the password input was
+      # tried too, but because the password is sometimes hidden, this didn't

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

I'm not sure I understand the issue here -- if the password input is hidden,
then how were the user expected to see it? This looks like the test should fail
in that case.

Also, to unblock the password, user interaction with the page is needed, not
necessarily with the password form. Interacting with the root of the DOM tree
should work as well.

It is crucial that we use the autofilled password for login, without retyping
the password again, as that would mask bugs we need to watch out for.

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

In www.163.com, we the driver fills the username with send_keys, the list of all
the similar users is shown. And the password input disappears behind the list
even if the full email address is completely filled.
So in this case if we click on the password, we are going to get an error
because the password is invisible.
The username field is almost always visible, so i m going to make sure to click
on it in case we didn't fill it and check the result

+      # worked out.

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

nit: worked -> work

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

Done.

+      password_element.send_keys("a")
+      ps = password_element.get_attribute("value")[:-1]

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

Pleas do not use cryptic abbreviations as names of variables.
This looks like the autofilled password, so |autofilled_password| might be an
appropriate name.

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

Done.

+      password_element.clear()
+      password_element.send_keys(ps)
+      if password_element.get_attribute("value") != self.password:
+        raise Exception("Error: autofilled password is different from the one "
+                        "we just saved for the following website : %s p1: %s "
+                        "p2:%s \n" % (self.name,
+                                       password_element.get_attribute("value"),

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

nit: indenting is off

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

Done.

+                                       self.password))
+    elif self.mode == self.Mode.NotAutofilled:
+      # Chrome protects the password inputs and doesn't fill them until
+      # the user interacts with the page. To guarantee that, we just
+      # send a key to the password input. Clicking on the password input was
+      # tried too, but because the password is sometimes hidden, this didn't
+      # worked out.
+      password_element.send_keys("a")
+      ps = password_element.get_attribute("value")[1:]

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

Again, please rename |ps| appropriately.

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

Done.

+      password_element.clear()
+      password_element.send_keys(ps)

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

Why do you send |ps| to password_element here, and why do you re-send "a" to
password_element below?

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

I' m going to replace all this by a click on the user input, when the driver
didn't fill the username. (if we already filled the username, we don't need an
interaction)
And see if it work

+      if ps:
+        raise Exception("Error: password is autofilled when it shouldn't  be "
+                        "for the following website : %s \n"
+                        % self.name)
+
+      # Chrome protects the password inputs and doesn't fill them until
+      # the user interacts with the page. To guarantee that, we just
+      # send a key to the password. Clicking on the password input was tried
+      # too, but because the password is sometime hidden, this didn't worked
+      # out.
+      password_element.send_keys("a")
+      password_element.clear()
+      password_element.send_keys(self.password)
+
+  def FillUsernameInto(self, selector):

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

In the concrete Websites, FillUsernameInto and FillPasswordInto is always used
as a sequence:
FillUsernameInto(...)
FillPasswordInto(...)

To make the code clearer, these two public methods should be replaced by one
FillUsernameAndPasswordInto(), which can then call two private (name starting
with "_") methods for filling the username and the password.

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

I think it's more flexible to keep it as it's now.
It's going to work to work with the current websites, but with other ones that
could be added, we could have same issues.

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

Fair enough, let's keep them separate.

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

Done.

+    """If the testing mode is the Autofilled mode, compares the Website username
+    to the input value.
+    Then, fills the input with the Website username.
+
+    Args:
+      selector: The username input CSS selector.
+
+    Raises:
+      Exception: An exception is raised if the DOM value of the username is
+      different that the one we expected.
+    """
+    self.environment.Log("action: FillUsernameInto %s" % selector)
+    username_element = self.driver.find_element_by_css_selector(selector)
+
+    if (self.mode == self.Mode.Autofilled and not self.username_not_auto):
+      if not (username_element.get_attribute("value") == self.username):
+        raise Exception("Error: autofilled username is different form the one "
+                        "we just saved for the following website : %s \n" %
+                        self.name)
+
+    else:
+      username_element.clear()

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

Should you check that either self.username_not_auto is true, or username_element
is empty, before you clear it?

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

I don't think it's important it important to do so.
The autofill manager is going to fill the user, but not the password.

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

Ah right, that's a good point, I forgot about the non-password autofill. :)
Thanks for your explanation.

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

You re welcome

+      username_element.send_keys(self.username)
+
+  def FillUsernameIfVisible(self, selector):
+    """Fills the input with the website username, if the input exists.

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

Please comment on ignoring self.mode here.

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

I removed the method, not needed

+
+    Args:
+      selector: The username input CSS selector.
+    """
+    self.environment.Log("action: FillUsernameIfVisible %s" % selector)
+    username_element = self.driver.find_element_by_css_selector(selector)
+    try:
+      username_element.clear()
+      username_element.send_keys(self.username)
+    except ElementNotVisibleException:
+      pass
+
+  def Submit(self, selector):
+    """Finds an element using CSS Selector and calls its submit() handler.
+
+    Args:
+      selector: The input CSS selector.
+    """
+    self.environment.Log("action: Submit %s" % selector)
+    element = self.driver.find_element_by_css_selector(selector)
+    element.submit()
+
+  # Login/Logout Methods
+
+  def Login(self):
+    """Login Method. Has to be overloaded by the Website test."""
+    raise NotImplementedError("Login is not implemented.")
+
+  def LoginWhenAutofilled(self):
+    """Logs in and checks that the password is autofilled."""
+    self.mode = self.Mode.Autofilled
+    self.Login()
+
+  def LoginWhenNotAutofilled(self):
+    """Logs in and checks that the password is not autofilled."""
+    self.mode = self.Mode.NotAutofilled
+    self.Login()
+
+  def Logout(self):
+    """Logout Method. Has to be overloaded by the Website test."""
+    raise NotImplementedError("Logout is not implemented.")
+
+  # TestsTools
+
+  def RemoveAllPasswords(self, urls):

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

Function removed

+    """Removes all the saved passwords for the current Website.
+
+    Args:
+      urls: All the available URLs in the saved passwords list.
+    """
+    if (self.url != ""):

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

nit: Just use
if self.url:

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

Done.

+      i = 0
+      for current_url in urls:
+        if _IsOneSubstringOfAnother(current_url, self.url):
+          self.driver.execute_script(

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

Please comment on what the script does.
In particular mention that it deletes the password entry from the list, and that
therefore we don't increase |i| in this case.

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

Done.

+              "document.querySelectorAll('#saved-passwords-list "
+              ".row-delete-button')[%d].click()" % i)

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

Actually, I don't think you use |i| correctly here.
Imagine that another Website just removed its passwords. Then the passwords the
other Website deleted are still included in |urls|, so you do increment |i| for
them, although they are no longer in the list. That shifts your index in a wrong
state, doesn't it?

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

we get a fresh copy of |urls|, after each deletion, this is why we don't have
that problem. 

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

Ah, that makes sense, thanks for your explanation.
Anyway, I like that you now delete all passwords in one pass. :)

+          time.sleep(1)  # Wait until command is executed.

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

Please use Wait().

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

Done.

+        else:
+          i = i + 1
+
+
+  # Tests
+
+  def WrongLoginTest(self):
+    """Does the wrong login test: Tries to login with a wrong password and
+    checks that the prompt is not shown.
+
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.environment.Log("\nWrong Login Test for %s \n" % self.name)
+    correct_password = self.password
+    self.password = self.password + "1"
+    self.LoginWhenNotAutofilled()
+    self.password = correct_password

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

Why do you not Wait() here, but you do wait at the same place in
SuccessfullLoginTest below?

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

Done.

+    self.environment.SwitchToInternals()
+    self.environment.CheckPromptIsNotShown(
+        False,
+        "Error: password manager thinks that a login with wrong password was "
+        "successful for the following website : %s \n" % self.name)
+    self.environment.SwitchFromInternals()
+
+  def SuccessfulLoginTest(self):
+    """Does the successful login when the password is not expected to be
+    autofilled test: Checks that the password is not autofilled, tries to login
+    with a right password and checks if the that the prompt is shown. Then logs
+    out.
+
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.environment.Log("\nSuccessful Login Test for %s \n" % self.name)
+    self.LoginWhenNotAutofilled()
+    time.sleep(2)

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

Please use Wait().

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

Done.

+    self.environment.SwitchToInternals()
+    self.environment.CheckPromptIsNotShown(True,
+        "Error: password manager hasn't detected a successful login for the "
+        "following website : %s \n"
+        % self.name)
+    self.environment.SwitchFromInternals()
+    self.Logout()
+
+  def SuccessfulLoginWithAutofilledPasswordTest(self):
+    """Does the successful login when the password is expected to be autofilled
+    test: Checks that the password is autofilled, tries to login with a right
+    password and checks if the that the prompt is shown. Then logs out.
+
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.environment.Log("\nSuccessful Login With Autofilled Password"
+                        " Test %s \n" % self.name)
+    self.LoginWhenAutofilled()
+    time.sleep(2)

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

Please use Wait().

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

Done.

+    self.environment.SwitchToInternals()
+    self.environment.CheckPromptIsNotShown(True,
+        "Error: password manager hasn't detected a successful login for the "
+        "following website : %s \n"
+        % self.name)
+    self.environment.SwitchFromInternals()
+    self.Logout()
+
+  def SuccessfulLoginAfterDeletionTest(self):

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

This does exactly the same thing as SuccessfulLoginTest (except it does not
Wait(), which looks like a mistake). Please remove this method and use
SuccessfulLoginTest instead.

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

Done.

+    """Does the successful login after the deletion of the password test: Checks
+    that the password is not autofilled, tries to login with a right password
+    and checks if the that the prompt is shown. Then logs out.
+
+    Raises:
+      Exception: An exception is raised if the tests fail.
+    """
+    self.environment.Log("\nSuccessful Login After Deletion Test"
+                         " for %s \n" % self.name)
+    self.LoginWhenNotAutofilled()
+    self.environment.SwitchToInternals()
+    self.environment.CheckPromptIsNotShown(
+        True,
+        "Error: password manager hasn't detected a successful login for the "
+        "following website : %s \n" % self.name)
+    self.environment.SwitchFromInternals()
+    self.Logout()