Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6850007:
Fixed a few Autofill automation failures and added new tests. (Closed)
|
Created:
9 years, 8 months ago by dyu1 Modified:
9 years, 7 months ago CC:
chromium-reviews, John Grabowski, anantha, Nirnimesh, dyu1, Paweł Hajdan Jr. Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionFixed a few Autofill automation failures and added new tests.
- Deprecated testFilterMalformedEmailAddresses as the logic changed.
- Add testProfilesNotAggregatedWithInvalidEmail
- Add testInvalidCreditCardIsNotAggregated
- Add testWhiteSpacesStrippedForValidCCNums
- Add testCharSeparatorsStrippedForValidCCNums
- Fixed testAutofillInvalid
- Fixed testComparePhoneNumbers
- Fixed testNoAutofillForReadOnlyFields
TEST=none
BUG=73439, 78844, 79405, 78519
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81861
Patch Set 1 #
Total comments: 44
Patch Set 2 : '' #
Total comments: 14
Patch Set 3 : '' #
Total comments: 28
Patch Set 4 : '' #
Total comments: 8
Patch Set 5 : '' #
Total comments: 2
Patch Set 6 : '' #
Total comments: 4
Patch Set 7 : '' #
Total comments: 1
Messages
Total messages: 17 (0 generated)
I could not combine the white space and character separator tests due to this limitation: crbug.com/70694
http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:96: expected_credit_card = {'CREDIT_CARD_NUMBER': 'Not_0123-5Checked'} Since "credit_card" is identical to "expected_credit_card" in this case, I think we can remove "expected_credit_card" and just use "credit_card" instead. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:100: msg='Credit card number in prefs not saved as-is.') Indent by 1 fewer space. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:103: """Test credit card info with an invalid cc number is not aggregated. Remove "cc"; I think it's already implied within the sentence. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:105: When filling out a form with an invalid CC number, one that does not pass "cc" --> "credit card" (for clarity). http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:106: the Luhn test, the credit card info is not saved into Autofill preferences. Instead of using commas around "one that does not pass the Luhn test", I recommend putting that within parens instead. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:106: the Luhn test, the credit card info is not saved into Autofill preferences. "is not saved" --> "should not be saved" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:109: 'CREDIT_CARD_NUMBER': '4408 0412 3456 7890', Would it be easy to add a brief comment to explain why this particular number does not pass the Luhn test? Is a particular digit within this number invalid? http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:126: # Wait until form is submitted and page completes loading. "form" --> "the form" "page" --> "the page" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:135: """Test whitespaces are stripped for valid CC numbers. "CC" --> "credit card" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:137: Credit Card Number entered must pass the Luhn test. For reference: "The credit card number used in this test passes the Luhn test. For reference:" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:148: for key, value in credit_card_info.iteritems() : Remove the space before the ':' http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:159: # Wait until form is submitted and page completes loading. "form" --> "the form" "page" --> "the page" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:169: msg='White spaces not stripped from a valid cc number.') "White spaces" --> "Whitespaces" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:172: """Test char seperators are stripped for valid CC numbers. "seperators" --> "separators" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:172: """Test char seperators are stripped for valid CC numbers. "CC" --> "credit card" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:174: Credit Card Number entered must pass the Luhn test. For reference: Same comment as line 137 above. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:185: for key, value in credit_card_info.iteritems() : Remove space before ':' http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:196: # Wait until form is submitted and page completes loading. "form" --> "the form" "page" --> "the page" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:229: self.assertEqual([], self.GetAutofillProfile()['profiles']) Add a msg='...' in case this assertion fails. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:232: """Test Autofill does not aggregate profiles with an invalid Email.""" "Email" --> "email" http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:256: self.assertEqual([], self.GetAutofillProfile()['profiles']) Add a msg='...' in case this assertion fails. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:304: 'Expected: "%s"\nReturned: "%s"' % (key, value, form_values[key]))) Indent this line 4 more spaces.
http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill.py File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:96: expected_credit_card = {'CREDIT_CARD_NUMBER': 'Not_0123-5Checked'} On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Since "credit_card" is identical to "expected_credit_card" in this case, I think > we can remove "expected_credit_card" and just use "credit_card" instead. Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:100: msg='Credit card number in prefs not saved as-is.') On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Indent by 1 fewer space. Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:103: """Test credit card info with an invalid cc number is not aggregated. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Remove "cc"; I think it's already implied within the sentence. Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:105: When filling out a form with an invalid CC number, one that does not pass On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "cc" --> "credit card" (for clarity). Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:106: the Luhn test, the credit card info is not saved into Autofill preferences. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Instead of using commas around "one that does not pass the Luhn test", I > recommend putting that within parens instead. Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:106: the Luhn test, the credit card info is not saved into Autofill preferences. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Instead of using commas around "one that does not pass the Luhn test", I > recommend putting that within parens instead. Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:109: 'CREDIT_CARD_NUMBER': '4408 0412 3456 7890', I added a helper function instead to do the checking. Not sure if that was the ideal way though. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Would it be easy to add a brief comment to explain why this particular number > does not pass the Luhn test? Is a particular digit within this number invalid? http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:126: # Wait until form is submitted and page completes loading. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "form" --> "the form" > "page" --> "the page" Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:135: """Test whitespaces are stripped for valid CC numbers. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "CC" --> "credit card" Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:137: Credit Card Number entered must pass the Luhn test. For reference: On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "The credit card number used in this test passes the Luhn test. For reference:" Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:148: for key, value in credit_card_info.iteritems() : On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Remove the space before the ':' Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:159: # Wait until form is submitted and page completes loading. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "form" --> "the form" > "page" --> "the page" Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:169: msg='White spaces not stripped from a valid cc number.') On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "White spaces" --> "Whitespaces" Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:172: """Test char seperators are stripped for valid CC numbers. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "seperators" --> "separators" Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:172: """Test char seperators are stripped for valid CC numbers. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "CC" --> "credit card" Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:174: Credit Card Number entered must pass the Luhn test. For reference: On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Same comment as line 137 above. Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:185: for key, value in credit_card_info.iteritems() : On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Remove space before ':' Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:196: # Wait until form is submitted and page completes loading. On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "form" --> "the form" > "page" --> "the page" Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:229: self.assertEqual([], self.GetAutofillProfile()['profiles']) On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Add a msg='...' in case this assertion fails. Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:232: """Test Autofill does not aggregate profiles with an invalid Email.""" On 2011/04/14 21:53:03, dennis_jeffrey wrote: > "Email" --> "email" Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:256: self.assertEqual([], self.GetAutofillProfile()['profiles']) On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Add a msg='...' in case this assertion fails. Done. http://codereview.chromium.org/6850007/diff/1/chrome/test/functional/autofill... chrome/test/functional/autofill.py:304: 'Expected: "%s"\nReturned: "%s"' % (key, value, form_values[key]))) On 2011/04/14 21:53:03, dennis_jeffrey wrote: > Indent this line 4 more spaces. Done.
http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:114: number: the credit card number being validated. Add: "as a string" http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:122: return (( you don't really need 3 lines here. http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:140: if self._LuhnCreditCardNumberValidator(cc_number) == False: Don't do an if. Do an assert. http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:162: else: remove the else http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:206: print aggregated_cc_1 why print? http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:209: print 'Whitespaces or separator chars not stripped.' why print? why not assert? http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:260: self.assertEqual([], self.GetAutofillProfile()['profiles'], use assertFalse
http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:114: number: the credit card number being validated. On 2011/04/15 05:29:45, Nirnimesh wrote: > Add: "as a string" Done. http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:122: return (( On 2011/04/15 05:29:45, Nirnimesh wrote: > you don't really need 3 lines here. Done. http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:140: if self._LuhnCreditCardNumberValidator(cc_number) == False: On 2011/04/15 05:29:45, Nirnimesh wrote: > Don't do an if. > Do an assert. Done. http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:162: else: On 2011/04/15 05:29:45, Nirnimesh wrote: > remove the else Done. http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:206: print aggregated_cc_1 On 2011/04/15 05:29:45, Nirnimesh wrote: > why print? Forgot to remove this while I was debugging. http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:209: print 'Whitespaces or separator chars not stripped.' On 2011/04/15 05:29:45, Nirnimesh wrote: > why print? why not assert? Done.
http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/3001/chrome/test/functional/autof... chrome/test/functional/autofill.py:260: self.assertEqual([], self.GetAutofillProfile()['profiles'], On 2011/04/15 05:29:45, Nirnimesh wrote: > use assertFalse Done.
LGTM. Please make sure the bots stay green after you check it in. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:114: number: the credit card number being validated as a string. nit: put a , after validated http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:117: boolean whether the credit card number is true or false. s/is true or false/is valid or not/
Some more comments. Before you submit, let me investigate the "Luhn test" a little more so we can make sure your Luhn check is implemented correctly :-) http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:114: number: the credit card number being validated as a string. Clarify that this function assumes the string contains only digits (no spaces or dashes). http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:123: for d in reverse[1::2])) % 10 == 0) As discussed offline, the implementation here might not quite match what we expect is happening - we can investigate this a bit more offline. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:126: """Test credit card info with an invalid number is not aggregated. Delete 1 of the extra spaces within "invalid number" http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:128: When filling out a form with an invalid credit card number (one that Delete 1 of the extra spaces within "card number" http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:140: msg='This test requires an invalid credit card number.') Indent by 1 more space. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:166: The credit card number used in this test passes the Luhn test. "number" --> "numbers" "passes" --> "pass" http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:177: (optional) In the previous test above, you assert that the credit card number does not pass the Luhn test. Should we assert here that the 2 credit card numbers do pass the Luhn test? http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:199: # Verify the filled in credit card number against the aggregated number. "filled in" --> "filled-in" http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:204: self.assertFalse((' ' in aggregated_cc_1 or ' ' in aggregated_cc_2 or '-' in Recommend moving: '-' in to the next line. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:205: aggregated_cc_1 or '-' in aggregated_cc_2), Indent line by 1 more space. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:206: msg='Whitespaces or separator chars not stripped.') Indent line by 1 more space.
One more comment regarding the Luhn test implementation. Thanks! http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:101: def _LuhnCreditCardNumberValidator(self, number): Based on what I read online (http://www.merriampark.com/anatomycc.htm), it seems we need to do the following to perform the Luhn test: 1) Reverse the digits. 2) Double the value of every digit that is in an even-numbered position (1-based, so starting with the second digit). 3) For every doubled digit: if the value is greater than 9, subtract 9 from the value. 4) Sum all resulting digit values in all positions. 5) If the sum is divisible by 10, the number is valid. Otherwise, the number is invalid. If you agree with the steps above, could you revise the comments and implementation in this function accordingly?
http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:101: def _LuhnCreditCardNumberValidator(self, number): On 2011/04/15 23:41:55, dennis_jeffrey wrote: > Based on what I read online (http://www.merriampark.com/anatomycc.htm), it seems > we need to do the following to perform the Luhn test: > > 1) Reverse the digits. > 2) Double the value of every digit that is in an even-numbered position > (1-based, so starting with the second digit). > 3) For every doubled digit: if the value is greater than 9, subtract 9 from the > value. > 4) Sum all resulting digit values in all positions. > 5) If the sum is divisible by 10, the number is valid. Otherwise, the number is > invalid. > > If you agree with the steps above, could you revise the comments and > implementation in this function accordingly? Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:114: number: the credit card number being validated as a string. On 2011/04/15 22:15:46, Nirnimesh wrote: > nit: put a , after validated Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:114: number: the credit card number being validated as a string. On 2011/04/15 22:51:19, dennis_jeffrey wrote: > Clarify that this function assumes the string contains only digits (no spaces or > dashes). I added a line to filter the numbers so it removed all non-digit characters. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:117: boolean whether the credit card number is true or false. On 2011/04/15 22:15:46, Nirnimesh wrote: > s/is true or false/is valid or not/ Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:123: for d in reverse[1::2])) % 10 == 0) On 2011/04/15 22:51:19, dennis_jeffrey wrote: > As discussed offline, the implementation here might not quite match what we > expect is happening - we can investigate this a bit more offline. Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:126: """Test credit card info with an invalid number is not aggregated. On 2011/04/15 22:51:19, dennis_jeffrey wrote: > Delete 1 of the extra spaces within "invalid number" Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:128: When filling out a form with an invalid credit card number (one that On 2011/04/15 22:51:19, dennis_jeffrey wrote: > Delete 1 of the extra spaces within "card number" Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:140: msg='This test requires an invalid credit card number.') On 2011/04/15 22:51:19, dennis_jeffrey wrote: > Indent by 1 more space. Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:166: The credit card number used in this test passes the Luhn test. On 2011/04/15 22:51:19, dennis_jeffrey wrote: > "number" --> "numbers" > "passes" --> "pass" Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:177: On 2011/04/15 22:51:19, dennis_jeffrey wrote: > (optional) In the previous test above, you assert that the credit card number > does not pass the Luhn test. Should we assert here that the 2 credit card > numbers do pass the Luhn test? Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:199: # Verify the filled in credit card number against the aggregated number. On 2011/04/15 22:51:19, dennis_jeffrey wrote: > "filled in" --> "filled-in" Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:204: self.assertFalse((' ' in aggregated_cc_1 or ' ' in aggregated_cc_2 or '-' in On 2011/04/15 22:51:19, dennis_jeffrey wrote: > Recommend moving: > > '-' in > > to the next line. Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:205: aggregated_cc_1 or '-' in aggregated_cc_2), On 2011/04/15 22:51:19, dennis_jeffrey wrote: > Indent line by 1 more space. Done. http://codereview.chromium.org/6850007/diff/11001/chrome/test/functional/auto... chrome/test/functional/autofill.py:206: msg='Whitespaces or separator chars not stripped.') On 2011/04/15 22:51:19, dennis_jeffrey wrote: > Indent line by 1 more space. Done.
http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... chrome/test/functional/autofill.py:7: import re move this 2 lines down to ensure it's in alphabetical order. http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... chrome/test/functional/autofill.py:110: 4. Sum all resulting digit values in all psitions. "psitions" --> "positions" http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... chrome/test/functional/autofill.py:111: 5. Is the sum is divisible by 10, the number is valid. Otherwise, the "Is the sum" --> "If the sum" http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... chrome/test/functional/autofill.py:125: return ((sum(reverse[0::2]) + sum(sum(divmod(d*2, 10)) The implementation needs to be updated to reflect the steps listed above in lines 106-112.
http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... chrome/test/functional/autofill.py:7: import re On 2011/04/16 00:22:16, dennis_jeffrey wrote: > move this 2 lines down to ensure it's in alphabetical order. Done. http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... chrome/test/functional/autofill.py:110: 4. Sum all resulting digit values in all psitions. On 2011/04/16 00:22:16, dennis_jeffrey wrote: > "psitions" --> "positions" Reworded it. http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... chrome/test/functional/autofill.py:111: 5. Is the sum is divisible by 10, the number is valid. Otherwise, the On 2011/04/16 00:22:16, dennis_jeffrey wrote: > "Is the sum" --> "If the sum" Re wrote it. http://codereview.chromium.org/6850007/diff/8004/chrome/test/functional/autof... chrome/test/functional/autofill.py:125: return ((sum(reverse[0::2]) + sum(sum(divmod(d*2, 10)) On 2011/04/16 00:22:16, dennis_jeffrey wrote: > The implementation needs to be updated to reflect the steps listed above in > lines 106-112. Rewrote it to fit this formula.
http://codereview.chromium.org/6850007/diff/10004/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/10004/chrome/test/functional/auto... chrome/test/functional/autofill.py:128: for d in reverse[1::2])) % 10 == 0) The re-written description doesn't seem to be complete - from step 4.1 to 4.2, you've subtracted 9 from each digit value that is greater than 9, but the comments don't explicitly say this. Another issue with the description is that you refer to "even digits" and "odd digits", when in fact you mean to refer to their positions, not their values, so this needs to be clear: "digits in an even-numbered position" or "digits in an odd-numbered position". One more issue in the implementation is that I don't see where 9 is being subtracted from any values that are greater than 9. So the implementation still doesn't seem to match the steps. Am I missing something?
http://codereview.chromium.org/6850007/diff/10004/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/10004/chrome/test/functional/auto... chrome/test/functional/autofill.py:128: for d in reverse[1::2])) % 10 == 0) 1. No I did not subtract nine, instead I added up the numbers such as 16 is 1 + 6 = 7 or 18 is 1 + 8 = 9 or 4 is 0 + 4 = 4. 2. fixed. 3. There is no subtraction of nine. It's the same as adding it. On 2011/04/16 00:44:48, dennis_jeffrey wrote: > The re-written description doesn't seem to be complete - from step 4.1 to 4.2, > you've subtracted 9 from each digit value that is greater than 9, but the > comments don't explicitly say this. > > Another issue with the description is that you refer to "even digits" and "odd > digits", when in fact you mean to refer to their positions, not their values, so > this needs to be clear: "digits in an even-numbered position" or "digits in an > odd-numbered position". > > One more issue in the implementation is that I don't see where 9 is being > subtracted from any values that are greater than 9. So the implementation still > doesn't seem to match the steps. Am I missing something?
http://codereview.chromium.org/6850007/diff/9006/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/9006/chrome/test/functional/autof... chrome/test/functional/autofill.py:112: 4.2. Sum the digits of each multiplication: 2, 7, 6, 4, 9 There's something missing between steps 4.1 and 4.2 How does the 16 change to 7, and the 18 change to 9? That needs to be described here. http://codereview.chromium.org/6850007/diff/9006/chrome/test/functional/autof... chrome/test/functional/autofill.py:129: for d in reverse[1::2])) % 10 == 0) Ok, I finally see how this works. The part I didn't realize was that "subtracting 9" in this case (which is how the webpage describing the Luhn test explains it) would be equivalent to adding the quotient + remainder after dividing by 10 (which is what "sum(divmod(d*2, 10))" is doing. This is a place where I think the conciseness is making things a little difficult to understand, though it works. I just want to make sure that it's clear how the steps listed in the example above map to the implementation in this line. The only confusing part is around steps 4.1 - 4.2 above, which map to the "sum(divmod(d*2, 10))" part in the implementation. Maybe just address the comment at line 112 above, and that should clarify things.
http://codereview.chromium.org/6850007/diff/9006/chrome/test/functional/autof... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/9006/chrome/test/functional/autof... chrome/test/functional/autofill.py:112: 4.2. Sum the digits of each multiplication: 2, 7, 6, 4, 9 On 2011/04/16 01:21:14, dennis_jeffrey wrote: > There's something missing between steps 4.1 and 4.2 How does the 16 change to > 7, and the 18 change to 9? That needs to be described here. Done. http://codereview.chromium.org/6850007/diff/9006/chrome/test/functional/autof... chrome/test/functional/autofill.py:129: for d in reverse[1::2])) % 10 == 0) I provided an example above how I'm adding up the values. On 2011/04/16 01:21:14, dennis_jeffrey wrote: > Ok, I finally see how this works. The part I didn't realize was that > "subtracting 9" in this case (which is how the webpage describing the Luhn test > explains it) would be equivalent to adding the quotient + remainder after > dividing by 10 (which is what "sum(divmod(d*2, 10))" is doing. > > This is a place where I think the conciseness is making things a little > difficult to understand, though it works. I just want to make sure that it's > clear how the steps listed in the example above map to the implementation in > this line. The only confusing part is around steps 4.1 - 4.2 above, which map > to the "sum(divmod(d*2, 10))" part in the implementation. Maybe just address > the comment at line 112 above, and that should clarify things.
LGTM Thanks for putting up with my pestering on the Luhn stuff :-D http://codereview.chromium.org/6850007/diff/10009/chrome/test/functional/auto... File chrome/test/functional/autofill.py (right): http://codereview.chromium.org/6850007/diff/10009/chrome/test/functional/auto... chrome/test/functional/autofill.py:113: (0 + 2 = 2, 1 + 6 = 7, 0 + 6 = 6, 0 + 4 = 4, 1 + 8 = 9) Aha - this clarifies so much. I think it would have helped if you had worded step 4.2 like this (might have prevented my confusion): "For each resulting value that is now 2 digits, add the digits together: 2, 7, 6, 4, 9". |
