Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6685077:
Two sets of Autofill tests.... (Closed)
|
Created:
9 years, 9 months ago by dyu1 Modified:
9 years, 7 months ago CC:
chromium-reviews, John Grabowski, anantha, Paweł Hajdan Jr. Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionTwo sets of Autofill tests.
1. testComparePhoneNumbers - Test phone fields parses correctly from a given profile.
Contains files: autofill.py, form_phones.html, phone_pinput_autofill.txt, phone_pexpected_autofill.txt, pyauto.py
2. FormFillLatencyAfterSubmit -Test latency time on form submit with lots of stored Autofill profiles.
Contains files: autofill.py, autofill_dataset_generator.py, latency_after_submit_test.html, pyauto.py
TEST=none
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79763
Patch Set 1 #
Total comments: 52
Patch Set 2 : '' #
Total comments: 104
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 2
Patch Set 7 : '' #
Total comments: 58
Patch Set 8 : '' #
Total comments: 16
Patch Set 9 : '' #
Total comments: 2
Patch Set 10 : '' #
Total comments: 10
Patch Set 11 : '' #
Total comments: 4
Patch Set 12 : '' #Messages
Total messages: 26 (0 generated)
I'm reviewing only pyauto.py http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:757: def SendKeyEvent(self, key_code, windex=0, tab_index=0): s/SendKeyEvent/SendWebkitKeyEvent/ to distinguish it from being a generic key event. (I'm assuming this will work only in the renderer portion, not on chrome UI elements.) http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:757: def SendKeyEvent(self, key_code, windex=0, tab_index=0): Put tab_index arg before windex http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:758: """Send a key event to the browser. Update this description accordingly http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:769: 'type': 0, # kRawKeyDownType need 2 spaces before # http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:780: cmd_dict['type'] = 3 # kKeyUpType need at least 2 spaces before # http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:780: cmd_dict['type'] = 3 # kKeyUpType modifying cmd_dict after calling _GetResultFromJSONRequest() has no use
http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:757: def SendKeyEvent(self, key_code, windex=0, tab_index=0): On 2011/03/18 03:53:29, Nirnimesh wrote: > Put tab_index arg before windex Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:757: def SendKeyEvent(self, key_code, windex=0, tab_index=0): On 2011/03/18 03:53:29, Nirnimesh wrote: > s/SendKeyEvent/SendWebkitKeyEvent/ > > to distinguish it from being a generic key event. > > (I'm assuming this will work only in the renderer portion, not on chrome UI > elements.) Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:758: """Send a key event to the browser. On 2011/03/18 03:53:29, Nirnimesh wrote: > Update this description accordingly Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:769: 'type': 0, # kRawKeyDownType On 2011/03/18 03:53:29, Nirnimesh wrote: > need 2 spaces before # Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:780: cmd_dict['type'] = 3 # kKeyUpType On 2011/03/18 03:53:29, Nirnimesh wrote: > need at least 2 spaces before # Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:780: cmd_dict['type'] = 3 # kKeyUpType On 2011/03/18 03:53:29, Nirnimesh wrote: > modifying cmd_dict after calling _GetResultFromJSONRequest() has no use I actually need both since the cmd_dict is two separate events, one is calling key up and one is calling key down.
In general, the style here looks pretty good! I think your code quality is improving :-) http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... File chrome/test/data/autofill/form_phones.html (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:5: <title>Autofill Phone fields test form</title> "Phone" --> "phone"? http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:22: <!-- Basic PHONE field --> I recommend all comments to be complete sentences, including a period at the end (here and elsewhere in this CL). http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:26: <!-- Set of phone fields with area code, and phone --> Remove the comma in this sentence. Also change "phone" to "phone number". http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:58: <!-- Set of fax fields with area code, and fax number --> Remove the comma in this sentence. http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... File chrome/test/data/autofill/phone_pexpected_autofill.txt (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pexpected_autofill.txt:1: # Expected Profile for comparing phone numbers Autofill test. The name of this file contains the substring "pexpected". Should the initial "p" be there? http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pexpected_autofill.txt:1: # Expected Profile for comparing phone numbers Autofill test. "Profile" --> "profile". "numbers Autofill" --> "numbers in Autofill". http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... File chrome/test/data/autofill/phone_pinput_autofill.txt (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pinput_autofill.txt:1: # Input Profile for comparing phone numbers Autofill test. Same comments as for the "phone_pexpected_autofill.txt" file. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:161: return_keypress=0x0D I think the above 3 lines are better declared as named constants. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:167: self.SendKeyEvent(tab_keypress, windex=0, tab_index=0) Could you add a comment to explain what you're doing with the next 4 key events? http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:171: raw_input() This line shouldn't be here in an automated test, right? http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:183: \nExpected: "%s"\nReturned: "%s"' % (key, value, form_values[key])) Rather than using "\" to continue the string onto the next line, I recommend enclosing the string in parens and then using implicit line joining: ('Original profile not equal to expected profile at key: "%s"\n' 'Expected: "%s"\nReturned: "%s"' % (key, value, form_values[key])) http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:185: def FormFillLatencyAfterSubmit(self): Shouldn't this function name begin with the substring "test"? Or is this because the test is only partially automated and you don't want it to execute along with all the other automated tests? http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:188: This test verifies when a profile is selected from the Autofill dictionary, Remove the comma at the end of this line. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:199: list_of_dict = gen.GenerateDataset(num_of_records_to_generate=50) Does this mean you're generating 50 profiles? The comment for this test says that it should involve "thousands" of profiles. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:203: return_keypress=0x0D Perhaps better as constants defined at the top of this file. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:205: self.SendKeyEvent(tab_keypress, windex=0, tab_index=0) Add a comment to describe what this sequence of keypresses is doing. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:209: # Requires manual intervention to test the performance time after submitted "submitted" --> "submitting" http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:211: raw_input() Is it considered acceptable to check in PyAuto tests that require manual intervention to do the verification? If so, that's fine, but then let's make it clear somehow. We should maybe label such tests with the suffix "_MANUAL", or at least make this clear in the comment above that describes this function/test. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:260: Add one more blank line before the start of the main() function, since this function is not inside the above class. http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:780: cmd_dict['type'] = 3 # kKeyUpType On 2011/03/18 03:53:29, Nirnimesh wrote: > modifying cmd_dict after calling _GetResultFromJSONRequest() has no use Unless I'm mistaken, I think David's actually sending two requests: one each for "key down" and "key up". David, if this is true, maybe you can make this more clear in a comment. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:18: import os "import os" should come between "logging" and "random". http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:51: ] Should we have more choices for cities, states, and zip codes to allow for more varied random outputs? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:53: re_single_quote = re.compile("'", re.UNICODE) Prefer single quotes for strings: '\'' http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:64: If we want the value to always be the same .e.g. u'John' we can use this ".e.g." --> "e.g." http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:65: instead a a method. We can even use None keyword which will give "a a method" --> "of a method" http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:111: self.output_pattern = u"{" Prefer single quotes over double quotes in strings. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:113: self.output_pattern += u"u'%s': u'%s', " %(key_and_method[0], "%s") Prefer single quotes over double quotes in strings. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:114: self.output_pattern = self.output_pattern[:-1] + "}," Prefer single quotes over double quotes in strings. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:122: The rest values are the args. If function is None then arg is just "rest values" --> "remaining values" http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:141: parts.append(u'%s' %function(*args)) Put a space after the '%' operator. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:142: return (' ').join(parts) I think you can remove the parens around the ' ' http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:180: """Generates Numerical First Names. "Numerical First Names" --> "a numerical first name" http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:182: Its Name is the number of the current dict. "Its Name" --> "The name" http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:185: Returns random first names. "random first names" --> "a random first name" http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:190: """Generates emails that correspond to the First Name. "emails that correspond to the First Name" --> "an email that corresponds to the first name" http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:194: Returns random email addresses. "random email addresses" --> "a random email address" http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:202: It first increments zero starting dict_no. Probably don't need to mention this in the function docstring. Seems like more of an implementation detail. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:204: Returns the output dictionary. Returns: The output dictionary. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:216: r"\'", output_dict[key]) # escaping single quote: "'" -> "\'" Put one more space before the "#" that starts the comment. Also start the comment like this: # Escaping single quote... http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:216: r"\'", output_dict[key]) # escaping single quote: "'" -> "\'" Prefer single quotes to double quotes in strings. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:230: self.output_filename, mode = 'wb', encoding = 'utf-8-sig') Remove the spaces around the two "=" in this line. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:242: output_line = self.output_pattern %tuple( Put a space after the "%" http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:248: "%d: %s" %(self.dict_no, output_line.encode(sys.stdout.encoding, Put a space after the "%" http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:248: "%d: %s" %(self.dict_no, output_line.encode(sys.stdout.encoding, Prefer single quotes to double quotes in strings. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:255: self.logger.info("--- FINISHED ---") Prefer single quotes to double quotes in strings (in the above 2 lines). http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:263: from optparse import OptionParser This should be up with the rest of the imports at the top of this file. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:266: dest="output_filename", default="", This should be indented to line up underneath the other parameters in the function call parameter list (here and in the other lines below): parser.add_option("-o", "--output", dest="output_filename", default="", http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:269: default=True, Default should probably be False if the action is to store_true. Otherwise this option will always be true. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:272: help="display nothing") What's the default here? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:275: metavar="LOG_LEVEL") Prefer single quotes to double quotes in strings (for lines 265-275). http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:282: options.logging_level = None This doesn't seem to make sense. Why would we clear the "logging_level" option if the "verbose" option is not true? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:284: options.logging_level = 'info' "Verbose" usually means the most logging messages. Wouldn't that be the "debug" level? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:296: gen.GenerateDataset(100) Maybe this value "100" should be configurable with a command-line option.
http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/form_... File chrome/test/data/autofill/form_phones.html (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:1: <!DOCTYPE html> nit: Let's make a py/ subdirectory for python test data -- or more focused directories, if those would make more sense. http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/laten... File chrome/test/data/autofill/latency_after_submit_test.html (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/laten... chrome/test/data/autofill/latency_after_submit_test.html:8: <p> nit: The <p> element does not make sense here; please remove it. http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/phone... File chrome/test/data/autofill/phone_pexpected_autofill.txt (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pexpected_autofill.txt:2: # Used by: chrome/test/functional/autofill.py testComparePhoneNumbers Curious: What's the motivation for making this a plain text file rather than a .py file that exports a constant? http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pexpected_autofill.txt:10: 'PHONE_FAX_COUNTRY_CODE-1': '1',}, nit: As long as you're wrapping the lines, please wrap them to 80-col. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:161: return_keypress=0x0D Are these constants really not defined in the global pyauto test infrastructure anywhere? :( http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:197: logging_level=logging.ERROR) # Set verbosity to INFO, WARNING, ERROR. nit: I think this comment ("Set verbosity...") is more confusing than helpful. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:198: list_of_dict = gen.GenerateDataset(num_of_records_to_generate=50) Why 50 and not more like 1000? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:210: raw_input() Why do we need manual intervention? It would be great to have this test run in a fully automated fashion. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:34: [ random.randint, 1, 10000], Do we really need randomness? Deterministic tests are generally preferable, as the results are more reproducible. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:53: re_single_quote = re.compile("'", re.UNICODE) On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Prefer single quotes for strings: > > '\'' Curious: What's the motivation for this? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:68: 'output_pattern' for one field would have been: "{u'NAME_FIRST': u'%s',}" I don't understand why there is both a trailing comma and a trailing curly brace in this example. Might be worth clarifying that in the comment. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:114: self.output_pattern = self.output_pattern[:-1] + "}," This line is confusing -- why are we trimming a space of the end of the string? Please add a comment if that's really what you mean to do. http://codereview.chromium.org/6685077/diff/9/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:757: def SendWebkitKeyEvent(self, key_code, tab_index=0, windex=0): I'd be very surprised if this functionality is not already available in pyauto. How do other tests send key events?
http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:53: re_single_quote = re.compile("'", re.UNICODE) On 2011/03/18 23:17:11, Ilya Sherman wrote: > On 2011/03/18 21:44:01, dennis_jeffrey wrote: > > Prefer single quotes for strings: > > > > '\'' > > Curious: What's the motivation for this? Consistency; most strings I've seen in python test code are specified with single quotes, and I've seen other code reviewers try to keep it that way too. I'm curious to hear and consider different opinions in case anyone has them :-)
http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:53: re_single_quote = re.compile("'", re.UNICODE) On 2011/03/18 23:25:39, dennis_jeffrey wrote: > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > On 2011/03/18 21:44:01, dennis_jeffrey wrote: > > > Prefer single quotes for strings: > > > > > > '\'' > > > > Curious: What's the motivation for this? > > Consistency; most strings I've seen in python test code are specified with > single quotes, and I've seen other code reviewers try to keep it that way too. > I'm curious to hear and consider different opinions in case anyone has them :-) In general, I too prefer single quotes for consistency; but in this case I'd personally have chosen double quotes to avoid escaping the inner quote. Haven't worked with our python codebase very much though -- thus the question :)
http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:161: return_keypress=0x0D On 2011/03/18 21:44:01, dennis_jeffrey wrote: > I think the above 3 lines are better declared as named constants. Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:167: self.SendKeyEvent(tab_keypress, windex=0, tab_index=0) On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Could you add a comment to explain what you're doing with the next 4 key events? I thought the named variables was self explanatory but I'll add the comments you suggested. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:171: raw_input() On 2011/03/18 21:44:01, dennis_jeffrey wrote: > This line shouldn't be here in an automated test, right? Left it in for debugging but removed it in my last revision. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:183: \nExpected: "%s"\nReturned: "%s"' % (key, value, form_values[key])) On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Rather than using "\" to continue the string onto the next line, I recommend > enclosing the string in parens and then using implicit line joining: > > ('Original profile not equal to expected profile at key: "%s"\n' > 'Expected: "%s"\nReturned: "%s"' % (key, value, form_values[key])) Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:185: def FormFillLatencyAfterSubmit(self): On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Shouldn't this function name begin with the substring "test"? Or is this > because the test is only partially automated and you don't want it to execute > along with all the other automated tests? Since this test is only partially automated I didn't name it test. All tests that begin with "tests" are automatically run in the continuous build, so this is treated more as a helper function. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:188: This test verifies when a profile is selected from the Autofill dictionary, On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Remove the comma at the end of this line. Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:199: list_of_dict = gen.GenerateDataset(num_of_records_to_generate=50) On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Does this mean you're generating 50 profiles? The comment for this test says > that it should involve "thousands" of profiles. Only left 50 for testing purposes. Will change to 1000 when I actually commit. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:203: return_keypress=0x0D On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Perhaps better as constants defined at the top of this file. Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:205: self.SendKeyEvent(tab_keypress, windex=0, tab_index=0) On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Add a comment to describe what this sequence of keypresses is doing. Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:209: # Requires manual intervention to test the performance time after submitted On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "submitted" --> "submitting" Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:211: raw_input() On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Is it considered acceptable to check in PyAuto tests that require manual > intervention to do the verification? If so, that's fine, but then let's make it > clear somehow. We should maybe label such tests with the suffix "_MANUAL", or > at least make this clear in the comment above that describes this function/test. Yes, should be fine as Nir approved of two other tests that requires manual intervention. These tests do the bulk of the work that would be tedious to do manually. The final steps missing are verification steps that can't be done with what's available. http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:780: cmd_dict['type'] = 3 # kKeyUpType On 2011/03/18 21:44:01, dennis_jeffrey wrote: > On 2011/03/18 03:53:29, Nirnimesh wrote: > > modifying cmd_dict after calling _GetResultFromJSONRequest() has no use > > Unless I'm mistaken, I think David's actually sending two requests: one each for > "key down" and "key up". David, if this is true, maybe you can make this more > clear in a comment. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/form_... File chrome/test/data/autofill/form_phones.html (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:1: <!DOCTYPE html> On 2011/03/18 23:17:11, Ilya Sherman wrote: > nit: Let's make a py/ subdirectory for python test data -- or more focused > directories, if those would make more sense. This is actually where all the data files fall under within chrome/test/data/_project_/ http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/laten... File chrome/test/data/autofill/latency_after_submit_test.html (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/laten... chrome/test/data/autofill/latency_after_submit_test.html:8: <p> On 2011/03/18 23:17:11, Ilya Sherman wrote: > nit: The <p> element does not make sense here; please remove it. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/phone... File chrome/test/data/autofill/phone_pexpected_autofill.txt (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pexpected_autofill.txt:2: # Used by: chrome/test/functional/autofill.py testComparePhoneNumbers On 2011/03/18 23:17:11, Ilya Sherman wrote: > Curious: What's the motivation for making this a plain text file rather than a > .py file that exports a constant? I plan to continue to add dictionaries which also include phone numbers from other countries that may have longer country codes. Most of the dictionaries within autofill.py contains smaller subsets. http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pexpected_autofill.txt:10: 'PHONE_FAX_COUNTRY_CODE-1': '1',}, On 2011/03/18 23:17:11, Ilya Sherman wrote: > nit: As long as you're wrapping the lines, please wrap them to 80-col. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:161: return_keypress=0x0D On 2011/03/18 23:17:11, Ilya Sherman wrote: > Are these constants really not defined in the global pyauto test infrastructure > anywhere? :( PyAUto was never meant to interact with the browser UI such as send key events to the browser. This is just a temp solution for now so I can execute this test until we get Chrome Driver linked with pyAuto. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:197: logging_level=logging.ERROR) # Set verbosity to INFO, WARNING, ERROR. On 2011/03/18 23:17:11, Ilya Sherman wrote: > nit: I think this comment ("Set verbosity...") is more confusing than helpful. Removed. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:198: list_of_dict = gen.GenerateDataset(num_of_records_to_generate=50) On 2011/03/18 23:17:11, Ilya Sherman wrote: > Why 50 and not more like 1000? I plan to change this to 1000 after I pass code review. This is more for testing levels while this CL is still in code review. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:210: raw_input() On 2011/03/18 23:17:11, Ilya Sherman wrote: > Why do we need manual intervention? It would be great to have this test run in > a fully automated fashion. There are no hooks exposed in pyauto to measure time against a set baseline unless this test is written in C++ and uses the performance functions from the chrome code base. (UIPerfTest). Plus it would be hard to measure time in pyauto since it's being run on the bots and the inconsistent time measurement could be caused by something different than a hang. I'm not sure writing a helper function to measure time would be worth the effort. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:18: import os On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "import os" should come between "logging" and "random". Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:34: [ random.randint, 1, 10000], On 2011/03/18 23:17:11, Ilya Sherman wrote: > Do we really need randomness? Deterministic tests are generally preferable, as > the results are more reproducible. This creates what you recommended... {u'NAME_FIRST': u'1', u'EMAIL_ADDRESS': u'1@example.com', ....} {u'NAME_FIRST': u'2', u'EMAIL_ADDRESS': u'2@example.com', ....} http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:51: ] On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Should we have more choices for cities, states, and zip codes to allow for more > varied random outputs? I don't think it makes a difference since the minimum requirements for a valid Autofill profile has been met. All the cities listed are in CA anyways. I might add corresponding zip codes to the associated city. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:53: re_single_quote = re.compile("'", re.UNICODE) On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Prefer single quotes for strings: > > '\'' Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:64: If we want the value to always be the same .e.g. u'John' we can use this On 2011/03/18 21:44:01, dennis_jeffrey wrote: > ".e.g." --> "e.g." Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:65: instead a a method. We can even use None keyword which will give On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "a a method" --> "of a method" Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:68: 'output_pattern' for one field would have been: "{u'NAME_FIRST': u'%s',}" On 2011/03/18 23:17:11, Ilya Sherman wrote: > I don't understand why there is both a trailing comma and a trailing curly brace > in this example. Might be worth clarifying that in the comment. I thought the trailing ,} is required for the last elements in a dictionary? At least that's what I see for all the dictionaries we have in our data directory. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:111: self.output_pattern = u"{" On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Prefer single quotes over double quotes in strings. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:113: self.output_pattern += u"u'%s': u'%s', " %(key_and_method[0], "%s") On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Prefer single quotes over double quotes in strings. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:114: self.output_pattern = self.output_pattern[:-1] + "}," On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Prefer single quotes over double quotes in strings. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:114: self.output_pattern = self.output_pattern[:-1] + "}," On 2011/03/18 23:17:11, Ilya Sherman wrote: > This line is confusing -- why are we trimming a space of the end of the string? > Please add a comment if that's really what you mean to do. This is because in the previous "for key_and_method in self.fields:" the "self.output_pattern += u"u'%s': u'%s', " %(key_and_method[0], "%s")" adds parts like this: "u'EMAIL_ADDRESS': u'%s', " There is a space before the last quote there for this is used to trim the space. How do you suggest I add a comment here? Should I say "Delete the space created in the previous pattern"? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:122: The rest values are the args. If function is None then arg is just On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "rest values" --> "remaining values" Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:141: parts.append(u'%s' %function(*args)) On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Put a space after the '%' operator. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:142: return (' ').join(parts) On 2011/03/18 21:44:01, dennis_jeffrey wrote: > I think you can remove the parens around the ' ' Seems weird to me without it. Does it hurt to leave it? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:180: """Generates Numerical First Names. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "Numerical First Names" --> "a numerical first name" Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:182: Its Name is the number of the current dict. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "Its Name" --> "The name" Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:185: Returns random first names. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "random first names" --> "a random first name" Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:190: """Generates emails that correspond to the First Name. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "emails that correspond to the First Name" --> > "an email that corresponds to the first name" Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:194: Returns random email addresses. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "random email addresses" --> "a random email address" Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:202: It first increments zero starting dict_no. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Probably don't need to mention this in the function docstring. Seems like more > of an implementation detail. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:204: Returns the output dictionary. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Returns: > The output dictionary. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:216: r"\'", output_dict[key]) # escaping single quote: "'" -> "\'" On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Put one more space before the "#" that starts the comment. Also start the > comment like this: > > # Escaping single quote... Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:216: r"\'", output_dict[key]) # escaping single quote: "'" -> "\'" On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Prefer single quotes to double quotes in strings. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:230: self.output_filename, mode = 'wb', encoding = 'utf-8-sig') On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Remove the spaces around the two "=" in this line. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:242: output_line = self.output_pattern %tuple( On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Put a space after the "%" Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:255: self.logger.info("--- FINISHED ---") On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Prefer single quotes to double quotes in strings (in the above 2 lines). Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:263: from optparse import OptionParser On 2011/03/18 21:44:01, dennis_jeffrey wrote: > This should be up with the rest of the imports at the top of this file. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:266: dest="output_filename", default="", On 2011/03/18 21:44:01, dennis_jeffrey wrote: > This should be indented to line up underneath the other parameters in the > function call parameter list (here and in the other lines below): > > parser.add_option("-o", "--output", > dest="output_filename", default="", Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:269: default=True, On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Default should probably be False if the action is to store_true. Otherwise this > option will always be true. The default should be True since this is the default behavior of the script. The script is verbose by default. Do you think I should just remove the options for -v and -q? I think one of them is no use anyways as it will be covered with -l LOG_LEVEL. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:272: help="display nothing") On 2011/03/18 21:44:01, dennis_jeffrey wrote: > What's the default here? The default is True since both options -v and -q use the same variable for the state. Both use dest="verbose". I'll probably just remove both of these options as it's covered by the -l LOG_LEVEL option anyways. thoughts? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:275: metavar="LOG_LEVEL") On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Prefer single quotes to double quotes in strings (for lines 265-275). Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:282: options.logging_level = None On 2011/03/18 21:44:01, dennis_jeffrey wrote: > This doesn't seem to make sense. Why would we clear the "logging_level" option > if the "verbose" option is not true? Deleted http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:284: options.logging_level = 'info' On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "Verbose" usually means the most logging messages. Wouldn't that be the "debug" > level? Deleted. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:296: gen.GenerateDataset(100) On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Maybe this value "100" should be configurable with a command-line option. Done. http://codereview.chromium.org/6685077/diff/9/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/pyautolib/pyauto.py... chrome/test/pyautolib/pyauto.py:757: def SendWebkitKeyEvent(self, key_code, tab_index=0, windex=0): On 2011/03/18 23:17:11, Ilya Sherman wrote: > I'd be very surprised if this functionality is not already available in pyauto. > How do other tests send key events? PyAuto was never meant to interact with the web UI such as send key events. These are the first set of tests that actually does so. Once pyauto is integrated with chrome driver, we'll be able to get away from this.
http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/form_... File chrome/test/data/autofill/form_phones.html (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:1: <!DOCTYPE html> On 2011/03/21 18:42:35, dyu1 wrote: > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > nit: Let's make a py/ subdirectory for python test data -- or more focused > > directories, if those would make more sense. > > This is actually where all the data files fall under within > chrome/test/data/_project_/ In the case of autofill, we have other subdirectories for non-pyauto test files. Unless these are going to be used with pyauto as well, I think it makes sense to move the pyauto-specific files into their own subdir. Nirnimesh -- any thoughts? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:198: list_of_dict = gen.GenerateDataset(num_of_records_to_generate=50) On 2011/03/21 18:42:35, dyu1 wrote: > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > Why 50 and not more like 1000? > > I plan to change this to 1000 after I pass code review. This is more for testing > levels while this CL is still in code review. Hmm... generally the idea of code review is to review the code that will be committed, rather than code ~similar~ to what will be committed. What makes 50 more appropriate as a temporary value? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:34: [ random.randint, 1, 10000], On 2011/03/21 18:42:35, dyu1 wrote: > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > Do we really need randomness? Deterministic tests are generally preferable, > as > > the results are more reproducible. > > This creates what you recommended... > > {u'NAME_FIRST': u'1', u'EMAIL_ADDRESS': u'1@example.com', ....} > {u'NAME_FIRST': u'2', u'EMAIL_ADDRESS': u'2@example.com', ....} > So, what's the purpose of the random.randint then? http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:51: ] On 2011/03/21 18:42:35, dyu1 wrote: > On 2011/03/18 21:44:01, dennis_jeffrey wrote: > > Should we have more choices for cities, states, and zip codes to allow for > more > > varied random outputs? > > > I don't think it makes a difference since the minimum requirements for a valid > Autofill profile has been met. All the cities listed are in CA anyways. I might > add corresponding zip codes to the associated city. In fact, it's fine to have the whole profile just be e.g. {u'NAME_FIRST': u'1'} for this particular test -- with no additional address data. The minimum requirements for a profile only apply when that profile is being learned implicitly via form submission. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:68: 'output_pattern' for one field would have been: "{u'NAME_FIRST': u'%s',}" On 2011/03/21 18:42:35, dyu1 wrote: > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > I don't understand why there is both a trailing comma and a trailing curly > brace > > in this example. Might be worth clarifying that in the comment. > > I thought the trailing ,} is required for the last elements in a dictionary? At > least that's what I see for all the dictionaries we have in our data directory. Both the comma and the whitespace are optional. I would recommend trimming either both or neither (probably neither, since that's simpler). http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:114: self.output_pattern = self.output_pattern[:-1] + "}," On 2011/03/21 18:42:35, dyu1 wrote: > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > This line is confusing -- why are we trimming a space of the end of the > string? > > Please add a comment if that's really what you mean to do. > > > This is because in the previous "for key_and_method in self.fields:" the > "self.output_pattern += u"u'%s': u'%s', " %(key_and_method[0], "%s")" adds parts > like this: "u'EMAIL_ADDRESS': u'%s', " > > There is a space before the last quote there for this is used to trim the space. > How do you suggest I add a comment here? Should I say "Delete the space created > in the previous pattern"? It's not clear to me why the space matters, and why we want to trim it. So I would appreciate a comment like "Trim the trailing space, because [insert reason here]."
http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill.py:198: list_of_dict = gen.GenerateDataset(num_of_records_to_generate=50) On 2011/03/22 01:18:58, Ilya Sherman wrote: > On 2011/03/21 18:42:35, dyu1 wrote: > > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > > Why 50 and not more like 1000? > > > > I plan to change this to 1000 after I pass code review. This is more for > testing > > levels while this CL is still in code review. > > Hmm... generally the idea of code review is to review the code that will be > committed, rather than code ~similar~ to what will be committed. What makes 50 > more appropriate as a temporary value? I changed it to the value that was being reported from crbug/74911. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:34: [ random.randint, 1, 10000], On 2011/03/22 01:18:58, Ilya Sherman wrote: > On 2011/03/21 18:42:35, dyu1 wrote: > > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > > Do we really need randomness? Deterministic tests are generally preferable, > > as > > > the results are more reproducible. > > > > This creates what you recommended... > > > > {u'NAME_FIRST': u'1', u'EMAIL_ADDRESS': u'1@example.com', ....} > > {u'NAME_FIRST': u'2', u'EMAIL_ADDRESS': u'2@example.com', ....} > > > > So, what's the purpose of the random.randint then? Purpose of [random.randint, 1, 10000] is to generate street address numbers. Without this the address will be "foobar Ln #3" or "foobar St #1", etc. With the random.randint the addresses will have digits associated with it. I thought that would be more realistic. Down the road when Autofill begins performing verification checks for street addresses, the ones without a street address could be rejected. http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:114: self.output_pattern = self.output_pattern[:-1] + "}," On 2011/03/22 01:18:58, Ilya Sherman wrote: > On 2011/03/21 18:42:35, dyu1 wrote: > > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > > This line is confusing -- why are we trimming a space of the end of the > > string? > > > Please add a comment if that's really what you mean to do. > > > > > > This is because in the previous "for key_and_method in self.fields:" the > > "self.output_pattern += u"u'%s': u'%s', " %(key_and_method[0], "%s")" adds > parts > > like this: "u'EMAIL_ADDRESS': u'%s', " > > > > There is a space before the last quote there for this is used to trim the > space. > > How do you suggest I add a comment here? Should I say "Delete the space > created > > in the previous pattern"? > > It's not clear to me why the space matters, and why we want to trim it. So I > would appreciate a comment like "Trim the trailing space, because [insert reason > here]." This is more for aesthetic reasons to remove unneeded whitespace. Such as {u"NAME_FIRST": u"9",}, versus {u"NAME_FIRST": u"9", },
http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:34: [ random.randint, 1, 10000], On 2011/03/22 02:52:35, dyu1 wrote: > On 2011/03/22 01:18:58, Ilya Sherman wrote: > > On 2011/03/21 18:42:35, dyu1 wrote: > > > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > > > Do we really need randomness? Deterministic tests are generally > preferable, > > > as > > > > the results are more reproducible. > > > > > > This creates what you recommended... > > > > > > {u'NAME_FIRST': u'1', u'EMAIL_ADDRESS': u'1@example.com', ....} > > > {u'NAME_FIRST': u'2', u'EMAIL_ADDRESS': u'2@example.com', ....} > > > > > > > So, what's the purpose of the random.randint then? > > Purpose of [random.randint, 1, 10000] is to generate street address numbers. > Without this the address will be "foobar Ln #3" or "foobar St #1", etc. With the > random.randint the addresses will have digits associated with it. I thought that > would be more realistic. Down the road when Autofill begins performing > verification checks for street addresses, the ones without a street address > could be rejected. We have no plans to validate user-entered addresses (i.e. ones added via the prefs). That said, I'm not really opposed to having nicely formatted addresses; but we should do it without randomness. If you're really attached to using python's 'random' class, please use 'random.seed' at the beginning of the test, so that the results are consistently reproducible. http://codereview.chromium.org/6685077/diff/9004/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/9004/chrome/test/functional/autof... chrome/test/functional/autofill.py:2: # Copyright (c) 2010 The Chromium Authors. All rights reserved. nit: Please don't reverse copyright updates ;) http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:19: class AutoFillTest(pyauto.PyUITest): nit: This should be "AutofillTest" -- see http://crbug.com/72758 http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:8: Used to test autofill.AutoFillTest.FormFillLatencyAfterSubmit. nit: AutoFill -> Autofill http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:114: self.output_pattern += u'u"%s": u"%s", ' % (key_and_method[0], "%s") nit: How about """ self.output_pattern += u'u"%s": u"%%s", ' % key_and_method[0] """ ? Also, it's probably better to use """ ', '.join() """ rather than "+=". http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:144: parts.append(u'%s' % function(*args)) nit: Why not just """ parts.append(function(*args)) """? http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:194: A random first name. nit: This does not return a random value, right? This comment is probably what confused me earlier about how you were generating values. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:204: A random email address. nit: Again, not actually random. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:225: r'\'', output_dict[key]) # Escaping single quote: "'" -> '\'' nit: I'm fairly sure this line is a no-op. Can you give me an example of when that would not be the case? http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:253: [output_dict[key_and_method[0]] for key_and_method in self.fields]) nit: How about """ [output_dict[key] for [key, method] in self.fields] """ ?
Some more comments below. Also, I had some comments in patch set 1 from the following files which I don't think have any reply: form_phones.html phone_pexpected_autofill.txt phone_pinput_autofill.txt autofill_dataset_generator.py Could you also make sure those comments are addressed? Thanks! http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:142: return (' ').join(parts) On 2011/03/21 18:42:35, dyu1 wrote: > On 2011/03/18 21:44:01, dennis_jeffrey wrote: > > I think you can remove the parens around the ' ' > > Seems weird to me without it. Does it hurt to leave it? No, I don't think it hurts except for requiring two extra characters :-) http://codereview.chromium.org/6685077/diff/9/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:269: default=True, On 2011/03/21 18:42:35, dyu1 wrote: > On 2011/03/18 21:44:01, dennis_jeffrey wrote: > > Default should probably be False if the action is to store_true. Otherwise > this > > option will always be true. > > The default should be True since this is the default behavior of the script. The > script is verbose by default. Do you think I should just remove the options for > -v and -q? I think one of them is no use anyways as it will be covered with -l > LOG_LEVEL. Yes, I recommend removing the -v and -q options and using only the -l option. Then you can set its default to the most verbose level, which is "debug". http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/fo... File chrome/test/data/autofill/form_phones.html (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/fo... chrome/test/data/autofill/form_phones.html:22: <!-- Basic PHONE field --> Add a period at the end of all comment sentences in this file. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/fo... chrome/test/data/autofill/form_phones.html:26: <!-- Set of phone fields with area code, and phone --> Remove the comma in this sentence. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/fo... chrome/test/data/autofill/form_phones.html:58: <!-- Set of fax fields with area code, and fax number --> Remove the comma in this sentence. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... File chrome/test/data/autofill/phone_pexpected_autofill.txt (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... chrome/test/data/autofill/phone_pexpected_autofill.txt:1: # Expected Profile for comparing phone numbers Autofill test. "Profile" --> "profile". http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... chrome/test/data/autofill/phone_pexpected_autofill.txt:1: # Expected Profile for comparing phone numbers Autofill test. "numbers Autofill test" --> "numbers in Autofill tests" http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... File chrome/test/data/autofill/phone_pinput_autofill.txt (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... chrome/test/data/autofill/phone_pinput_autofill.txt:1: # Input Profile for comparing phone numbers Autofill test. "Profile" --> "profile" http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... chrome/test/data/autofill/phone_pinput_autofill.txt:1: # Input Profile for comparing phone numbers Autofill test. "numbers Autofill test" --> "numbers in Autofill tests" http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:155: """Test phone fields parses correctly from a given profile.""" "fields parses" --> "fields parse" http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:173: # Return keyboard key press. Sorry I wasn't clear before: when I asked for comments about what these keypresses are doing, I meant for the comments to be at more of a high level to describe what's going on from the user's point of view when you send this sequence of key presses. For example, something like this: "Select the first text field, invoke the autofill dropdown, and select one of the profiles to cause the form to be populated". http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:193: submitted. Should probably also mention here that this test is only partially automated. You may also want to add a TODO to make this fully automated later if/when the necessary functionality is available. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:202: list_of_dict = gen.GenerateDataset(num_of_dict_to_generate=1501) 1501 is a bit of a weird number! Just curious if this number was selected arbitrarily, or if there's some special significance to this number? http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:211: # Return keyboard key press. Similar comment as the one above at line 173. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:111: self.dict_length = len(self.fields) This variable doesn't seem to be used in this file anywhere. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:164: """Uses _GenerateField() and state_construct to generate a state. "a state" --> "a random state" http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:172: """Uses _GenerateField() and zip_construct to generate a zip code. "a zip" --> "a random zip" http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:180: """Uses _GenerateField() and country_construct to generate a country. "a country" --> "a random country" http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:291: parser.error('Wrong log_level argument.') May also want to do "parser.print_help()" here, so users can see what the expected log_level arguments are. http://codereview.chromium.org/6685077/diff/7003/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:757: def SendWebkitKeyEvent(self, key_code, tab_index=0, windex=0): For consistency in naming variables, I recommend that you change "tab_index" and "windex" to either: (1) "tab_index" and "window_index" or (2) "tindex" and "windex" or (3) "t_index" and "w_index" http://codereview.chromium.org/6685077/diff/7003/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:765: tab_index: tab index to work on. Defaults to 0 (first tab) Swap the descriptions for the "windex" and "tab_index" parameters to match the order they appear in the parameter list at line 757 above.
http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... File chrome/test/data/autofill/form_phones.html (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:5: <title>Autofill Phone fields test form</title> On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "Phone" --> "phone"? Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:22: <!-- Basic PHONE field --> On 2011/03/18 21:44:01, dennis_jeffrey wrote: > I recommend all comments to be complete sentences, including a period at the end > (here and elsewhere in this CL). These comments are more for myself to quickly see which section is which. http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:26: <!-- Set of phone fields with area code, and phone --> On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Remove the comma in this sentence. Also change "phone" to "phone number". Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:58: <!-- Set of fax fields with area code, and fax number --> On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Remove the comma in this sentence. Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... File chrome/test/data/autofill/phone_pexpected_autofill.txt (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pexpected_autofill.txt:1: # Expected Profile for comparing phone numbers Autofill test. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > The name of this file contains the substring "pexpected". Should the initial > "p" be there? yes, since the p stands for profile. Typing it all out just makes the file name longer. http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pexpected_autofill.txt:1: # Expected Profile for comparing phone numbers Autofill test. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > "Profile" --> "profile". > > "numbers Autofill" --> "numbers in Autofill". Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... File chrome/test/data/autofill/phone_pinput_autofill.txt (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/data/autofill/phone... chrome/test/data/autofill/phone_pinput_autofill.txt:1: # Input Profile for comparing phone numbers Autofill test. On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Same comments as for the "phone_pexpected_autofill.txt" file. Done. http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill_dataset_generator.py:260: On 2011/03/18 21:44:01, dennis_jeffrey wrote: > Add one more blank line before the start of the main() function, since this > function is not inside the above class. Done. http://codereview.chromium.org/6685077/diff/9004/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/9004/chrome/test/functional/autof... chrome/test/functional/autofill.py:2: # Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2011/03/22 04:08:58, Ilya Sherman wrote: > nit: Please don't reverse copyright updates ;) Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/fo... File chrome/test/data/autofill/form_phones.html (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/fo... chrome/test/data/autofill/form_phones.html:22: <!-- Basic PHONE field --> On 2011/03/22 23:28:53, dennis_jeffrey wrote: > Add a period at the end of all comment sentences in this file. Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/fo... chrome/test/data/autofill/form_phones.html:26: <!-- Set of phone fields with area code, and phone --> On 2011/03/22 23:28:53, dennis_jeffrey wrote: > Remove the comma in this sentence. Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/fo... chrome/test/data/autofill/form_phones.html:58: <!-- Set of fax fields with area code, and fax number --> On 2011/03/22 23:28:53, dennis_jeffrey wrote: > Remove the comma in this sentence. Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... File chrome/test/data/autofill/phone_pexpected_autofill.txt (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... chrome/test/data/autofill/phone_pexpected_autofill.txt:1: # Expected Profile for comparing phone numbers Autofill test. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > "Profile" --> "profile". Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... chrome/test/data/autofill/phone_pexpected_autofill.txt:1: # Expected Profile for comparing phone numbers Autofill test. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > "numbers Autofill test" > --> > "numbers in Autofill tests" Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... File chrome/test/data/autofill/phone_pinput_autofill.txt (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... chrome/test/data/autofill/phone_pinput_autofill.txt:1: # Input Profile for comparing phone numbers Autofill test. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > "Profile" --> "profile" Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/data/autofill/ph... chrome/test/data/autofill/phone_pinput_autofill.txt:1: # Input Profile for comparing phone numbers Autofill test. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > "numbers Autofill test" > --> > "numbers in Autofill tests" Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:19: class AutoFillTest(pyauto.PyUITest): On 2011/03/22 04:08:58, Ilya Sherman wrote: > nit: This should be "AutofillTest" -- see http://crbug.com/72758 Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:155: """Test phone fields parses correctly from a given profile.""" On 2011/03/22 23:28:53, dennis_jeffrey wrote: > "fields parses" --> "fields parse" Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:173: # Return keyboard key press. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > Sorry I wasn't clear before: when I asked for comments about what these > keypresses are doing, I meant for the comments to be at more of a high level to > describe what's going on from the user's point of view when you send this > sequence of key presses. For example, something like this: > > "Select the first text field, invoke the autofill dropdown, and select one of > the profiles to cause the form to be populated". I added the high level steps in the description. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:193: submitted. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > Should probably also mention here that this test is only partially automated. > You may also want to add a TODO to make this fully automated later if/when the > necessary functionality is available. Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:202: list_of_dict = gen.GenerateDataset(num_of_dict_to_generate=1501) On 2011/03/22 23:28:53, dennis_jeffrey wrote: > 1501 is a bit of a weird number! Just curious if this number was selected > arbitrarily, or if there's some special significance to this number? Wanted more than 1500 profiles based on the bug. Bug said 1500 plus profiles. 1501 is 1500+ profiles. ;) http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:211: # Return keyboard key press. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > Similar comment as the one above at line 173. Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:8: Used to test autofill.AutoFillTest.FormFillLatencyAfterSubmit. On 2011/03/22 04:08:58, Ilya Sherman wrote: > nit: AutoFill -> Autofill Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:111: self.dict_length = len(self.fields) On 2011/03/22 23:28:53, dennis_jeffrey wrote: > This variable doesn't seem to be used in this file anywhere. Nice catch. Removed. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:114: self.output_pattern += u'u"%s": u"%s", ' % (key_and_method[0], "%s") On 2011/03/22 04:08:58, Ilya Sherman wrote: > nit: How about """ self.output_pattern += u'u"%s": u"%%s", ' % key_and_method[0] > """ ? Also, it's probably better to use """ ', '.join() """ rather than "+=". Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:144: parts.append(u'%s' % function(*args)) On 2011/03/22 04:08:58, Ilya Sherman wrote: > nit: Why not just """ parts.append(function(*args)) """? This can't be done as I'm generating addresses. I need the integer type. This line is needed as it's translating each part to a string. I tried and the script threw an error: TypeError: sequence item 0: expected string, int found http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:164: """Uses _GenerateField() and state_construct to generate a state. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > "a state" --> "a random state" Well it's only one state which is CA. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:172: """Uses _GenerateField() and zip_construct to generate a zip code. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > "a zip" --> "a random zip" Only one zip code. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:180: """Uses _GenerateField() and country_construct to generate a country. On 2011/03/22 23:28:53, dennis_jeffrey wrote: > "a country" --> "a random country" One country. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:194: A random first name. On 2011/03/22 04:08:58, Ilya Sherman wrote: > nit: This does not return a random value, right? This comment is probably what > confused me earlier about how you were generating values. Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:204: A random email address. On 2011/03/22 04:08:58, Ilya Sherman wrote: > nit: Again, not actually random. Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:225: r'\'', output_dict[key]) # Escaping single quote: "'" -> '\'' On 2011/03/22 04:08:58, Ilya Sherman wrote: > nit: I'm fairly sure this line is a no-op. Can you give me an example of when > that would not be the case? Deleted. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:253: [output_dict[key_and_method[0]] for key_and_method in self.fields]) On 2011/03/22 04:08:58, Ilya Sherman wrote: > nit: How about """ [output_dict[key] for [key, method] in self.fields] """ ? Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:291: parser.error('Wrong log_level argument.') On 2011/03/22 23:28:53, dennis_jeffrey wrote: > May also want to do "parser.print_help()" here, so users can see what the > expected log_level arguments are. Done. http://codereview.chromium.org/6685077/diff/7003/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:757: def SendWebkitKeyEvent(self, key_code, tab_index=0, windex=0): On 2011/03/22 23:28:53, dennis_jeffrey wrote: > For consistency in naming variables, I recommend that you change "tab_index" and > "windex" to either: > > (1) "tab_index" and "window_index" > or > (2) "tindex" and "windex" > or > (3) "t_index" and "w_index" From the pydocs for pyauto most of the variables for this is set to tab_index and windex. http://circus:9999/pyauto.html#PyUITest-InstallExtension http://codereview.chromium.org/6685077/diff/7003/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:765: tab_index: tab index to work on. Defaults to 0 (first tab) On 2011/03/22 23:28:53, dennis_jeffrey wrote: > Swap the descriptions for the "windex" and "tab_index" parameters to match the > order they appear in the parameter list at line 757 above. Done.
LGTM with nit (but please wait for Dennis's LGTM as well). Thanks =) http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:144: parts.append(u'%s' % function(*args)) On 2011/03/24 19:46:51, dyu1 wrote: > On 2011/03/22 04:08:58, Ilya Sherman wrote: > > nit: Why not just """ parts.append(function(*args)) """? > > This can't be done as I'm generating addresses. I need the integer type. This > line is needed as it's translating each part to a string. I tried and the script > threw an error: TypeError: sequence item 0: expected string, int found In that case, I think you want """ parts.append(str(function(*args))) """
Actually, one more question: I seem to remember you mentioning earlier that you were running into timing issues with testing 1500 profiles. In particular, the Autofill popup would not always be available in time for the second "down arrow" keypress. Have you found a way to resolve this problem?
No, this has not yet been resolved. The reason is because we are sending key events without waiting for any notifications. Your tests in autofill_browsertest.cc sends a key press and waits for a NotificationType. We need to implement this in our hooks. We are still deciding if we want to implement generic event notifications to Chrome automation or just for this one test which could be messy. Essentially the hook for this would be to send the keydown event, wait for the event to be processed, then send the keyup event and wait for the notification. I believe that Dennis may look into implementing this if he has more time. On 2011/03/25 04:22:58, Ilya Sherman wrote: > Actually, one more question: I seem to remember you mentioning earlier that you > were running into timing issues with testing 1500 profiles. In particular, the > Autofill popup would not always be available in time for the second "down arrow" > keypress. Have you found a way to resolve this problem?
Just a few more minor comments. Also, since there is a C++ change here, you might want to consider running a try job to make sure that the build isn't broken by this CL. I don't think these minor C++ changes will break anything, but it might be a good habit to get into if you start modifying C++ code. http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill.py:202: list_of_dict = gen.GenerateDataset(num_of_dict_to_generate=1501) On 2011/03/24 19:46:51, dyu1 wrote: > On 2011/03/22 23:28:53, dennis_jeffrey wrote: > > 1501 is a bit of a weird number! Just curious if this number was selected > > arbitrarily, or if there's some special significance to this number? > > Wanted more than 1500 profiles based on the bug. Bug said 1500 plus profiles. > 1501 is 1500+ profiles. ;) What?!?! That's like an ice cream shop advertising "more than 30" flavors. That usually means they have 31 flavors :-D http://codereview.chromium.org/6685077/diff/7003/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:757: def SendWebkitKeyEvent(self, key_code, tab_index=0, windex=0): On 2011/03/24 19:46:51, dyu1 wrote: > On 2011/03/22 23:28:53, dennis_jeffrey wrote: > > For consistency in naming variables, I recommend that you change "tab_index" > and > > "windex" to either: > > > > (1) "tab_index" and "window_index" > > or > > (2) "tindex" and "windex" > > or > > (3) "t_index" and "w_index" > > From the pydocs for pyauto most of the variables for this is set to tab_index > and windex. > http://circus:9999/pyauto.html#PyUITest-InstallExtension Maybe we should change them all then >:-). Just kidding, you don't have to do that here :-) http://codereview.chromium.org/6685077/diff/21001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6685077/diff/21001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4336: autofill_type_to_string[PHONE_FAX_WHOLE_NUMBER] = "PHONE_FAX_WHOLE_NUMBER"; It might be a good idea to alphabetize these types/strings in lines 4318-4336. http://codereview.chromium.org/6685077/diff/21001/chrome/test/data/autofill/p... File chrome/test/data/autofill/phone_pinput_autofill.txt (right): http://codereview.chromium.org/6685077/diff/21001/chrome/test/data/autofill/p... chrome/test/data/autofill/phone_pinput_autofill.txt:7: 'ADDRESS_HOME_ZIP': '95110','PHONE_HOME_WHOLE_NUMBER': '1-408-123-4567', Is all of this extra spaces at the end of each line? If so, the trailing spaces should be deleted. http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill.py:157: The high level key presses executes the following: Select the first text "executes" --> "execute" http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill.py:200: The high level key presses executes the following: Select the first text "executes" --> "execute" http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill.py:206: selecting the a profile from the list. The tester will need to click on the "the a profile" --> "a profile" http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill.py:229: # TODO (dyu@chromium.org): add form hang or crash verification. "add form" --> "add automated form" http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:113: u',}' It seems more conventional to use implicit string concatenation within parens, rather than using the "+" operator and joining lines with "\".
http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/7003/chrome/test/functional/autof... chrome/test/functional/autofill_dataset_generator.py:144: parts.append(u'%s' % function(*args)) On 2011/03/25 03:55:31, Ilya Sherman wrote: > On 2011/03/24 19:46:51, dyu1 wrote: > > On 2011/03/22 04:08:58, Ilya Sherman wrote: > > > nit: Why not just """ parts.append(function(*args)) """? > > > > This can't be done as I'm generating addresses. I need the integer type. This > > line is needed as it's translating each part to a string. I tried and the > script > > threw an error: TypeError: sequence item 0: expected string, int found > > In that case, I think you want """ parts.append(str(function(*args))) """ Done. http://codereview.chromium.org/6685077/diff/21001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6685077/diff/21001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4336: autofill_type_to_string[PHONE_FAX_WHOLE_NUMBER] = "PHONE_FAX_WHOLE_NUMBER"; On 2011/03/25 16:56:14, dennis_jeffrey wrote: > It might be a good idea to alphabetize these types/strings in lines 4318-4336. I believe these strings are in the order or how most forms are presented and the ordering when adding an Autofill profile. http://codereview.chromium.org/6685077/diff/21001/chrome/test/data/autofill/p... File chrome/test/data/autofill/phone_pinput_autofill.txt (right): http://codereview.chromium.org/6685077/diff/21001/chrome/test/data/autofill/p... chrome/test/data/autofill/phone_pinput_autofill.txt:7: 'ADDRESS_HOME_ZIP': '95110','PHONE_HOME_WHOLE_NUMBER': '1-408-123-4567', On 2011/03/25 16:56:14, dennis_jeffrey wrote: > Is all of this extra spaces at the end of each line? If so, the trailing spaces > should be deleted. No, there are no extra spaces in this file. I updated it again to be sure. http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill.py:157: The high level key presses executes the following: Select the first text On 2011/03/25 16:56:14, dennis_jeffrey wrote: > "executes" --> "execute" Done. http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill.py:200: The high level key presses executes the following: Select the first text On 2011/03/25 16:56:14, dennis_jeffrey wrote: > "executes" --> "execute" Done. http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill.py:206: selecting the a profile from the list. The tester will need to click on the On 2011/03/25 16:56:14, dennis_jeffrey wrote: > "the a profile" --> "a profile" Done. http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill.py:229: # TODO (dyu@chromium.org): add form hang or crash verification. On 2011/03/25 16:56:14, dennis_jeffrey wrote: > "add form" --> "add automated form" Done. http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/21001/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:113: u',}' On 2011/03/25 16:56:14, dennis_jeffrey wrote: > It seems more conventional to use implicit string concatenation within parens, > rather than using the "+" operator and joining lines with "\". Done.
LGTM Just a few minor nits. http://codereview.chromium.org/6685077/diff/21001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6685077/diff/21001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4336: autofill_type_to_string[PHONE_FAX_WHOLE_NUMBER] = "PHONE_FAX_WHOLE_NUMBER"; On 2011/03/25 19:10:57, dyu1 wrote: > On 2011/03/25 16:56:14, dennis_jeffrey wrote: > > It might be a good idea to alphabetize these types/strings in lines 4318-4336. > > I believe these strings are in the order or how most forms are presented and the > ordering when adding an Autofill profile. Ok, if there's already some ordering to these values, great! Maybe you could add a comment to indicate this though. http://codereview.chromium.org/6685077/diff/21001/chrome/test/data/autofill/p... File chrome/test/data/autofill/phone_pinput_autofill.txt (right): http://codereview.chromium.org/6685077/diff/21001/chrome/test/data/autofill/p... chrome/test/data/autofill/phone_pinput_autofill.txt:7: 'ADDRESS_HOME_ZIP': '95110','PHONE_HOME_WHOLE_NUMBER': '1-408-123-4567', On 2011/03/25 19:10:57, dyu1 wrote: > On 2011/03/25 16:56:14, dennis_jeffrey wrote: > > Is all of this extra spaces at the end of each line? If so, the trailing > spaces > > should be deleted. > > No, there are no extra spaces in this file. I updated it again to be sure. Ok, just checking. In the previous patch I was looking at in the codereview site, I saw a bunch of green highlights that looked like added spaces at the end of each line. But in the current patch, I don't see them anymore. http://codereview.chromium.org/6685077/diff/26001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/26001/chrome/test/functional/auto... chrome/test/functional/autofill.py:229: # TODO: add automated form hang or crash verification. Why did you remove the "(dyu@chromium.org)"? It shows who authored the TODO so people know who to ask about it later (if the need arises).
http://codereview.chromium.org/6685077/diff/26001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/26001/chrome/test/functional/auto... chrome/test/functional/autofill.py:229: # TODO: add automated form hang or crash verification. On 2011/03/25 21:08:45, dennis_jeffrey wrote: > Why did you remove the "(dyu@chromium.org)"? It shows who authored the TODO so > people know who to ask about it later (if the need arises). Done.
LGTM A few more minor nits. http://codereview.chromium.org/6685077/diff/27002/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6685077/diff/27002/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4315: // Strings ordered by order of fields when adding a profile in Autofill prefs. Recommend moving this comment down below right above line 4319, immediately before the strings start getting declared. http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:50: zip_construct = [ u'95110', u'94109', u'94203', u'90120'] Add a comment saying that these zips are now matched to the corresponding cities in city_construct above. http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:112: u', '.join([u'u"%s" : u"%%s"' % key for key, method in self.fields]) +\ Recommend adding a space in-between the "+\" to make it "+ \" here (and at line 111 above). Thanks for the added comment, by the way. http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:168: """Uses _GenerateField() and zip_construct to generate a zip code. This function no longer uses "_GenerateField()". http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:171: A matched zip code. Maybe be a little more specific about what "matched" means here: "A zip code matched to the currently-selected city." (or something along those lines)
http://codereview.chromium.org/6685077/diff/27002/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/6685077/diff/27002/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:4315: // Strings ordered by order of fields when adding a profile in Autofill prefs. On 2011/03/27 16:42:35, dennis_jeffrey wrote: > Recommend moving this comment down below right above line 4319, immediately > before the strings start getting declared. Done. http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... File chrome/test/functional/autofill_dataset_generator.py (right): http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:50: zip_construct = [ u'95110', u'94109', u'94203', u'90120'] On 2011/03/27 16:42:35, dennis_jeffrey wrote: > Add a comment saying that these zips are now matched to the corresponding cities > in city_construct above. Done. http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:112: u', '.join([u'u"%s" : u"%%s"' % key for key, method in self.fields]) +\ On 2011/03/27 16:42:35, dennis_jeffrey wrote: > Recommend adding a space in-between the "+\" to make it "+ \" here (and at line > 111 above). Thanks for the added comment, by the way. Done. http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:168: """Uses _GenerateField() and zip_construct to generate a zip code. On 2011/03/27 16:42:35, dennis_jeffrey wrote: > This function no longer uses "_GenerateField()". Done. http://codereview.chromium.org/6685077/diff/27002/chrome/test/functional/auto... chrome/test/functional/autofill_dataset_generator.py:171: A matched zip code. On 2011/03/27 16:42:35, dennis_jeffrey wrote: > Maybe be a little more specific about what "matched" means here: > > "A zip code matched to the currently-selected city." > (or something along those lines) Done.
Code I commented on LGTM http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/form_... File chrome/test/data/autofill/form_phones.html (right): http://codereview.chromium.org/6685077/diff/9/chrome/test/data/autofill/form_... chrome/test/data/autofill/form_phones.html:1: <!DOCTYPE html> On 2011/03/22 01:18:58, Ilya Sherman wrote: > On 2011/03/21 18:42:35, dyu1 wrote: > > On 2011/03/18 23:17:11, Ilya Sherman wrote: > > > nit: Let's make a py/ subdirectory for python test data -- or more focused > > > directories, if those would make more sense. > > > > This is actually where all the data files fall under within > > chrome/test/data/_project_/ > > In the case of autofill, we have other subdirectories for non-pyauto test files. > Unless these are going to be used with pyauto as well, I think it makes sense > to move the pyauto-specific files into their own subdir. Nirnimesh -- any > thoughts? I'm fine with creating py subdir. http://codereview.chromium.org/6685077/diff/32001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/32001/chrome/test/functional/auto... chrome/test/functional/autofill.py:230: raw_input() remove this before checking in
http://codereview.chromium.org/6685077/diff/32001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/32001/chrome/test/functional/auto... chrome/test/functional/autofill.py:230: raw_input() On 2011/03/29 20:54:18, Nirnimesh wrote: > remove this before checking in I need this as I need to complete the test manually until I can automate the steps to check for form hang or crash verification.
http://codereview.chromium.org/6685077/diff/32001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/32001/chrome/test/functional/auto... chrome/test/functional/autofill.py:230: raw_input() On 2011/03/29 21:49:04, dyu1 wrote: > On 2011/03/29 20:54:18, Nirnimesh wrote: > > remove this before checking in > > I need this as I need to complete the test manually until I can automate the > steps to check for form hang or crash verification. In that case make the raw_input more verbose raw_input('Now verify manually. ')
Committing this if no more suggestions for fixes. http://codereview.chromium.org/6685077/diff/32001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6685077/diff/32001/chrome/test/functional/auto... chrome/test/functional/autofill.py:230: raw_input() On 2011/03/29 22:04:13, Nirnimesh wrote: > On 2011/03/29 21:49:04, dyu1 wrote: > > On 2011/03/29 20:54:18, Nirnimesh wrote: > > > remove this before checking in > > > > I need this as I need to complete the test manually until I can automate the > > steps to check for form hang or crash verification. > > In that case make the raw_input more verbose > > raw_input('Now verify manually. ') Done. |
