Two sets of Autofill tests....

chrome/test/functional/autofill_dataset_generator.py

Index: chrome/test/functional/autofill_dataset_generator.py
===================================================================
--- chrome/test/functional/autofill_dataset_generator.py	(revision 0)
+++ chrome/test/functional/autofill_dataset_generator.py	(revision 0)
@@ -0,0 +1,300 @@
+#!/usr/bin/python
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Generates profile dictionaries for Autofill.
+
+Used to test autofill.AutoFillTest.FormFillLatencyAfterSubmit.
+Can be used as a stand alone script with -h to print out help text by running:
+  python autofill_dataset_generator.py -h
+"""
+
+import codecs
+import logging
+import random
+import re
+import sys
+import os

[dennis_jeffrey, 2011/03/18 21:44:01]

"import os" should come between "logging" and "random".

[dyu1, 2011/03/21 18:42:35]

Done.

+
+
+class NullHandler(logging.Handler):
+  def emit(self, record):
+    pass
+
+
+class DatasetGenerator(object):
+  """Generates a dataset of dictionaries.
+
+  The lists (such as address_construct, city_construct) define the way the
+  corresponding field is generated. They accomplish this by specifying a
+  list of function-args lists.
+  """
+  address_construct = [
+    [ random.randint, 1, 10000],

[Ilya Sherman, 2011/03/18 23:17:11]

Do we really need randomness?  Deterministic tests are generally preferable, as
the results are more reproducible.

[dyu1, 2011/03/21 18:42:35]

This creates what you recommended...

{u'NAME_FIRST': u'1', u'EMAIL_ADDRESS': u'1@example.com', ....}
{u'NAME_FIRST': u'2', u'EMAIL_ADDRESS': u'2@example.com', ....}

[Ilya Sherman, 2011/03/22 01:18:58]

So, what's the purpose of the random.randint then?

[dyu1, 2011/03/22 02:52:35]

Purpose of [random.randint, 1, 10000] is to generate street address numbers.
Without this the address will be "foobar Ln #3" or "foobar St #1", etc. With the
random.randint the addresses will have digits associated with it. I thought that
would be more realistic. Down the road when Autofill begins performing
verification checks for street addresses, the ones without a street address
could be rejected.

[Ilya Sherman, 2011/03/22 04:08:57]

We have no plans to validate user-entered addresses (i.e. ones added via the
prefs).  That said, I'm not really opposed to having nicely formatted addresses;
but we should do it without randomness.  If you're really attached to using
python's 'random' class, please use 'random.seed' at the beginning of the test,
so that the results are consistently reproducible.

+    [ None, u'foobar'],
+    [ random.choice, [ u'St', u'Ave', u'Ln', u'Ct', ]],
+    [ random.choice, [ u'#1', u'#2', u'#3', ]],
+  ]
+
+  city_construct = [
+    [ random.choice, [ u'San Jose', u'San Francisco', u'Sacramento',
+                      u'Los Angeles', ]],
+  ]
+
+  state_construct = [
+    [ None, u'CA']
+  ]
+
+  zip_construct = [
+    [ None, u'95110'],
+  ]

[dennis_jeffrey, 2011/03/18 21:44:01]

Should we have more choices for cities, states, and zip codes to allow for more
varied random outputs?

[dyu1, 2011/03/21 18:42:35]

I don't think it makes a difference since the minimum requirements for a valid
Autofill profile has been met. All the cities listed are in CA anyways. I might
add corresponding zip codes to the associated city.

[Ilya Sherman, 2011/03/22 01:18:58]

In fact, it's fine to have the whole profile just be e.g. {u'NAME_FIRST': u'1'}
for this particular test -- with no additional address data.  The minimum
requirements for a profile only apply when that profile is being learned
implicitly via form submission.

+
+  re_single_quote = re.compile("'", re.UNICODE)

[dennis_jeffrey, 2011/03/18 21:44:01]

Prefer single quotes for strings:

'\''

[Ilya Sherman, 2011/03/18 23:17:11]

Curious: What's the motivation for this?

[dennis_jeffrey, 2011/03/18 23:25:39]

Consistency; most strings I've seen in python test code are specified with
single quotes, and I've seen other code reviewers try to keep it that way too. 
I'm curious to hear and consider different opinions in case anyone has them :-)

[Ilya Sherman, 2011/03/19 00:41:52]

In general, I too prefer single quotes for consistency; but in this case I'd
personally have chosen double quotes to avoid escaping the inner quote.  Haven't
worked with our python codebase very much though -- thus the question :)

[dyu1, 2011/03/21 18:42:35]

Done.

+  logger = logging.getLogger(__name__)
+  logger.addHandler(NullHandler())
+  log_handlers = {'StreamHandler': None}
+
+  def __init__(self, output_filename=None, logging_level=None):
+    """Constructs dataset generator object.
+
+    Creates 'fields' data member which is a list of pair (two values) lists.
+    These pairs are comprised of a field key e.g. u'NAME_FIRST' and a
+    generator method e.g. self.GenerateNameFirst which will generate the value.
+    If we want the value to always be the same .e.g. u'John' we can use this

[dennis_jeffrey, 2011/03/18 21:44:01]

".e.g." --> "e.g."

[dyu1, 2011/03/21 18:42:35]

Done.

+    instead a a method. We can even use None keyword which will give

[dennis_jeffrey, 2011/03/18 21:44:01]

"a a method" --> "of a method"

[dyu1, 2011/03/21 18:42:35]

Done.

+    a value of u''.
+
+    'output_pattern' for one field would have been: "{u'NAME_FIRST': u'%s',}"

[Ilya Sherman, 2011/03/18 23:17:11]

I don't understand why there is both a trailing comma and a trailing curly brace
in this example.  Might be worth clarifying that in the comment.

[dyu1, 2011/03/21 18:42:35]

I thought the trailing ,} is required for the last elements in a dictionary? At
least that's what I see for all the dictionaries we have in our data directory.

[Ilya Sherman, 2011/03/22 01:18:58]

Both the comma and the whitespace are optional.  I would recommend trimming
either both or neither (probably neither, since that's simpler).

+    which is ready to accept a value for the 'NAME_FIRST' field key once
+    this value is generated.
+    'output_pattern' is used in 'GenerateNextDict()' to generate the next
+    dict line.
+
+    Args:
+      output_filename: specified filename of generated dataset to be saved.
+        Default value is None and no saving takes place.
+      logging_level: set verbosity levels, default is None.
+    """
+    if logging_level:
+      if not self.log_handlers['StreamHandler']:
+        console = logging.StreamHandler()
+        console.setLevel(logging.INFO)
+        self.log_handlers['StreamHandler'] = console
+        self.logger.addHandler(console)
+      self.logger.setLevel(logging_level)
+    else:
+      if self.log_handlers['StreamHandler']:
+        self.logger.removeHandler(self._log_handlers['StreamHandler'])
+        self.log_handlers['StreamHandler'] = None
+
+    self.output_filename = output_filename
+
+    self.dict_no = 0
+    self.fields = [
+      [u'NAME_FIRST', self.GenerateNameFirst],
+      [u'NAME_MIDDLE', None],
+      [u'NAME_LAST', None],
+      [u'EMAIL_ADDRESS', self.GenerateEmail],
+      [u'COMPANY_NAME', None],
+      [u'ADDRESS_HOME_LINE1', self.GenerateAddress],
+      [u'ADDRESS_HOME_LINE2', None],
+      [u'ADDRESS_HOME_CITY', self.GenerateCity],
+      [u'ADDRESS_HOME_STATE', self.GenerateState],
+      [u'ADDRESS_HOME_ZIP', self.GenerateZip],
+      [u'ADDRESS_HOME_COUNTRY', u'United States'],
+      [u'PHONE_HOME_WHOLE_NUMBER', None],
+      [u'PHONE_FAX_WHOLE_NUMBER', u'6501234555'],
+    ]
+
+    self.dict_length = len(self.fields)
+    self.output_pattern = u"{"

[dennis_jeffrey, 2011/03/18 21:44:01]

Prefer single quotes over double quotes in strings.

[dyu1, 2011/03/21 18:42:35]

Done.

+    for key_and_method in self.fields:
+      self.output_pattern += u"u'%s': u'%s', " %(key_and_method[0], "%s")

[dennis_jeffrey, 2011/03/18 21:44:01]

Prefer single quotes over double quotes in strings.

[dyu1, 2011/03/21 18:42:35]

Done.

+    self.output_pattern = self.output_pattern[:-1] + "},"

[dennis_jeffrey, 2011/03/18 21:44:01]

Prefer single quotes over double quotes in strings.

[Ilya Sherman, 2011/03/18 23:17:11]

This line is confusing -- why are we trimming a space of the end of the string? 
Please add a comment if that's really what you mean to do.

[dyu1, 2011/03/21 18:42:35]

Done.

[dyu1, 2011/03/21 18:42:35]

This is because in the previous "for key_and_method in self.fields:" the
"self.output_pattern += u"u'%s': u'%s', " %(key_and_method[0], "%s")" adds parts
like this: "u'EMAIL_ADDRESS': u'%s', "

There is a space before the last quote there for this is used to trim the space.
How do you suggest I add a comment here? Should I say "Delete the space created
in the previous pattern"?

[Ilya Sherman, 2011/03/22 01:18:58]

It's not clear to me why the space matters, and why we want to trim it.  So I
would appreciate a comment like "Trim the trailing space, because [insert reason
here]."

[dyu1, 2011/03/22 02:52:35]

This is more for aesthetic reasons to remove unneeded whitespace. Such as
{u"NAME_FIRST": u"9",}, versus {u"NAME_FIRST": u"9", },

+
+  def _GenerateField(self, field_construct):
+    """Generates each field in each dictionary.
+
+    Args:
+      field_construct: it is a list of lists.
+        The first value (index 0) of each containing list is a function or None.
+        The rest values are the args. If function is None then arg is just

[dennis_jeffrey, 2011/03/18 21:44:01]

"rest values" --> "remaining values"

[dyu1, 2011/03/21 18:42:35]

Done.

+        returned.
+
+        Example 1: zip_construct = [[ None, u'95110']]. There is one
+        containing list only and function here is None and arg is u'95110'.
+        This just returns u'95110'.
+
+        Example 2: address_construct = [ [ random.randint, 1, 10000],
+        [ None, u'foobar'] ] This has two containing lists and it will return
+        the result of:
+        random.randint(1, 10000) + ' ' + u'foobar'
+        which could be u'7832 foobar'
+    """
+    parts = []
+    for function_and_args in field_construct:
+      function = function_and_args[0]
+      args = function_and_args[1:]
+      if not function:
+        function = lambda x: x
+      parts.append(u'%s' %function(*args))

[dennis_jeffrey, 2011/03/18 21:44:01]

Put a space after the '%' operator.

[dyu1, 2011/03/21 18:42:35]

Done.

+    return (' ').join(parts)

[dennis_jeffrey, 2011/03/18 21:44:01]

I think you can remove the parens around the ' '

[dyu1, 2011/03/21 18:42:35]

Seems weird to me without it. Does it hurt to leave it?

[dennis_jeffrey, 2011/03/22 23:28:52]

No, I don't think it hurts except for requiring two extra characters :-)

+
+  def GenerateAddress(self):
+    """Uses _GenerateField() and address_construct to gen a random address.
+
+    Returns random address.
+    """
+    return self._GenerateField(self.address_construct)
+
+  def GenerateCity(self):
+    """Uses _GenerateField() and city_construct to gen a random city.
+
+    Returns random city.
+    """
+    return self._GenerateField(self.city_construct)
+
+  def GenerateState(self):
+    """Uses _GenerateField() and state_construct to generate a state.
+
+    Returns random state.
+    """
+    return self._GenerateField(self.state_construct)
+
+  def GenerateZip(self):
+    """Uses _GenerateField() and zip_construct to generate a zip code.
+
+    Returns random zip code.
+    """
+    return self._GenerateField(self.zip_construct)
+
+  def GenerateCountry(self):
+    """Uses _GenerateField() and country_construct to generate a country.
+
+    Returns random country.
+    """
+    return self._GenerateField(self.country_construct)
+
+  def GenerateNameFirst(self):
+    """Generates Numerical First Names.

[dennis_jeffrey, 2011/03/18 21:44:01]

"Numerical First Names" --> "a numerical first name"

[dyu1, 2011/03/21 18:42:35]

Done.

+
+    Its Name is the number of the current dict.

[dennis_jeffrey, 2011/03/18 21:44:01]

"Its Name" --> "The name"

[dyu1, 2011/03/21 18:42:35]

Done.

+    i.e. u'1', u'2', u'3'
+
+    Returns random first names.

[dennis_jeffrey, 2011/03/18 21:44:01]

"random first names" --> "a random first name"

[dyu1, 2011/03/21 18:42:35]

Done.

+    """
+    return u'%s' % self.dict_no
+
+  def GenerateEmail(self):
+    """Generates emails that correspond to the First Name.

[dennis_jeffrey, 2011/03/18 21:44:01]

"emails that correspond to the First Name" -->
"an email that corresponds to the first name"

[dyu1, 2011/03/21 18:42:35]

Done.

+
+    i.e. u'1@example.com', u'2@example.com', u'3@example.com'
+
+    Returns random email addresses.

[dennis_jeffrey, 2011/03/18 21:44:01]

"random email addresses" --> "a random email address"

[dyu1, 2011/03/21 18:42:35]

Done.

+    """
+    return u'%s@example.com' % self.dict_no
+
+
+  def GenerateNextDict(self):
+    """Generates next dictionary of the dataset.
+
+    It first increments zero starting dict_no.

[dennis_jeffrey, 2011/03/18 21:44:01]

Probably don't need to mention this in the function docstring.  Seems like more
of an implementation detail.

[dyu1, 2011/03/21 18:42:35]

Done.

+
+    Returns the output dictionary.

[dennis_jeffrey, 2011/03/18 21:44:01]

Returns:
  The output dictionary.

[dyu1, 2011/03/21 18:42:35]

Done.

+    """
+    self.dict_no += 1
+    output_dict = {}
+    for key, method_or_value in self.fields:
+      if not method_or_value:
+        output_dict[key] = ''
+      elif type(method_or_value) in [str, unicode]:
+        output_dict[key] = '%s' % method_or_value
+      else:
+        output_dict[key] = method_or_value()
+    output_dict[key] = self.re_single_quote.sub(
+        r"\'", output_dict[key]) #  escaping single quote: "'" -> "\'"

[dennis_jeffrey, 2011/03/18 21:44:01]

Put one more space before the "#" that starts the comment.  Also start the
comment like this:

# Escaping single quote...

[dennis_jeffrey, 2011/03/18 21:44:01]

Prefer single quotes to double quotes in strings.

[dyu1, 2011/03/21 18:42:35]

Done.

[dyu1, 2011/03/21 18:42:35]

Done.

+    return output_dict
+
+  def GenerateDataset(self, num_of_dict_to_generate=10):
+    """Generates a list of dictionaries.
+
+    Args:
+      num_of_dict_to_generate: The number of dictionaries to be generated.
+      Default value is 10.
+
+    Returns the dictionary list.
+    """
+    if self.output_filename:
+      output_file = codecs.open(
+          self.output_filename, mode = 'wb', encoding = 'utf-8-sig')

[dennis_jeffrey, 2011/03/18 21:44:01]

Remove the spaces around the two "=" in this line.

[dyu1, 2011/03/21 18:42:35]

Done.

+    else:
+      output_file = None
+    try:
+      list_of_dict = []
+      if output_file:
+        output_file.write('[')
+        output_file.write(os.linesep)
+
+      while self.dict_no < num_of_dict_to_generate:
+        output_dict = self.GenerateNextDict()
+        list_of_dict.append(output_dict)
+        output_line = self.output_pattern %tuple(

[dennis_jeffrey, 2011/03/18 21:44:01]

Put a space after the "%"

[dyu1, 2011/03/21 18:42:35]

Done.

+            [output_dict[key_and_method[0]] for key_and_method in self.fields])
+        if output_file:
+          output_file.write(output_line)
+          output_file.write(os.linesep)
+        self.logger.info(
+            "%d: %s" %(self.dict_no, output_line.encode(sys.stdout.encoding,

[dennis_jeffrey, 2011/03/18 21:44:01]

Put a space after the "%"

[dennis_jeffrey, 2011/03/18 21:44:01]

Prefer single quotes to double quotes in strings.

+                                                        'ignore')))
+
+      if output_file:
+        output_file.write(']')
+        output_file.write(os.linesep)
+      self.logger.info("%d dictionaries generated SUCCESSFULLY!", self.dict_no)
+      self.logger.info("--- FINISHED ---")

[dennis_jeffrey, 2011/03/18 21:44:01]

Prefer single quotes to double quotes in strings (in the above 2 lines).

[dyu1, 2011/03/21 18:42:35]

Done.

+      return list_of_dict
+    finally:
+      if output_file:
+        output_file.close()
+
+def main():
+  # Command line options.
+  from optparse import OptionParser

[dennis_jeffrey, 2011/03/18 21:44:01]

This should be up with the rest of the imports at the top of this file.

[dyu1, 2011/03/21 18:42:35]

Done.

+  parser = OptionParser()
+  parser.add_option("-o", "--output",
+           dest="output_filename", default="",

[dennis_jeffrey, 2011/03/18 21:44:01]

This should be indented to line up underneath the other parameters in the
function call parameter list (here and in the other lines below):

parser.add_option("-o", "--output",
                  dest="output_filename", default="",

[dyu1, 2011/03/21 18:42:35]

Done.

+           help="write output to FILE [optional]", metavar="FILE")
+  parser.add_option("-v", "--verbose", action="store_true", dest="verbose",
+                    default=True,

[dennis_jeffrey, 2011/03/18 21:44:01]

Default should probably be False if the action is to store_true.  Otherwise this
option will always be true.

[dyu1, 2011/03/21 18:42:35]

The default should be True since this is the default behavior of the script. The
script is verbose by default. Do you think I should just remove the options for
-v and -q? I think one of them is no use anyways as it will be covered with -l
LOG_LEVEL. 

[dennis_jeffrey, 2011/03/22 23:28:52]

Yes, I recommend removing the -v and -q options and using only the -l option. 
Then you can set its default to the most verbose level, which is "debug".

+           help="display all [default]")
+  parser.add_option("-q", "--quiet", action="store_false", dest="verbose",
+           help="display nothing")

[dennis_jeffrey, 2011/03/18 21:44:01]

What's the default here?

[dyu1, 2011/03/21 18:42:35]

The default is True since both options -v and -q use the same variable for the
state. Both use dest="verbose". I'll probably just remove both of these options
as it's covered by the -l LOG_LEVEL option anyways. thoughts?

+  parser.add_option("-l", "--log", dest="logging_level", default = None,
+           help="LOG_LEVEL: 'debug, 'info', 'warning' or 'error'",
+                    metavar="LOG_LEVEL")

[dennis_jeffrey, 2011/03/18 21:44:01]

Prefer single quotes to double quotes in strings (for lines 265-275).

[dyu1, 2011/03/21 18:42:35]

Done.

+
+  (options, args) = parser.parse_args()
+  if args:
+    parser.print_help()
+    sys.exit(1)
+  if not options.verbose:
+    options.logging_level = None

[dennis_jeffrey, 2011/03/18 21:44:01]

This doesn't seem to make sense.  Why would we clear the "logging_level" option
if the "verbose" option is not true?

[dyu1, 2011/03/21 18:42:35]

Deleted

+  if options.verbose and not options.logging_level:
+    options.logging_level = 'info'

[dennis_jeffrey, 2011/03/18 21:44:01]

"Verbose" usually means the most logging messages.  Wouldn't that be the "debug"
level?

[dyu1, 2011/03/21 18:42:35]

Deleted.

+  if options.logging_level:
+    if 'debug' in options.logging_level.lower():
+      options.logging_level = logging.DEBUG
+    elif 'info' in options.logging_level.lower():
+      options.logging_level = logging.INFO
+    elif 'warn' in options.logging_level.lower():
+      options.logging_level = logging.WARNING
+    elif 'error' in options.logging_level.lower():
+      options.logging_level = logging.ERROR
+
+  gen = DatasetGenerator(options.output_filename, options.logging_level)
+  gen.GenerateDataset(100)

[dennis_jeffrey, 2011/03/18 21:44:01]

Maybe this value "100" should be configurable with a command-line option.

[dyu1, 2011/03/21 18:42:35]

Done.

+
+
+if __name__ == '__main__':
+  main()