Test Autofill's ability to merge duplicate profiles and...

chrome/test/functional/autofill.py

Index: chrome/test/functional/autofill.py
===================================================================
--- chrome/test/functional/autofill.py	(revision 74353)
+++ chrome/test/functional/autofill.py	(working copy)
@@ -4,7 +4,9 @@
 # found in the LICENSE file.
 
 import os
+import pickle
 
+import dataset_converter
 import pyauto_functional  # Must be imported before pyauto
 import pyauto
 
@@ -93,9 +95,61 @@
     self.assertEqual([expected_credit_card],
                      self.GetAutoFillProfile()['credit_cards'])
 
-  def testAutofillCrowdSourcing(self):
-    """Test able to send POST request of web form to crowd source server.
-    Require a loop of 1000 submits as the source server only collects 1% of
+  def testFilterIncompleteAddresses(self):
+    """Test Autofill filters out profiles with incomplete address info
+    during form submission and aggregation steps. Regression test for
+    bug 71710."""

[dennisjeffrey, 2011/02/11 00:53:17]

The first line of this comment should be a 1-line summary of the method.

[dyu1, 2011/02/16 03:17:31]

Done.

On 2011/02/11 00:53:17, dennisjeffrey wrote:

+    profiles = [{'NAME_FIRST': 'Bob',
+                 'NAME_LAST': 'Smith',
+                 'EMAIL_ADDRESS': 'bsmith@example.com',
+                 'COMPANY_NAME': 'Company X',
+                 'PHONE_HOME_WHOLE_NUMBER': '650-123-4567',}]
+    url = self.GetHttpURLForDataPath(
+        os.path.join('autofill', 'dup-profiles-test.html'))
+    for p in profiles:

[dennisjeffrey, 2011/02/11 00:53:17]

This test case seems to use just a single profile, so I think there's no need to
loop through each profile here (unless you think that more profiles might be
added to this test case later).

[dyu1, 2011/02/16 03:17:31]

Done. Changed the array to a dict.

On 2011/02/11 00:53:17, dennisjeffrey wrote:

+      self.NavigateToURL(url)
+      for key, value in p.iteritems():
+        script = 'document.getElementById("%s").value = "%s"; ' \
+                 'window.domAutomationController.send("done")' % (key, value)

[dennisjeffrey, 2011/02/11 00:53:17]

Python will automatically concatenate strings on multiple lines when enclosed in
parens.  I think that's preferred by the python style guide:

script = ('document.getElementById("%s").value = "%s"; '
          'window.domAutomationController.send("done")') % (key, value)

[dyu1, 2011/02/16 03:17:31]

Done.

+        self.ExecuteJavascript(script, 0, 0)
+      self.ExecuteJavascript('document.getElementById("merge_dup").submit();'
+                             'window.addEventListener("unload", function(){'
+                             'window.domAutomationController.send("done");});',
+                             0, 0)

[dennisjeffrey, 2011/02/11 00:53:17]

I think it might look neater to put the code into a separate variable, something
like this:

code = """
  document.getElementById("merge_dup").submit();
  window.addEventListener("unload", function() {
    window.domAutomationController.send("done");
  });
"""

self.ExecuteJavascript(code, 0, 0)

[dyu1, 2011/02/16 03:17:31]

Done.

+    self.assertEqual([], self.GetAutoFillProfile()['profiles'])
+
+  def testFilterMalformedEmailAddresses(self):
+    """Test Autofill filters out malformed email addresses when form is
+    submitted. Regression test for bug 73654."""

[dennisjeffrey, 2011/02/11 00:53:17]

The first line of this comment should be a 1-line summary of the method.

[dyu1, 2011/02/16 03:17:31]

Done.

+    profiles = [{'NAME_FIRST': 'Bob',
+                 'NAME_LAST': 'Smith',
+                 'EMAIL_ADDRESS': 'garbage',
+                 'ADDRESS_HOME_LINE1': '1234 H St.',
+                 'ADDRESS_HOME_CITY': 'San Jose',
+                 'ADDRESS_HOME_STATE': 'CA',
+                 'ADDRESS_HOME_ZIP': '95110',
+                 'COMPANY_NAME': 'Company X',
+                 'PHONE_HOME_WHOLE_NUMBER': '408-123-4567',}]
+    url = self.GetHttpURLForDataPath(
+        os.path.join('autofill', 'dup-profiles-test.html'))
+    for p in profiles:

[dennisjeffrey, 2011/02/11 00:53:17]

It also looks like there's only 1 profile here, so maybe we don't need a loop.

[dyu1, 2011/02/16 03:17:31]

Done.

+      self.NavigateToURL(url)
+      for key, value in p.iteritems():
+        script = 'document.getElementById("%s").value = "%s"; ' \
+                 'window.domAutomationController.send("done")' % (key, value)

[dennisjeffrey, 2011/02/11 00:53:17]

Same comment as line 113 above.

[dyu1, 2011/02/16 03:17:31]

Done.

+        self.ExecuteJavascript(script, 0, 0)
+      self.ExecuteJavascript('document.getElementById("merge_dup").submit();'
+                             'window.addEventListener("unload", function(){'
+                             'window.domAutomationController.send("done");});',
+                             0, 0)

[dennisjeffrey, 2011/02/11 00:53:17]

Same comment as line 118 above.

[dyu1, 2011/02/16 03:17:31]

Done.

+    if 'EMAIL_ADDRESS' in self.GetAutoFillProfile()['profiles'][0]:
+      raise KeyError('TEST FAIL: Malformed email address is saved in profiles.')
+    else:
+      print 'TEST PASS: Malformed email address is not saved in profiles.'

[dennisjeffrey, 2011/02/11 00:53:17]

I think we can remove lines 147-148.  Probably no need for a print statement if
the test passes.

[dyu1, 2011/02/16 03:17:31]

Done.

+
+  def testAutofillCrowdsourcing(self):
+    """Test able to send POST request of web form to Autofill server.
+    Require a loop of 1000 submits as the Autofill server only collects 1% of

[dennisjeffrey, 2011/02/11 00:53:17]

The first line of this comment should be a 1-line summary of the method.

[dyu1, 2011/02/16 03:17:31]

Done.

     the data posted."""
     # HTML file needs to be run from a specific http:// url to be able to verify
     # the results a few days later by visiting the same url.
@@ -124,6 +178,34 @@
                              'window.domAutomationController.send("done")',
                              0, 0)
 
+  def testMergeDuplicateProfilesInAutofill(self):
+    """Test Autofill ability to merge duplicate profiles and throw away junk
+    profiles. The goal is to submit a couple hundred profiles and see the merge
+    outcome."""

[dennisjeffrey, 2011/02/11 00:53:17]

The first line of this comment should be a 1-line summary of the method.

[dyu1, 2011/02/16 03:17:31]

Done.

+    # HTML file needs to be run from a http:// url.
+    url = self.GetHttpURLForDataPath(
+        os.path.join('autofill', 'duplicate_profiles_test.html'))
+    # Run the parser script to generate the dictionary list needed for the
+    # profiles.
+    c = dataset_converter.DatasetConverter(os.path.join(
+        self.DataDir(), 'autofill', 'dataset.txt'))

[dennisjeffrey, 2011/02/11 00:53:17]

Move the "os.path.join(" down to the next line:

c = dataset_converter.DatasetConverter(
    os.path.join(self.DataDir(), 'autofill', 'dataset.txt'))

[dyu1, 2011/02/16 03:17:31]

Done.

+    list_of_dict = c.Convert()
 
+    for profile in list_of_dict:
+      self.NavigateToURL(url)
+      for key, value in profile.iteritems():
+        script = 'document.getElementById("%s").value = "%s"; ' \
+                 'window.domAutomationController.send("done")' % (key, value)

[dennisjeffrey, 2011/02/11 00:53:17]

Same comment as line 113 above.

[dyu1, 2011/02/16 03:17:31]

Done.

+        self.ExecuteJavascript(script, 0, 0)
+      self.ExecuteJavascript('document.getElementById("merge_dup").submit();'
+                             'window.domAutomationController.send("done")',
+                             0, 0)
+    # Write profile dictionary to a file.
+    profile_dict = self.GetAutoFillProfile()['profiles']
+    output = open('merged-profile.txt', 'wb')
+    pickle.dump(profile_dict, output)
+    output.close()

[dennisjeffrey, 2011/02/11 00:53:17]

Shouldn't the test do some kind of verification here so that the test will fail
if something has gone wrong?  Or is the verification just the fact that the
above lines of code execute without raising any exceptions?

[dyu1, 2011/02/16 03:17:31]

I added a test to compare the number of input profiles originally to the final
number of profiles after the merge. Just a general test to make sure the input
number of profiles is greater than the merged profiles. In general this test
doesn't run on the continuous build. It will require a manual check of the dump
file, basically the output file should have less than the 243 profiles from the
input file. Since the dataset could change, I'm not sure what could be
generically verified.

On 2011/02/11 00:53:17, dennisjeffrey wrote:

[dennis_jeffrey, 2011/02/16 19:43:29]

If the only thing we need to verify is that the total number of profiles after
the merge is less than the original number of profiles, then you already added
this check, so there's maybe no longer any need to write things to a file to be
manually inspected.

If we want a stronger check, to verify that for a given X number of initial
profiles, this leads to a resulting Y number of final profiles (with Y < X),
then we can perhaps bake this into the input data file.  In other words, the
input data file could not only have all the input profiles, but it could also
specify the final number of resulting profiles expected after merging.  The test
can then verify this.

If we want to do something even more complex, like check the contents of the
profiles rather than just the number of profiles, then we might need an entirely
new module to do this kind of verification in general (may not be worth the
effort?).

+
+
 if __name__ == '__main__':
   pyauto_functional.Main()