Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6246147:
Test Autofill's ability to merge duplicate profiles and... (Closed)
|
Created:
9 years, 10 months ago by dyu1 Modified:
9 years, 7 months ago CC:
chromium-reviews, John Grabowski, anantha, Paweł Hajdan Jr., Ilya Sherman, dennis_jeffrey Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionTest Autofill's ability to merge duplicate profiles and
throw away junk profiles. Includes a dataset converter script
to convert csv file into profile dictionary list.
testMergeDuplicateProfilesInAutofill
Added additional tests:
testFilterMalformedEmailAddresses - covers fixed bug 73654.
testFilterIncompleteAddresses - covers fixed bug 71710.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75344
Patch Set 1 #
Total comments: 6
Patch Set 2 : '' #
Total comments: 21
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 36
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 104
Patch Set 10 : '' #
Total comments: 39
Patch Set 11 : '' #
Total comments: 24
Patch Set 12 : '' #
Messages
Total messages: 19 (0 generated)
Autofill test to test profile merge. Includes the anonymized dataset input file and converted output dictionary list. Also including the converter script.
dataset-converter.py fixed a small bug dealing with line separator in windows.
At a high level, keep in mind that we're adding a C++ analog[1] to this test that will run as part of the continuous build, so we should think of this CL as potentially providing an extra layer of testing for QA. In that light, does this test add extra value for the team? [1] http://codereview.chromium.org/6312174/ Other than that, I don't know this area of our codebase well enough to be a thorough reviewer. A few really minor nits below: http://codereview.chromium.org/6246147/diff/1/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:98: Require a loop of 1000 submits as the toolbar server only collects 1% of nit: "Autofill server" is probably more appropriate -- it's shared by toolbar and chrome http://codereview.chromium.org/6246147/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:129: profiles. The goal is to submit a couple hundre profiles and see the merge nit: "hundred" http://codereview.chromium.org/6246147/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:149: import pickle nit: Are inline imports ok? I really don't know the Python style guide, but this line strikes me as out of place.
http://codereview.chromium.org/6246147/diff/1008/chrome/test/data/autofill/du... File chrome/test/data/autofill/dup-profiles-test.html (right): http://codereview.chromium.org/6246147/diff/1008/chrome/test/data/autofill/du... chrome/test/data/autofill/dup-profiles-test.html:7: <form id="merge_dup" action="dup-profiles-test.html" method="post"> remove "action" key. It's redundant http://codereview.chromium.org/6246147/diff/1008/chrome/test/data/autofill/du... chrome/test/data/autofill/dup-profiles-test.html:10: <label for="NAME_FIRST">First Name:</label> what's the significance of "for="? http://codereview.chromium.org/6246147/diff/1008/chrome/test/data/autofill/du... chrome/test/data/autofill/dup-profiles-test.html:11: <input type="text" id="NAME_FIRST" name="firstname"><br/> Add a comment somewhere mentioning that the form element names must match the keys in dataset_duplicate-profiles.txt http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:139: for i in range(len(self.GetAutoFillProfile()['profiles'])): Why not: for profile in self.GetAutoFillProfile()['profiles']: http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:140: self.NavigateToURL(url, 0, 0) remove 0, 0 http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:148: print self.GetAutoFillProfile()['profiles'] Remove http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:150: # Write profile dictionary to a file. Why? http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:154: output.close() What's the objective of this test? Where do you test it? http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/datas... File chrome/test/functional/dataset-converter.py (right): http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:7: """Takes in a CSV profiles file and outputs to a pyAuto dictionary list format what's a pyauto dictionary list format? http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:11: """ Do you really need dataset.txt? Why not just have dataset_duplicate-profiles.txt and then you won't need this script after all?
http://codereview.chromium.org/6246147/diff/1/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:98: Require a loop of 1000 submits as the toolbar server only collects 1% of On 2011/02/07 21:39:04, Ilya Sherman wrote: > nit: "Autofill server" is probably more appropriate -- it's shared by toolbar > and chrome Done. http://codereview.chromium.org/6246147/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:129: profiles. The goal is to submit a couple hundre profiles and see the merge On 2011/02/07 21:39:04, Ilya Sherman wrote: > nit: "hundred" Done. http://codereview.chromium.org/6246147/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:149: import pickle On 2011/02/07 21:39:04, Ilya Sherman wrote: > nit: Are inline imports ok? I really don't know the Python style guide, but > this line strikes me as out of place. Done. http://codereview.chromium.org/6246147/diff/1008/chrome/test/data/autofill/du... File chrome/test/data/autofill/dup-profiles-test.html (right): http://codereview.chromium.org/6246147/diff/1008/chrome/test/data/autofill/du... chrome/test/data/autofill/dup-profiles-test.html:7: <form id="merge_dup" action="dup-profiles-test.html" method="post"> On 2011/02/07 22:20:02, Nirnimesh wrote: > remove "action" key. It's redundant Done. http://codereview.chromium.org/6246147/diff/1008/chrome/test/data/autofill/du... chrome/test/data/autofill/dup-profiles-test.html:10: <label for="NAME_FIRST">First Name:</label> the for attribute is matching the id attribute, it's just matching tying the label to the control. In this case we're using the form for best practice as this attribute is not used a lot of times. On 2011/02/07 22:20:02, Nirnimesh wrote: > what's the significance of "for="? http://codereview.chromium.org/6246147/diff/1008/chrome/test/data/autofill/du... chrome/test/data/autofill/dup-profiles-test.html:11: <input type="text" id="NAME_FIRST" name="firstname"><br/> On 2011/02/07 22:20:02, Nirnimesh wrote: > Add a comment somewhere mentioning that the form element names must match the > keys in dataset_duplicate-profiles.txt Done. http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:139: for i in range(len(self.GetAutoFillProfile()['profiles'])): The second for loop wont work as profile will be a dictionary value versus an int. On 2011/02/07 22:20:02, Nirnimesh wrote: > Why not: > for profile in self.GetAutoFillProfile()['profiles']: http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:140: self.NavigateToURL(url, 0, 0) On 2011/02/07 22:20:02, Nirnimesh wrote: > remove 0, 0 Done. http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:148: print self.GetAutoFillProfile()['profiles'] On 2011/02/07 22:20:02, Nirnimesh wrote: > Remove Done. http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:150: # Write profile dictionary to a file. Retrieve a data dump to compare against original dictionary list to make sure number are not the same. On 2011/02/07 22:20:02, Nirnimesh wrote: > Why? http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:154: output.close() Final test is a manual test to make sure the profiles are merged properly. This is a large enough feature where we need a test such as this but wont be run on the continuous build. On 2011/02/07 22:20:02, Nirnimesh wrote: > What's the objective of this test? Where do you test it? http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/datas... File chrome/test/functional/dataset-converter.py (right): http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:7: """Takes in a CSV profiles file and outputs to a pyAuto dictionary list format Changed the wording. Input is a csv file with a bunch of profiles and outputs to a dictionary list. On 2011/02/07 22:20:02, Nirnimesh wrote: > what's a pyauto dictionary list format? http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:11: """ Then I have to manually create this list. I was given a csv formatted list and this script allows me to convert it to a dictionary list to be used with the pyauto test. On 2011/02/07 22:20:02, Nirnimesh wrote: > Do you really need dataset.txt? > Why not just have dataset_duplicate-profiles.txt and then you won't need this > script after all?
http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/1008/chrome/test/functional/autof... chrome/test/functional/autofill.py:139: for i in range(len(self.GetAutoFillProfile()['profiles'])): On 2011/02/07 23:06:59, dyu1 wrote: > The second for loop wont work as profile will be a dictionary value versus an > int. > > On 2011/02/07 22:20:02, Nirnimesh wrote: > > Why not: > > for profile in self.GetAutoFillProfile()['profiles']: > for profile in self.GetAutoFillProfile()['profiles']: # |profile| is a dictionary # then.. for key, val in profile.iteritems(): .. http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... File chrome/test/data/autofill/dataset.txt (right): http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... chrome/test/data/autofill/dataset.txt:1: first_name, middle_name, last_name, email, company_name, address_line_1, address_line_2, city, state, zipcode, country, phone, fax Add a comment mentioning that this can be parsed with dataset-converter.py http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... chrome/test/data/autofill/dataset.txt:3: John||Doe|john.doe@gmail.com||1950 Amphitheatre Ave #2||Mountain View|California|14888|US|4195551234| this is not csv http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... File chrome/test/data/autofill/dataset_duplicate-profiles.txt (right): http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... chrome/test/data/autofill/dataset_duplicate-profiles.txt:1: [ if this file can be generated on the fly, why check this in? You should check in either this or the csv file http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/autof... chrome/test/functional/autofill.py:8: import pickle group with os above. pickle is a system import http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/autof... chrome/test/functional/autofill.py:137: 'dataset_duplicate-profiles.txt') Why use this precooked file? Why not call the csv parser script to generate this file on the fly? http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... File chrome/test/functional/dataset-converter.py (right): http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:2: # Copyright (c) 2010 The Chromium Authors. All rights reserved. 2011 http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:5: Remove blank line http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:15: INPUT_FILE = r"../data/autofill/dataset.txt" Can these be moved inside the class as memeber vars? http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:42: Leave another blank line http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:43: class Converter(object): Use a more descriptive name. |Converter| is too general http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:44: def __init__(self, fields, filein, fileout): Explain the args. Add an "Args:" section in the docstring http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:46: The pattern is a regular expression which has named parenthesis groups An example would explain this much better http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:75: self.fields = fields[:] self._fields use _ prefix for member vars Repeat for all other member vars http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:78: self.pattern += '\|(?P<%s>.*?)' %key Isn't it easier to split by '|'? re.split('|', line) http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:89: def __getRec(self, line): why double _? Method names begin with Cap letter. _GetRec. Use more descriptive func name. Repeat for all method names http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:91: Constructs and returns a dictionary from a line using patterns. Merge with last line. Repeat for all methods http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:94: rePat = re.compile("'", re.UNICODE) the style guide prohibits camelCase style for local vars Use underscored_var_names. Repeat for all local vars http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:151: c = Converter(FIELDS, INPUT_FILE, OUTPUT_FILE) Do you really need OUTPUT_FILE? You could just return a dictionary as the test expects
I made some changes in the latest revision: 1. I added two functional tests in pyauto for autofill, which covers two fixed bugs that I verified yesterday. One is testing the filtering for incomplete addresses and the other is testing the filtering of malformed email addresses. 2. I removed the converted output file since the converter script will now create data on the fly and use it within the pyauto test as suggested by Nirnimesh. 3. I renamed the converter script name from dataset-converter.py to dataset_converter.py. 4. Moved everything in the converter script into the class. I want to mention that I have have two methods with the same name, one is a private method. I could have put the code into one except then I would have duplicate code since I call the private method in two places, so I decided to do it this way instead. http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... File chrome/test/data/autofill/dataset.txt (right): http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... chrome/test/data/autofill/dataset.txt:1: first_name, middle_name, last_name, email, company_name, address_line_1, address_line_2, city, state, zipcode, country, phone, fax On 2011/02/07 23:26:20, Nirnimesh wrote: > Add a comment mentioning that this can be parsed with dataset-converter.py Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... chrome/test/data/autofill/dataset.txt:3: John||Doe|john.doe@gmail.com||1950 Amphitheatre Ave #2||Mountain View|California|14888|US|4195551234| On 2011/02/07 23:26:20, Nirnimesh wrote: > this is not csv Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... File chrome/test/data/autofill/dataset_duplicate-profiles.txt (right): http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... chrome/test/data/autofill/dataset_duplicate-profiles.txt:1: [ Will check in the dataset file instead. On 2011/02/07 23:26:20, Nirnimesh wrote: > if this file can be generated on the fly, why check this in? > You should check in either this or the csv file http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/autof... chrome/test/functional/autofill.py:8: import pickle On 2011/02/07 23:26:20, Nirnimesh wrote: > group with os above. > pickle is a system import Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/autof... chrome/test/functional/autofill.py:137: 'dataset_duplicate-profiles.txt') On 2011/02/07 23:26:20, Nirnimesh wrote: > Why use this precooked file? Why not call the csv parser script to generate this > file on the fly? Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... File chrome/test/functional/dataset-converter.py (right): http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:2: # Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2011/02/07 23:26:20, Nirnimesh wrote: > 2011 Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:5: On 2011/02/07 23:26:20, Nirnimesh wrote: > Remove blank line Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:15: INPUT_FILE = r"../data/autofill/dataset.txt" On 2011/02/07 23:26:20, Nirnimesh wrote: > Can these be moved inside the class as memeber vars? Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:42: On 2011/02/07 23:26:20, Nirnimesh wrote: > Leave another blank line Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:43: class Converter(object): On 2011/02/07 23:26:20, Nirnimesh wrote: > Use a more descriptive name. |Converter| is too general Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:44: def __init__(self, fields, filein, fileout): On 2011/02/07 23:26:20, Nirnimesh wrote: > Explain the args. Add an "Args:" section in the docstring Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:46: The pattern is a regular expression which has named parenthesis groups On 2011/02/07 23:26:20, Nirnimesh wrote: > An example would explain this much better Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:75: self.fields = fields[:] On 2011/02/07 23:26:20, Nirnimesh wrote: > self._fields > use _ prefix for member vars > Repeat for all other member vars Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:78: self.pattern += '\|(?P<%s>.*?)' %key On 2011/02/07 23:26:20, Nirnimesh wrote: > Isn't it easier to split by '|'? > re.split('|', line) Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:89: def __getRec(self, line): Based on this http://docs.python.org/tutorial/classes.html#tut-private But I changed it. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:91: Constructs and returns a dictionary from a line using patterns. On 2011/02/07 23:26:20, Nirnimesh wrote: > Merge with last line. Repeat for all methods Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:94: rePat = re.compile("'", re.UNICODE) On 2011/02/07 23:26:20, Nirnimesh wrote: > the style guide prohibits camelCase style for local vars > Use underscored_var_names. Repeat for all local vars Done. http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... chrome/test/functional/dataset-converter.py:151: c = Converter(FIELDS, INPUT_FILE, OUTPUT_FILE) Will write this script so that it be standalone and return an output file so the pyauto test can use it on the fly. On 2011/02/07 23:26:20, Nirnimesh wrote: > Do you really need OUTPUT_FILE? > You could just return a dictionary as the test expects
Since we're renaming files how about: duplicate_profiles_test.html On 2011/02/09 19:44:56, dyu1 wrote: > I made some changes in the latest revision: > > 1. I added two functional tests in pyauto for autofill, which covers two fixed > bugs that I verified yesterday. One is testing the filtering for incomplete > addresses and the other is testing the filtering of malformed email addresses. > > 2. I removed the converted output file since the converter script will now > create data on the fly and use it within the pyauto test as suggested by > Nirnimesh. > > 3. I renamed the converter script name from dataset-converter.py to > dataset_converter.py. > > 4. Moved everything in the converter script into the class. I want to mention > that I have have two methods with the same name, one is a private method. I > could have put the code into one except then I would have duplicate code since I > call the private method in two places, so I decided to do it this way instead. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... > File chrome/test/data/autofill/dataset.txt (right): > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... > chrome/test/data/autofill/dataset.txt:1: first_name, middle_name, last_name, > email, company_name, address_line_1, address_line_2, city, state, zipcode, > country, phone, fax > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Add a comment mentioning that this can be parsed with dataset-converter.py > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... > chrome/test/data/autofill/dataset.txt:3: John||Doe|john.doe@gmail.com||1950 > Amphitheatre Ave #2||Mountain View|California|14888|US|4195551234| > On 2011/02/07 23:26:20, Nirnimesh wrote: > > this is not csv > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... > File chrome/test/data/autofill/dataset_duplicate-profiles.txt (right): > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/data/autofill/da... > chrome/test/data/autofill/dataset_duplicate-profiles.txt:1: [ > Will check in the dataset file instead. > > On 2011/02/07 23:26:20, Nirnimesh wrote: > > if this file can be generated on the fly, why check this in? > > You should check in either this or the csv file > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/autof... > File chrome/test/functional/autofill.py (right): > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/autof... > chrome/test/functional/autofill.py:8: import pickle > On 2011/02/07 23:26:20, Nirnimesh wrote: > > group with os above. > > pickle is a system import > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/autof... > chrome/test/functional/autofill.py:137: 'dataset_duplicate-profiles.txt') > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Why use this precooked file? Why not call the csv parser script to generate > this > > file on the fly? > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > File chrome/test/functional/dataset-converter.py (right): > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:2: # Copyright (c) 2010 The Chromium > Authors. All rights reserved. > On 2011/02/07 23:26:20, Nirnimesh wrote: > > 2011 > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:5: > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Remove blank line > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:15: INPUT_FILE = > r"../data/autofill/dataset.txt" > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Can these be moved inside the class as memeber vars? > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:42: > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Leave another blank line > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:43: class Converter(object): > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Use a more descriptive name. |Converter| is too general > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:44: def __init__(self, fields, > filein, fileout): > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Explain the args. Add an "Args:" section in the docstring > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:46: The pattern is a regular > expression which has named parenthesis groups > On 2011/02/07 23:26:20, Nirnimesh wrote: > > An example would explain this much better > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:75: self.fields = fields[:] > On 2011/02/07 23:26:20, Nirnimesh wrote: > > self._fields > > use _ prefix for member vars > > Repeat for all other member vars > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:78: self.pattern += '\|(?P<%s>.*?)' > %key > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Isn't it easier to split by '|'? > > re.split('|', line) > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:89: def __getRec(self, line): > Based on this http://docs.python.org/tutorial/classes.html#tut-private > But I changed it. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:91: Constructs and returns a > dictionary from a line using patterns. > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Merge with last line. Repeat for all methods > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:94: rePat = re.compile("'", > re.UNICODE) > On 2011/02/07 23:26:20, Nirnimesh wrote: > > the style guide prohibits camelCase style for local vars > > Use underscored_var_names. Repeat for all local vars > > Done. > > http://codereview.chromium.org/6246147/diff/3009/chrome/test/functional/datas... > chrome/test/functional/dataset-converter.py:151: c = Converter(FIELDS, > INPUT_FILE, OUTPUT_FILE) > Will write this script so that it be standalone and return an output file so the > pyauto test can use it on the fly. > > On 2011/02/07 23:26:20, Nirnimesh wrote: > > Do you really need OUTPUT_FILE? > > You could just return a dictionary as the test expects
I'm still new to looking at Chrome test code, but as requested, I took a look at your Python code in this CL and tried to offer any helpful suggestions that I could :-) http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:101: bug 71710.""" The first line of this comment should be a 1-line summary of the method. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:109: for p in profiles: 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). http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:113: 'window.domAutomationController.send("done")' % (key, value) 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) http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:118: 0, 0) 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) http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:123: submitted. Regression test for bug 73654.""" The first line of this comment should be a 1-line summary of the method. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:135: for p in profiles: It also looks like there's only 1 profile here, so maybe we don't need a loop. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:139: 'window.domAutomationController.send("done")' % (key, value) Same comment as line 113 above. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:144: 0, 0) Same comment as line 118 above. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:148: print 'TEST PASS: Malformed email address is not saved in profiles.' I think we can remove lines 147-148. Probably no need for a print statement if the test passes. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:152: Require a loop of 1000 submits as the Autofill server only collects 1% of The first line of this comment should be a 1-line summary of the method. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:184: outcome.""" The first line of this comment should be a 1-line summary of the method. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:191: self.DataDir(), 'autofill', 'dataset.txt')) Move the "os.path.join(" down to the next line: c = dataset_converter.DatasetConverter( os.path.join(self.DataDir(), 'autofill', 'dataset.txt')) http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:198: 'window.domAutomationController.send("done")' % (key, value) Same comment as line 113 above. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:207: output.close() 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? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... File chrome/test/functional/dataset_converter.py (right): http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:6: """Takes in a dataset profiles file and outputs to a dictionary list format The first line of this comment should be a 1-line summary of this module. Currently it's using 2 lines. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:15: import os These should be specified in alphabetical order. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:21: display_converted_lines = False): Don't put spaces around the "=" when you're defining the default argument values. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:21: display_converted_lines = False): Using the "logging" module with different verbosity levels might be a better approach, as opposed to having different parameters to describe what should or should not be displayed. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:91: ] Since _fields is just a constant array, would it be better declared as a class attribute rather than a data attribute (since all objects of the DatasetConverter class would have the same set of _fields anyway, right?). http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:94: self._output_pattern += u"u'%s': u'%s', " %(key, "%s") I think this could be re-written like this: self._output_pattern += u"u'%s': u'%%s', " % key http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:97: self._input_filename = input_filename We should probably check to ensure that input_filename refers to a valid file and raise an exception if not. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:102: self._record_length = len(self._fields) Perhaps we could remove this variable and just replace its two uses below with "len(self._fields)" itself? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:104: def CreateDictionaryFromRecord(self, line): If this function is only used by the _Convert() function below, then should the function name here also be preceded by an underscore? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:106: Escapes single quotation first and uses split('|') to separate values. This first line of the comment should be a 1-line summary of the method. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:117: Arg: "Arg" --> "Args" (I think it should be "Args" even if there's only one). http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:118: line: row of record from the dataset file. Since this method returns something, you should have a "Returns:" section after the "Args:" section. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:120: # Ignore irrelevant record lines such as comment lines. Besides comment lines, what other lines are considered irrelevant? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:121: if not '|' in line: What if a comment contains a "|" character? Then will the code below erroneously try to process that comment line? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:122: return Is it possible to have a valid line that does not contain any "|", for example, if the line only contains a single value? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:124: line = re_pattern.sub(r"\'", line) You might want to add a comment to describe what you're doing in these two lines. It looks weird to call "compile" on a quote character, and then just substitute that for the line. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:133: return How about raising an exception rather than just returning nothing? Also, you might want to consider using the "logging" module rather than using "print". http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:138: i += 1 It looks like here, you're assuming that the order in which entries are specified in line_list, matches the order in which keys are considered when you iterate through self._fields. Is that really ok to assume? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:142: """The real conversion takes place here. I think it would be more useful to say what's being converted in this comment. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:146: output_file: the converted dictionary list output file. Since this function returns something, you need a "Returns:" section after the "Args:" section. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:162: output_line = self._output_pattern %tuple( Put a space after the "%". http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:169: print "\n%d: %s" %(i, line.encode(sys.stdout.encoding, 'ignore')) Put a space after the "%". http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:171: print "\tconverted to: %s" %output_line.encode( You may want to consider using the "logging" module rather than "print". http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:171: print "\tconverted to: %s" %output_line.encode( Put a space after the "%". http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:175: print "\t%d lines converted so far!" %i Put a space after the "%". http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:175: print "\t%d lines converted so far!" %i I assume all lines should be converted nearly instantaneously from the perspective of a human user (unless input files can be huge). Is it really helpful to print a message after every 10 lines are converted? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:181: print "%d lines converted SUCCESSFULLY!" %i Put a space after the "%". http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:183: print Again, consider using "logging" instead of "print". That way, you can specify verbosity levels for the different printed messages, and let a user specify what level of verbosity they want to see as output. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:187: """Takes arguments of two file names and creates two file objects, then This method actually doesn't take any parameter arguments. It just uses the values of two data attributes of the current object. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:188: calls _Convert() with these two file objects to do the real conversion.""" The first comment line should be a 1-line summary of this method. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:192: encoding = 'utf-8-sig') as output_file: Remove the spaces around the "=" when specifying the named parameter values. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:196: Should have an extra blank line here: the style guide says to put 2 blank lines before top-level definitions. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:198: c = DatasetConverter(r'../data/autofill/dataset.txt', Is it better to hard-code the input filename and output filename here, or would it be better to specify these as command-line inputs to the main() function? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:199: r'../data/autofill/dataset_duplicate-profiles.txt') The second argument should line up underneath the first argument.
http://codereview.chromium.org/6246147/diff/20001/chrome/test/data/autofill/d... File chrome/test/data/autofill/dataset.txt (right): http://codereview.chromium.org/6246147/diff/20001/chrome/test/data/autofill/d... chrome/test/data/autofill/dataset.txt:1: This dataset file can be parsed with dataset-converter.py dataset_converter.py (note: underscore) http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... File chrome/test/functional/dataset_converter.py (right): http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:40: The pattern is a regular expression which has named parenthesis groups I think the input/output pattern above is illustrative enough. The long description seems overly detailed to me -- and seems like describing the code. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:92: self._output_pattern = u"{" prefer single quote char '
http://codereview.chromium.org/6246147/diff/20001/chrome/test/data/autofill/d... File chrome/test/data/autofill/dataset.txt (right): http://codereview.chromium.org/6246147/diff/20001/chrome/test/data/autofill/d... chrome/test/data/autofill/dataset.txt:1: This dataset file can be parsed with dataset-converter.py On 2011/02/11 19:39:54, Nirnimesh wrote: > dataset_converter.py (note: underscore) Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:101: bug 71710.""" Done. On 2011/02/11 00:53:17, dennisjeffrey wrote: > The first line of this comment should be a 1-line summary of the method. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:109: for p in profiles: Done. Changed the array to a dict. On 2011/02/11 00:53:17, dennisjeffrey wrote: > 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). http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:113: 'window.domAutomationController.send("done")' % (key, value) On 2011/02/11 00:53:17, dennisjeffrey wrote: > 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) Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:118: 0, 0) On 2011/02/11 00:53:17, dennisjeffrey wrote: > 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) Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:123: submitted. Regression test for bug 73654.""" On 2011/02/11 00:53:17, dennisjeffrey wrote: > The first line of this comment should be a 1-line summary of the method. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:135: for p in profiles: On 2011/02/11 00:53:17, dennisjeffrey wrote: > It also looks like there's only 1 profile here, so maybe we don't need a loop. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:139: 'window.domAutomationController.send("done")' % (key, value) On 2011/02/11 00:53:17, dennisjeffrey wrote: > Same comment as line 113 above. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:144: 0, 0) On 2011/02/11 00:53:17, dennisjeffrey wrote: > Same comment as line 118 above. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:148: print 'TEST PASS: Malformed email address is not saved in profiles.' On 2011/02/11 00:53:17, dennisjeffrey wrote: > I think we can remove lines 147-148. Probably no need for a print statement if > the test passes. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:152: Require a loop of 1000 submits as the Autofill server only collects 1% of On 2011/02/11 00:53:17, dennisjeffrey wrote: > The first line of this comment should be a 1-line summary of the method. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:184: outcome.""" On 2011/02/11 00:53:17, dennisjeffrey wrote: > The first line of this comment should be a 1-line summary of the method. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:191: self.DataDir(), 'autofill', 'dataset.txt')) On 2011/02/11 00:53:17, dennisjeffrey wrote: > Move the "os.path.join(" down to the next line: > > c = dataset_converter.DatasetConverter( > os.path.join(self.DataDir(), 'autofill', 'dataset.txt')) Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:198: 'window.domAutomationController.send("done")' % (key, value) On 2011/02/11 00:53:17, dennisjeffrey wrote: > Same comment as line 113 above. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:207: output.close() 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: > 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? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... File chrome/test/functional/dataset_converter.py (right): http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:6: """Takes in a dataset profiles file and outputs to a dictionary list format On 2011/02/11 00:53:17, dennisjeffrey wrote: > The first line of this comment should be a 1-line summary of this module. > Currently it's using 2 lines. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:15: import os On 2011/02/11 00:53:17, dennisjeffrey wrote: > These should be specified in alphabetical order. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:21: display_converted_lines = False): On 2011/02/11 00:53:17, dennisjeffrey wrote: > Using the "logging" module with different verbosity levels might be a better > approach, as opposed to having different parameters to describe what should or > should not be displayed. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:40: The pattern is a regular expression which has named parenthesis groups On 2011/02/11 19:39:54, Nirnimesh wrote: > I think the input/output pattern above is illustrative enough. > The long description seems overly detailed to me -- and seems like describing > the code. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:91: ] On 2011/02/11 00:53:17, dennisjeffrey wrote: > Since _fields is just a constant array, would it be better declared as a class > attribute rather than a data attribute (since all objects of the > DatasetConverter class would have the same set of _fields anyway, right?). Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:92: self._output_pattern = u"{" On 2011/02/11 19:39:54, Nirnimesh wrote: > prefer single quote char ' Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:94: self._output_pattern += u"u'%s': u'%s', " %(key, "%s") On 2011/02/11 00:53:17, dennisjeffrey wrote: > I think this could be re-written like this: > > self._output_pattern += u"u'%s': u'%%s', " % key Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:97: self._input_filename = input_filename On 2011/02/11 00:53:17, dennisjeffrey wrote: > We should probably check to ensure that input_filename refers to a valid file > and raise an exception if not. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:102: self._record_length = len(self._fields) On 2011/02/11 00:53:17, dennisjeffrey wrote: > Perhaps we could remove this variable and just replace its two uses below with > "len(self._fields)" itself? Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:104: def CreateDictionaryFromRecord(self, line): On 2011/02/11 00:53:17, dennisjeffrey wrote: > If this function is only used by the _Convert() function below, then should the > function name here also be preceded by an underscore? Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:106: Escapes single quotation first and uses split('|') to separate values. On 2011/02/11 00:53:17, dennisjeffrey wrote: > This first line of the comment should be a 1-line summary of the method. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:117: Arg: On 2011/02/11 00:53:17, dennisjeffrey wrote: > "Arg" --> "Args" > > (I think it should be "Args" even if there's only one). Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:118: line: row of record from the dataset file. On 2011/02/11 00:53:17, dennisjeffrey wrote: > Since this method returns something, you should have a "Returns:" section after > the "Args:" section. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:120: # Ignore irrelevant record lines such as comment lines. On 2011/02/11 00:53:17, dennisjeffrey wrote: > Besides comment lines, what other lines are considered irrelevant? Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:121: if not '|' in line: No, I have a check in place (line 129) where it checks if the pipe separates 13 fields (12 pipes/13 fields) and if not then it just displays and error line and return nothing (on the fly it will ignore that line completely and move to the next line). On 2011/02/11 00:53:17, dennisjeffrey wrote: > What if a comment contains a "|" character? Then will the code below > erroneously try to process that comment line? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:122: return Well the dataset given to me is in the following format john||doe|john.doe@gmail.com||1950 Amphitheatre Ave #2||||14888|US|4195551234| and contains total of 12 pipes and that's what I'm parsing for and converting it a dictionary list output on the fly. It might still be valid but the 243 records given doesn't seem to be of that format you asked. On 2011/02/11 00:53:17, dennisjeffrey wrote: > Is it possible to have a valid line that does not contain any "|", for example, > if the line only contains a single value? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:124: line = re_pattern.sub(r"\'", line) On 2011/02/11 00:53:17, dennisjeffrey wrote: > You might want to add a comment to describe what you're doing in these two > lines. It looks weird to call "compile" on a quote character, and then just > substitute that for the line. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:133: return Done for logging. If I raise an exception here then the script will stop rather than continuing on and just ignoring the line, which is what I need it to do when creating the list of dictionaries on the fly in my pyauto test. On 2011/02/11 00:53:17, dennisjeffrey wrote: > How about raising an exception rather than just returning nothing? Also, you > might want to consider using the "logging" module rather than using "print". http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:138: i += 1 Yes, since the order of the keys from the order in the _fields list. The ordering will not change and it's the order of the entries in the line_list as they appear. On 2011/02/11 00:53:17, dennisjeffrey wrote: > It looks like here, you're assuming that the order in which entries are > specified in line_list, matches the order in which keys are considered when you > iterate through self._fields. Is that really ok to assume? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:142: """The real conversion takes place here. On 2011/02/11 00:53:17, dennisjeffrey wrote: > I think it would be more useful to say what's being converted in this comment. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:146: output_file: the converted dictionary list output file. On 2011/02/11 00:53:17, dennisjeffrey wrote: > Since this function returns something, you need a "Returns:" section after the > "Args:" section. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:162: output_line = self._output_pattern %tuple( On 2011/02/11 00:53:17, dennisjeffrey wrote: > Put a space after the "%". Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:169: print "\n%d: %s" %(i, line.encode(sys.stdout.encoding, 'ignore')) On 2011/02/11 00:53:17, dennisjeffrey wrote: > Put a space after the "%". Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:171: print "\tconverted to: %s" %output_line.encode( On 2011/02/11 00:53:17, dennisjeffrey wrote: > Put a space after the "%". Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:181: print "%d lines converted SUCCESSFULLY!" %i On 2011/02/11 00:53:17, dennisjeffrey wrote: > Put a space after the "%". Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:183: print On 2011/02/11 00:53:17, dennisjeffrey wrote: > Again, consider using "logging" instead of "print". That way, you can specify > verbosity levels for the different printed messages, and let a user specify what > level of verbosity they want to see as output. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:187: """Takes arguments of two file names and creates two file objects, then On 2011/02/11 00:53:17, dennisjeffrey wrote: > This method actually doesn't take any parameter arguments. It just uses the > values of two data attributes of the current object. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:188: calls _Convert() with these two file objects to do the real conversion.""" On 2011/02/11 00:53:17, dennisjeffrey wrote: > The first comment line should be a 1-line summary of this method. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:192: encoding = 'utf-8-sig') as output_file: On 2011/02/11 00:53:17, dennisjeffrey wrote: > Remove the spaces around the "=" when specifying the named parameter values. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:196: On 2011/02/11 00:53:17, dennisjeffrey wrote: > Should have an extra blank line here: the style guide says to put 2 blank lines > before top-level definitions. Done. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:198: c = DatasetConverter(r'../data/autofill/dataset.txt', Well command-line input would be find for the standalone version of this script but what about creating the dictionary list on the fly in the pyAuto test? On 2011/02/11 00:53:17, dennisjeffrey wrote: > Is it better to hard-code the input filename and output filename here, or would > it be better to specify these as command-line inputs to the main() function? http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:199: r'../data/autofill/dataset_duplicate-profiles.txt') On 2011/02/11 00:53:17, dennisjeffrey wrote: > The second argument should line up underneath the first argument. Done.
Thanks a lot for making those python changes! I have some more comments below. I don't mean to be nit-picky here; I just want to give whatever helpful suggestions I can think of. If you think any of my comments don't make sense or would require too much effort for little pay-off, feel free to ignore (or maybe leave as a TODO in the code?). http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/auto... chrome/test/functional/autofill.py:207: output.close() On 2011/02/16 03:17:31, dyu1 wrote: > 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: > > 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? > 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?). http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... File chrome/test/functional/dataset_converter.py (right): http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:121: if not '|' in line: On 2011/02/16 03:17:31, dyu1 wrote: > No, I have a check in place (line 129) where it checks if the pipe separates 13 > fields (12 pipes/13 fields) and if not then it just displays and error line and > return nothing (on the fly it will ignore that line completely and move to the > next line). > > On 2011/02/11 00:53:17, dennisjeffrey wrote: > > What if a comment contains a "|" character? Then will the code below > > erroneously try to process that comment line? > Oh, ok. I didn't realize that each line is expected to be exactly 13 fields separated by 12 pipes. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:122: return On 2011/02/16 03:17:31, dyu1 wrote: > Well the dataset given to me is in the following format > john||doe|john.doe@gmail.com||1950 Amphitheatre Ave #2||||14888|US|4195551234| > > and contains total of 12 pipes and that's what I'm parsing for and converting it > a dictionary list output on the fly. It might still be valid but the 243 records > given doesn't seem to be of that format you asked. > On 2011/02/11 00:53:17, dennisjeffrey wrote: > > Is it possible to have a valid line that does not contain any "|", for > example, > > if the line only contains a single value? > Ok, I see. I was thinking that in general, a record with a single field may have no '|' characters. I didn't realize that you're parsing only a particular record format here with 13 fields and 12 pipes. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:124: line = re_pattern.sub(r"\'", line) On 2011/02/16 03:17:31, dyu1 wrote: > On 2011/02/11 00:53:17, dennisjeffrey wrote: > > You might want to add a comment to describe what you're doing in these two > > lines. It looks weird to call "compile" on a quote character, and then just > > substitute that for the line. > > Done. Oops, sorry - Now that I see your comment, I realize that I misread the original line (See, the comments can help! :-D). http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:133: return On 2011/02/16 03:17:31, dyu1 wrote: > Done for logging. > > If I raise an exception here then the script will stop rather than continuing on > and just ignoring the line, which is what I need it to do when creating the list > of dictionaries on the fly in my pyauto test. > > On 2011/02/11 00:53:17, dennisjeffrey wrote: > > How about raising an exception rather than just returning nothing? Also, you > > might want to consider using the "logging" module rather than using "print". > Ok, I think a logging.warning like what you do now is a good solution in this case. http://codereview.chromium.org/6246147/diff/20001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:198: c = DatasetConverter(r'../data/autofill/dataset.txt', On 2011/02/16 03:17:31, dyu1 wrote: > Well command-line input would be find for the standalone version of this script > but what about creating the dictionary list on the fly in the pyAuto test? > > On 2011/02/11 00:53:17, dennisjeffrey wrote: > > Is it better to hard-code the input filename and output filename here, or > would > > it be better to specify these as command-line inputs to the main() function? > When this module is invoked via the PyAuto test, the input and output filenames will be passed as input to this module when class DatasetConverter is instantiated (just as you're already doing). However, when running this module as a standalone program, right now it uses hard-coded values for the input and output filenames. I was just wondering whether it might be more useful to allow a user, when invoking this module as a standalone program, to specify the desired input and output filenames (perhaps the existing hardcoded filenames could be used as defaults, but at least it seems useful to allow the user to override these defaults if desired). Also, since you're now using the logging module, then when this module is invoked from a PyAuto test, you should probably have the PyAuto test pass as input (to the class DatasetConverter constructor) the desired verbosity level. Next, in the event this module is invoked as a standalone program, you may want to allow the user to specify the desired verbosity level too, as a command-line argument to this program. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:110: 'window.domAutomationController.send("done")') % (key, value) Indent this line by 1 more space so the strings on both lines line up. Also, you may want to put a semicolon after the Javascript statement at this line (may not be necessary though). http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:137: 'window.domAutomationController.send("done")') % (key, value) You might want to put a semicolon after the Javascript statement in this line (though it may not be necessary). http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:159: # Autofill server captures 2.5% of the data posted. Is there any verification we can do to check that the Autofill server is receiving the data we're sending in this test? Also, is it really necessary to submit the data 1000 times? Based on your comment here, it seems that if we only do it 100 times, we'd still expect the Autofill server to capture the data in about 2 of those 100 cases. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:171: 'window.domAutomationController.send("done")' % email In lines 166-171 above, use Python's ability to concatenate strings within parens, rather than using '\'. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:176: 'window.domAutomationController.send("done")', Might want to add a semicolon after the Javascript statement in this line. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:194: 'window.domAutomationController.send("done")') % (key, value) Same comment as line 110 above. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:197: 'window.domAutomationController.send("done")', Might want to add a semicolon after the Javascript statement in this line. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:200: len(list_of_dict) > len(self.GetAutoFillProfile()['profiles'])) Add comment here to indicate why we're doing this check (e.g., "Verify that the total number of inputted profiles is greater than the final number of profiles after merging."). http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... File chrome/test/functional/dataset_converter.py (right): http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:20: pass Right now it looks like you will never see any logging messages because all logs are going through this NullHandler. I think what you should do here is to set the logging level dynamically, based on what is requested by the user: (1) The user might be a PyAuto test, in which case they can pass the desired verbosity level as input to the DatasetConverter constructor. (2) The user might be running this as a standalone program using the main() function, in which case you might want to provide a command-line option that can let the user specify the desired logging level. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:21: Put one more blank line here, to separate these two classes. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:66: args: Capitalize "a" in "args". http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:68: output_filename: name and path of the converted file, default is none. Since this method can now possibly raise "IOError", you should add a "Raises:" section after the "Args:" section of this comment. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:90: line: row of record from the dataset file. Maybe a variable name of "record" might be better than "line"? Each line represents a record, right? http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:94: same as the output_record. In the "Returns:" section, I think you don't need to list the variable name that is returned. This is because when you want to know what is returned from a function, you don't care what variable internal to the function stored that value; all you care about is what kind of thing is actually returned. So in this case, I think you can just say something like this: Returns: A dictionary representing a a single record from the dataset file. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:96: # Ignore irrelevant record lines that does not contain '|'. "does" --> "do" http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:106: 'A "|" seperated line has %d fields instead of %d: %s' % ( "seperated" --> "separated" http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:119: The output pattern takes place in this function. Each field needs to be What does it mean for an "output pattern" to "take place"? http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:127: list_of_dict: list that holds all the dictionaries. Can remove the returned variable name "list_of_dict" here. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:160: """Uses values of the two data attributes of the current objects.""" I think a more descriptive comment might be something like this: """Wrapper function to convert input data into the desired output format."""
One more thing I just remembered: the file "dataset_converter.py" right now is used for autofill data conversion. Since the file is located in chrome/test/functional (a more general folder), maybe a file name like "autofill_dataset_converter.py" might be more descriptive.
http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:159: # Autofill server captures 2.5% of the data posted. On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Is there any verification we can do to check that the Autofill server is > receiving the data we're sending in this test? > Not really. The Autofill server processes the data offline, so I can take a few days for the "result" to be detectable. > Also, is it really necessary to submit the data 1000 times? Based on your > comment here, it seems that if we only do it 100 times, we'd still expect the > Autofill server to capture the data in about 2 of those 100 cases. The Autofill server use a statistical model to determine field types. Only 2 samples will not exceed the server's threshold of "noise" so 1000 iterations is a safe minimum.
Got rid of the _Convert() function and added StreamHandler to specify logging_level in __init__ http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:110: 'window.domAutomationController.send("done")') % (key, value) On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Indent this line by 1 more space so the strings on both lines line up. Also, > you may want to put a semicolon after the Javascript statement at this line (may > not be necessary though). Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:137: 'window.domAutomationController.send("done")') % (key, value) On 2011/02/16 19:43:29, dennis_jeffrey wrote: > You might want to put a semicolon after the Javascript statement in this line > (though it may not be necessary). Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:171: 'window.domAutomationController.send("done")' % email On 2011/02/16 19:43:29, dennis_jeffrey wrote: > In lines 166-171 above, use Python's ability to concatenate strings within > parens, rather than using '\'. Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:176: 'window.domAutomationController.send("done")', On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Might want to add a semicolon after the Javascript statement in this line. Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:194: 'window.domAutomationController.send("done")') % (key, value) On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Same comment as line 110 above. Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:197: 'window.domAutomationController.send("done")', On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Might want to add a semicolon after the Javascript statement in this line. Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:200: len(list_of_dict) > len(self.GetAutoFillProfile()['profiles'])) On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Add comment here to indicate why we're doing this check (e.g., "Verify that the > total number of inputted profiles is greater than the final number of profiles > after merging."). Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... File chrome/test/functional/dataset_converter.py (right): http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:20: pass On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Right now it looks like you will never see any logging messages because all logs > are going through this NullHandler. I think what you should do here is to set > the logging level dynamically, based on what is requested by the user: > > (1) The user might be a PyAuto test, in which case they can pass the desired > verbosity level as input to the DatasetConverter constructor. > > (2) The user might be running this as a standalone program using the main() > function, in which case you might want to provide a command-line option that can > let the user specify the desired logging level. Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:21: On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Put one more blank line here, to separate these two classes. Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:66: args: On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Capitalize "a" in "args". Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:68: output_filename: name and path of the converted file, default is none. On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Since this method can now possibly raise "IOError", you should add a "Raises:" > section after the "Args:" section of this comment. Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:90: line: row of record from the dataset file. On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Maybe a variable name of "record" might be better than "line"? Each line > represents a record, right? Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:94: same as the output_record. On 2011/02/16 19:43:29, dennis_jeffrey wrote: > In the "Returns:" section, I think you don't need to list the variable name that > is returned. This is because when you want to know what is returned from a > function, you don't care what variable internal to the function stored that > value; all you care about is what kind of thing is actually returned. So in > this case, I think you can just say something like this: > > Returns: > A dictionary representing a a single record from the > dataset file. Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:96: # Ignore irrelevant record lines that does not contain '|'. On 2011/02/16 19:43:29, dennis_jeffrey wrote: > "does" --> "do" Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:106: 'A "|" seperated line has %d fields instead of %d: %s' % ( On 2011/02/16 19:43:29, dennis_jeffrey wrote: > "seperated" --> "separated" Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:119: The output pattern takes place in this function. Each field needs to be Removed this function. On 2011/02/16 19:43:29, dennis_jeffrey wrote: > What does it mean for an "output pattern" to "take place"? http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:127: list_of_dict: list that holds all the dictionaries. On 2011/02/16 19:43:29, dennis_jeffrey wrote: > Can remove the returned variable name "list_of_dict" here. Done. http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/data... chrome/test/functional/dataset_converter.py:160: """Uses values of the two data attributes of the current objects.""" On 2011/02/16 19:43:29, dennis_jeffrey wrote: > I think a more descriptive comment might be something like this: > > """Wrapper function to convert input data into the desired output format.""" Done.
A few more comments. It's looking good! http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/25001/chrome/test/functional/auto... chrome/test/functional/autofill.py:159: # Autofill server captures 2.5% of the data posted. On 2011/02/16 20:34:15, dhollowa wrote: > On 2011/02/16 19:43:29, dennis_jeffrey wrote: > > Is there any verification we can do to check that the Autofill server is > > receiving the data we're sending in this test? > > > > Not really. The Autofill server processes the data offline, so I can take a few > days for the "result" to be detectable. > > > > Also, is it really necessary to submit the data 1000 times? Based on your > > comment here, it seems that if we only do it 100 times, we'd still expect the > > Autofill server to capture the data in about 2 of those 100 cases. > > The Autofill server use a statistical model to determine field types. Only 2 > samples will not exceed the server's threshold of "noise" so 1000 iterations is > a safe minimum. Ok - thanks for the explanation! http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill.py:195: self.ExecuteJavascript(script, 0, 0) Remove the spaces around the "=" in this line. The python style guide says "Don't use spaces around the '=' sign when used to indicate a keyword argument or a default parameter value." http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill.py:195: self.ExecuteJavascript(script, 0, 0) I think we can remove this comment, since the code is always using "logging.INFO" as the verbosity here (not WARNING or ERROR). But if you'd prefer to leave the comment, add one more space before the comment starts; the style guide says this about inline comments: "To improve legibility, these comments should be at least 2 spaces away from the code." http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... File chrome/test/functional/autofill_dataset_converter.py (right): http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:46: _logger.addHandler(NullHandler()) In the rest of this file, you use "self._logger", so I think this class variable "_logger" is not actually used anywhere, is it? If not, we can remove it (and then also probably the NullHandler too, unless that's used elsewhere). http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:49: error_level = logging.ERROR I think there's no need to define "info_level", "warning_level", or "error_level". We can simply use "logging.INFO", "logging.WARNING", or "logging.ERROR" whenever we need to. For example, in autofill.py, you pass "logging.INFO" to the constructor of this class. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:51: def __init__(self, input_filename, output_filename=None, logging_level=None): Rather than having "logging_level" default to "None", wouldn't it be better to have it default to some valid logging level, perhaps one with less verbosity such as "ERROR"? http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:82: self._logger.setLevel(logging_level) Right now, if the default logging level of "None" is used, then I think you'd get a runtime error the first time you try to log a message because "self._logger" would be undefined (since you only define it if "logging_level" is specified). If you change line 51 above as recommended, then you can remove the "if" statement here at line 78, and then simply do: console.setLevel(logging_level) Finally, I think you can remove the call to "self._logger.setLevel(...)", since you're already setting the verbosity level of the console handler. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:103: } You may want to also mention in the comment here that the method assumes that a valid record always contains at least one "|" character. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:109: A dictionary representing a single record from the dataset file. The method may also potentially return None if the current record line is invalid. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:128: i += 1 There's a cool way in python to iterate through a list while getting the index at the same time. So you can shorten lines 125-128 above to just this instead: for i, key in enumerate(self._fields): out_record[key] = record_list[i] http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:132: """Wrapper function to convert input data into the desired output format.""" This function can return something, so you should have a "Returns:" section in the docstring here. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:132: """Wrapper function to convert input data into the desired output format.""" Since you've removed the "_Convert()" function, this function is no longer a "wrapper" around it. So we can probably remove the word "Wrapper" here in the docstring. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:177: DatasetConverter.info_level) I recommend changing "DatasetConverter.info_level" to just "logging.INFO"
http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill.py:195: logging_level = logging.INFO) # Set verbosity to INFO, WARNING, ERROR. On 2011/02/17 22:58:35, dennis_jeffrey wrote: > Remove the spaces around the "=" in this line. The python style guide says > "Don't use spaces around the '=' sign when used to indicate a keyword argument > or a default parameter value." Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill.py:195: logging_level = logging.INFO) # Set verbosity to INFO, WARNING, ERROR. I prefer to keep the comment for future ref. On 2011/02/17 22:58:35, dennis_jeffrey wrote: > I think we can remove this comment, since the code is always using > "logging.INFO" as the verbosity here (not WARNING or ERROR). But if you'd > prefer to leave the comment, add one more space before the comment starts; the > style guide says this about inline comments: "To improve legibility, these > comments should be at least 2 spaces away from the code." http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... File chrome/test/functional/autofill_dataset_converter.py (right): http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:46: _logger.addHandler(NullHandler()) On 2011/02/17 22:58:35, dennis_jeffrey wrote: > In the rest of this file, you use "self._logger", so I think this class variable > "_logger" is not actually used anywhere, is it? If not, we can remove it (and > then also probably the NullHandler too, unless that's used elsewhere). Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:49: error_level = logging.ERROR On 2011/02/17 22:58:35, dennis_jeffrey wrote: > I think there's no need to define "info_level", "warning_level", or > "error_level". We can simply use "logging.INFO", "logging.WARNING", or > "logging.ERROR" whenever we need to. For example, in autofill.py, you pass > "logging.INFO" to the constructor of this class. Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:51: def __init__(self, input_filename, output_filename=None, logging_level=None): On 2011/02/17 22:58:35, dennis_jeffrey wrote: > Rather than having "logging_level" default to "None", wouldn't it be better to > have it default to some valid logging level, perhaps one with less verbosity > such as "ERROR"? Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:82: self._logger.setLevel(logging_level) On 2011/02/17 22:58:35, dennis_jeffrey wrote: > Right now, if the default logging level of "None" is used, then I think you'd > get a runtime error the first time you try to log a message because > "self._logger" would be undefined (since you only define it if "logging_level" > is specified). > > If you change line 51 above as recommended, then you can remove the "if" > statement here at line 78, and then simply do: > > console.setLevel(logging_level) > > Finally, I think you can remove the call to "self._logger.setLevel(...)", since > you're already setting the verbosity level of the console handler. Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:103: } On 2011/02/17 22:58:35, dennis_jeffrey wrote: > You may want to also mention in the comment here that the method assumes that a > valid record always contains at least one "|" character. Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:109: A dictionary representing a single record from the dataset file. On 2011/02/17 22:58:35, dennis_jeffrey wrote: > The method may also potentially return None if the current record line is > invalid. Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:128: i += 1 On 2011/02/17 22:58:35, dennis_jeffrey wrote: > There's a cool way in python to iterate through a list while getting the index > at the same time. So you can shorten lines 125-128 above to just this instead: > > for i, key in enumerate(self._fields): > out_record[key] = record_list[i] Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:132: """Wrapper function to convert input data into the desired output format.""" On 2011/02/17 22:58:35, dennis_jeffrey wrote: > This function can return something, so you should have a "Returns:" section in > the docstring here. Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:132: """Wrapper function to convert input data into the desired output format.""" On 2011/02/17 22:58:35, dennis_jeffrey wrote: > Since you've removed the "_Convert()" function, this function is no longer a > "wrapper" around it. So we can probably remove the word "Wrapper" here in the > docstring. Done. http://codereview.chromium.org/6246147/diff/25002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_converter.py:177: DatasetConverter.info_level) On 2011/02/17 22:58:35, dennis_jeffrey wrote: > I recommend changing > > "DatasetConverter.info_level" > > to just > > "logging.INFO" Done.
LGTM Thanks for addressing all of my comments!
This has caused the following error on Mac bots
Traceback (most recent call last):
File "../src/chrome/test/functional/pyauto_functional.py", line 51, in
<module>
Main()
File "../src/chrome/test/functional/pyauto_functional.py", line 44, in
__init__
pyauto.Main.__init__(self)
File "../src/chrome/test/functional/../pyautolib/pyauto.py", line 2152, in
__init__
self._Run()
File "../src/chrome/test/functional/../pyautolib/pyauto.py", line 2402, in
_Run
test_names = self._ExpandTestNames(self._args)
File "../src/chrome/test/functional/../pyautolib/pyauto.py", line 2334, in
_ExpandTestNames
self._options.suite)
File "../src/chrome/test/functional/../pyautolib/pyauto.py", line 2376, in
_ExpandTestNamesFrom
args.extend(self._ImportTestsFromName(name))
File "../src/chrome/test/functional/../pyautolib/pyauto.py", line 2252, in
_ImportTestsFromName
module = __import__('.'.join(parts_copy))
File
"/b/build/slave/chrome-mac-10_5-qa/build/src/chrome/test/functional/autofill.py",
line 10, in <module>
import autofill_dataset_converter
File
"/b/build/slave/chrome-mac-10_5-qa/build/src/chrome/test/functional/autofill_dataset_converter.py",
line 128
with open(self._input_filename) as input_file:
This is because the 'with open..' construct does not exist in python2.5 used on
Mac.
Also, looks like you renamed to autofill_dataset_converter but did not change it
where it's used in autofill.py
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
