Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6770023:
Added a test for Autofill to check the postal code and state label change based on corresponding ... (Closed)
|
Created:
9 years, 9 months ago by dyu1 Modified:
9 years, 7 months ago CC:
chromium-reviews, kkania, Paweł Hajdan Jr., anantha, Nirnimesh Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdded a test for Autofill to check the postal code and state label change based on corresponding country.
- AutofillTest.testPostalCodeAndStateLabelsBasedOnCountry
- state_zip_labels.txt
TEST=none
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79958
Patch Set 1 #
Total comments: 19
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 8
Patch Set 4 : '' #
Total comments: 6
Patch Set 5 : '' #
Messages
Total messages: 12 (0 generated)
http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:364: def NewDriver(self): Fold the code here into where you're calling NewDriver(). http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:371: print dir(simplejson) Do you need this print? Remove if it is for debugging. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:380: driver.find_element_by_id( This is fine but if multiple postal codes are not present only the first missing code will be reported. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:389: 'Postal code label on the UI does not correspond to the selected \ This is a UI test so you don't need to say "on the UI". Just say "Postal code label does not match country: %s". http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:396: 'State label on the UI does not correspond to the selected \ Same as above. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:398: You might want to call driver.quit() here. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/state_zip... File chrome/test/webdriver/state_zip_labels.txt (right): http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/state_zip... chrome/test/webdriver/state_zip_labels.txt:1: { You might want to add a data directory under chrome/test/webdriver to place these test data files.
http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:364: def NewDriver(self): On 2011/03/30 19:17:02, Huyen wrote: > Fold the code here into where you're calling NewDriver(). What do you mean by fold the code here? If I didn't have this function then I would have to type out "self._launcher.GetURL()" instead of just use the "driver" object. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:371: print dir(simplejson) On 2011/03/30 19:17:02, Huyen wrote: > Do you need this print? Remove if it is for debugging. Done. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:380: driver.find_element_by_id( On 2011/03/30 19:17:02, Huyen wrote: > This is fine but if multiple postal codes are not present only the first missing > code will be reported. What do you mean by multiple postal codes? In this case the data in the dictionary file is set so if the test does break then I'll know there is a bug or something changed. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:389: 'Postal code label on the UI does not correspond to the selected \ On 2011/03/30 19:17:02, Huyen wrote: > This is a UI test so you don't need to say "on the UI". Just say "Postal code > label does not match country: %s". Done. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:396: 'State label on the UI does not correspond to the selected \ On 2011/03/30 19:17:02, Huyen wrote: > Same as above. Done. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:398: On 2011/03/30 19:17:02, Huyen wrote: > You might want to call driver.quit() here. The driver quits on it's own when I run the test. Ken mentioned that this was not needed. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/state_zip... File chrome/test/webdriver/state_zip_labels.txt (right): http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/state_zip... chrome/test/webdriver/state_zip_labels.txt:1: { On 2011/03/30 19:17:02, Huyen wrote: > You might want to add a data directory under chrome/test/webdriver to place > these test data files. Ken mentioned this was not desired as all the other data files seem to be in the same directory. I believe this is just temp solution.
http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:364: def NewDriver(self): On 2011/03/30 21:51:45, dyu1 wrote: > On 2011/03/30 19:17:02, Huyen wrote: > > Fold the code here into where you're calling NewDriver(). > > What do you mean by fold the code here? If I didn't have this function then I > would have to type out "self._launcher.GetURL()" instead of just use the > "driver" object. Inline WebDriver(self._launcher.GetUrl(), {}) where you need a driver. For example, line 374: driver = WebDriver(...) http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:380: driver.find_element_by_id( On 2011/03/30 21:51:45, dyu1 wrote: > On 2011/03/30 19:17:02, Huyen wrote: > > This is fine but if multiple postal codes are not present only the first > missing > > code will be reported. > > What do you mean by multiple postal codes? In this case the data in the > dictionary file is set so if the test does break then I'll know there is a bug > or something changed. > You're looping through the list of postal codes to verify that they all exist in the select menu right? So if multiple codes are not in the select menu your test would not detect the subsequent missing codes, only the first one. http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/state_zip... File chrome/test/webdriver/state_zip_labels.txt (right): http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/state_zip... chrome/test/webdriver/state_zip_labels.txt:1: { On 2011/03/30 21:51:45, dyu1 wrote: > On 2011/03/30 19:17:02, Huyen wrote: > > You might want to add a data directory under chrome/test/webdriver to place > > these test data files. > > Ken mentioned this was not desired as all the other data files seem to be in the > same directory. I believe this is just temp solution. > It is okay for now, but all data file should be moved into data dir eventually.
http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:364: def NewDriver(self): On 2011/03/30 22:08:03, Huyen wrote: > On 2011/03/30 21:51:45, dyu1 wrote: > > On 2011/03/30 19:17:02, Huyen wrote: > > > Fold the code here into where you're calling NewDriver(). > > > > What do you mean by fold the code here? If I didn't have this function then I > > would have to type out "self._launcher.GetURL()" instead of just use the > > "driver" object. > > Inline WebDriver(self._launcher.GetUrl(), {}) where you need a driver. For > example, line 374: > driver = WebDriver(...) If I fold it in then instead of driver.get for all future tests I write in this class, I will need to type in the longer version, which will be tedious. As you can see below there are a lot of driver objects.
http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6770023/diff/1/chrome/test/webdriver/chromedri... chrome/test/webdriver/chromedriver_tests.py:364: def NewDriver(self): On 2011/03/30 22:33:10, dyu1 wrote: > On 2011/03/30 22:08:03, Huyen wrote: > > On 2011/03/30 21:51:45, dyu1 wrote: > > > On 2011/03/30 19:17:02, Huyen wrote: > > > > Fold the code here into where you're calling NewDriver(). > > > > > > What do you mean by fold the code here? If I didn't have this function then > I > > > would have to type out "self._launcher.GetURL()" instead of just use the > > > "driver" object. > > > > Inline WebDriver(self._launcher.GetUrl(), {}) where you need a driver. For > > example, line 374: > > driver = WebDriver(...) > > If I fold it in then instead of driver.get for all future tests I write in this > class, I will need to type in the longer version, which will be tedious. As you > can see below there are a lot of driver objects. When you add more tests later you can add that method or you can change the setUp() step to instantiate a new driver. But for now you don't need this.
LGTM
http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:355: """Autofill tests that interacts with web UI.""" web settings UI?, is that what you mean http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:355: """Autofill tests that interacts with web UI.""" Can you put these tests near the end of the file, right after a comment that says: """Chrome functional test section. Put all tests of ChromeDriver above. TODO(dyu): Move these tests out of here when pyauto has these capabilities. """ http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:376: driver.find_element_by_id( this looks a bit funky; can you check the style guide for this; I think the newline should go after the '.' maybe http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/state_... File chrome/test/webdriver/state_zip_labels.txt (right): http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/state_... chrome/test/webdriver/state_zip_labels.txt:2: "AD":{"name":"Andorra","postalCodeLabel":"Postal code","stateLabel":"Parish"}, Where'd you get these from? Are we going to have problems keeping this up to date? Do we really need to test every single country?
http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:355: """Autofill tests that interacts with web UI.""" On 2011/03/31 00:48:48, kkania wrote: > web settings UI?, is that what you mean I don't think all Autofill tests will be related to the web settings UI. Rewording the description. http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:355: """Autofill tests that interacts with web UI.""" On 2011/03/31 00:48:48, kkania wrote: > Can you put these tests near the end of the file, right after a comment that > says: > > """Chrome functional test section. Put all tests of ChromeDriver above. > > TODO(dyu): Move these tests out of here when pyauto has these capabilities. > """ Done. http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:376: driver.find_element_by_id( On 2011/03/31 00:48:48, kkania wrote: > this looks a bit funky; can you check the style guide for this; I think the > newline should go after the '.' maybe Turns out the line fits fine. The newline can go after the (. http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/state_... File chrome/test/webdriver/state_zip_labels.txt (right): http://codereview.chromium.org/6770023/diff/5004/chrome/test/webdriver/state_... chrome/test/webdriver/state_zip_labels.txt:2: "AD":{"name":"Andorra","postalCodeLabel":"Postal code","stateLabel":"Parish"}, On 2011/03/31 00:48:48, kkania wrote: > Where'd you get these from? Are we going to have problems keeping this up to > date? Do we really need to test every single country? There are more countries than this, I only took a subset that displays different state labels. I don't think this file will need much maintaining unless the country code changes. Forgot where in the code base I got this but the mapping is in autofill_country.cc
LGTM after nits; make sure it passes the bots http://codereview.chromium.org/6770023/diff/9003/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6770023/diff/9003/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:379: """Chrome functional test section. Put all tests of ChromeDriver below. except you got the wording wrong; how about: "all tests of the implementation of ChromeDriver should go above" http://codereview.chromium.org/6770023/diff/9003/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:383: class AutofillTest(unittest.TestCase): two newlines above http://codereview.chromium.org/6770023/diff/9003/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:384: """Autofill tests.""" hehe, if this is the best you can do just drop the comment
http://codereview.chromium.org/6770023/diff/9003/chrome/test/webdriver/chrome... File chrome/test/webdriver/chromedriver_tests.py (right): http://codereview.chromium.org/6770023/diff/9003/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:379: """Chrome functional test section. Put all tests of ChromeDriver below. On 2011/03/31 03:33:25, kkania wrote: > except you got the wording wrong; how about: "all tests of the implementation of > ChromeDriver should go above" Done. http://codereview.chromium.org/6770023/diff/9003/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:383: class AutofillTest(unittest.TestCase): On 2011/03/31 03:33:25, kkania wrote: > two newlines above Done. http://codereview.chromium.org/6770023/diff/9003/chrome/test/webdriver/chrome... chrome/test/webdriver/chromedriver_tests.py:384: """Autofill tests.""" On 2011/03/31 03:33:25, kkania wrote: > hehe, if this is the best you can do just drop the comment Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
