Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7748032:
Close last tab test (Closed)
|
Created:
9 years, 4 months ago by tturchetto Modified:
9 years, 4 months ago CC:
chromium-reviews, Nirnimesh, John Grabowski, anantha, dyu1, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPyauto Test: adding test for Chrome OS close tabs
BUG=
TEST=this is a test
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98444
Patch Set 1 #
Total comments: 20
Patch Set 2 : Modified files based on the previous code review #
Total comments: 8
Patch Set 3 : modified chromeos_browser.py based on previous CL review #
Total comments: 11
Patch Set 4 : Enable Chrome os browser #Patch Set 5 : Modified chromeos_browser.py based on the code review #
Total comments: 2
Patch Set 6 : modified based on the CL review #Messages
Total messages: 13 (0 generated)
I think the CL description should be a little more specific. Something like this maybe: "New pyauto test for ChromeOS that closes all tabs." http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... File chrome/test/functional/close_tab.py (right): http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:1: #!/usr/bin/python Since it looks like this test is chromeos-specific, we should probably name the file something like "chromeos_tabs.py". http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:6: import pyauto_functional I recommend adding a comment saying that this import must occur before "import pyauto", since there's a dependency there. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:11: """Close last tab test for ChromeOS. Although there's only 1 test in this class right now, I think the class itself would involve any tests that deal with tabs in ChromeOS. So I think the docstring should probably be a little more general, maybe something like "Tests involving tabs on ChromeOS". http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:17: """Close all tabs. There should be 1 tab open on Chrome OS""" "Closes all tabs and verifies 1 tab is still open on ChromeOS." http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:20: for tab_index in xrange(tab_count - 1, -1, -1): how about using xrange(tab_count, 0, -1)? It seems more intuitive. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:24: self.assertEqual(1, len(info['windows'])) # one window I think the comment is not necessary; it's easy to see that from the code. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:25: self.assertEqual(1, len(info['windows'][0]['tabs'])) # one tab I think the comment is not necessary; it's easy to see that from the code. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:27: info['windows'][0]['tabs'][0]['url']) # empty URL I think the comment is not necessary, but it also seems a bit misleading. We're making sure the url is 'chrome://newtab/' here. I think we can just remove the comment. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:27: info['windows'][0]['tabs'][0]['url']) # empty URL For each of the 3 assertions above, you may want to consider adding an informative error message in the event that the assertion fails. For example, something like this: url = info['windows'][0]['tabs'][0]['url'] self.assertEqual('chrome://newtab/', url, msg='Unexpected URL: %s' % url) http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:28: Add 1 more blank line here.
The name of the file is too closely tied to the single test. Just having one test in a file doesn't make sense. It should be renamed to something like: chromeos_browser.py (or chromeos_desktopui.py.. or anything beginning with chromeos_ that you like), so that more chromeos-specific ui tests can go in there
http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... File chrome/test/functional/close_tab.py (right): http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:1: #!/usr/bin/python Renamed the file to chromeos_browser.py On 2011/08/25 23:09:20, dennis_jeffrey wrote: > Since it looks like this test is chromeos-specific, we should probably name the > file something like "chromeos_tabs.py". http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:6: import pyauto_functional On 2011/08/25 23:09:20, dennis_jeffrey wrote: > I recommend adding a comment saying that this import must occur before "import > pyauto", since there's a dependency there. Done. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:11: """Close last tab test for ChromeOS. On 2011/08/25 23:09:20, dennis_jeffrey wrote: > Although there's only 1 test in this class right now, I think the class itself > would involve any tests that deal with tabs in ChromeOS. So I think the > docstring should probably be a little more general, maybe something like "Tests > involving tabs on ChromeOS". Done. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:17: """Close all tabs. There should be 1 tab open on Chrome OS""" On 2011/08/25 23:09:20, dennis_jeffrey wrote: > "Closes all tabs and verifies 1 tab is still open on ChromeOS." Done. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:20: for tab_index in xrange(tab_count - 1, -1, -1): On 2011/08/25 23:09:20, dennis_jeffrey wrote: > how about using xrange(tab_count, 0, -1)? It seems more intuitive. this will give error message, therefore, I keep the way it is. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:24: self.assertEqual(1, len(info['windows'])) # one window On 2011/08/25 23:09:20, dennis_jeffrey wrote: > I think the comment is not necessary; it's easy to see that from the code. Done. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:25: self.assertEqual(1, len(info['windows'][0]['tabs'])) # one tab On 2011/08/25 23:09:20, dennis_jeffrey wrote: > I think the comment is not necessary; it's easy to see that from the code. Done. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:27: info['windows'][0]['tabs'][0]['url']) # empty URL On 2011/08/25 23:09:20, dennis_jeffrey wrote: > I think the comment is not necessary, but it also seems a bit misleading. We're > making sure the url is 'chrome://newtab/' here. I think we can just remove the > comment. Done. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:27: info['windows'][0]['tabs'][0]['url']) # empty URL On 2011/08/25 23:09:20, dennis_jeffrey wrote: > I think the comment is not necessary, but it also seems a bit misleading. We're > making sure the url is 'chrome://newtab/' here. I think we can just remove the > comment. Done. http://codereview.chromium.org/7748032/diff/1/chrome/test/functional/close_ta... chrome/test/functional/close_tab.py:28: On 2011/08/25 23:09:20, dennis_jeffrey wrote: > Add 1 more blank line here. Done.
LGTM after you fix my comments. They're minor http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... File chrome/test/functional/chromeos_browser.py (right): http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:6: import pyauto_functional # pyauto_functional must come before pyauto nit: put another space before # http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:11: """Close last tab test for ChromeOS. Remove this http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:13: Requires ChromeOS to be logged in. Remove this http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:30: need another blank line here
http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... File chrome/test/functional/chromeos_browser.py (right): http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:6: import pyauto_functional # pyauto_functional must come before pyauto On 2011/08/26 01:14:59, Nirnimesh wrote: > nit: put another space before # Done. http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:11: """Close last tab test for ChromeOS. On 2011/08/26 01:14:59, Nirnimesh wrote: > Remove this Done. http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:13: Requires ChromeOS to be logged in. On 2011/08/26 01:14:59, Nirnimesh wrote: > Remove this Done. http://codereview.chromium.org/7748032/diff/3002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:30: On 2011/08/26 01:14:59, Nirnimesh wrote: > need another blank line here Done.
You didn't upload
uploaded. On Thu, Aug 25, 2011 at 6:24 PM, <nirnimesh@chromium.org> wrote: > You didn't upload > > > http://codereview.chromium.**org/7748032/<http://codereview.chromium.org/7748... >
lgtm still
Just a few more comments. http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... File chrome/test/functional/chromeos_browser.py (right): http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:6: import pyauto_functional # pyauto_functional must come before pyauto nit: add period at end of comment sentence http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:13: """Close all tabs and verifies 1 tab is still open on Chrome OS.""" nit: choose 1 of the following: 1) 'verifies' --> 'verify' 2) 'Close' --> 'Closes' http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:16: for tab_index in xrange(tab_count - 1, -1, -1): (optional): I think it should work the same if you change this line to: for tab_index in xrange(tab_count, 0, -1): It must have been an unrelated error you saw last time, since I don't think this change should cause an error; if it does, I'd like to know why :-) http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:22: self.assertEqual('chrome://newtab/', info['windows'][0]['tabs'][0]['url']) Remove this line; this is already checked now in lines 23-25 below. http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:25: msg = 'Unexpected URL: %s' % url) remove the spaces before and after the '='
http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... File chrome/test/functional/chromeos_browser.py (right): http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:6: import pyauto_functional # pyauto_functional must come before pyauto On 2011/08/26 01:35:50, dennis_jeffrey wrote: > nit: add period at end of comment sentence Done. http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:13: """Close all tabs and verifies 1 tab is still open on Chrome OS.""" On 2011/08/26 01:35:50, dennis_jeffrey wrote: > nit: choose 1 of the following: > > 1) 'verifies' --> 'verify' > 2) 'Close' --> 'Closes' Done. http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:16: for tab_index in xrange(tab_count - 1, -1, -1): Chatted this offline. Will keep as it is. On 2011/08/26 01:35:50, dennis_jeffrey wrote: > (optional): I think it should work the same if you change this line to: > > for tab_index in xrange(tab_count, 0, -1): > > It must have been an unrelated error you saw last time, since I don't think this > change should cause an error; if it does, I'd like to know why :-) http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:22: self.assertEqual('chrome://newtab/', info['windows'][0]['tabs'][0]['url']) On 2011/08/26 01:35:50, dennis_jeffrey wrote: > Remove this line; this is already checked now in lines 23-25 below. Done. http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:25: msg = 'Unexpected URL: %s' % url) On 2011/08/26 01:35:50, dennis_jeffrey wrote: > remove the spaces before and after the '=' Done.
LGTM Please just remove the 1 line mentioned in my comment below before you commit. Thank you! http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... File chrome/test/functional/chromeos_browser.py (right): http://codereview.chromium.org/7748032/diff/6002/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:16: for tab_index in xrange(tab_count - 1, -1, -1): On 2011/08/26 02:07:37, tturchetto wrote: > Chatted this offline. Will keep as it is. > > On 2011/08/26 01:35:50, dennis_jeffrey wrote: > > (optional): I think it should work the same if you change this line to: > > > > for tab_index in xrange(tab_count, 0, -1): > > > > It must have been an unrelated error you saw last time, since I don't think > this > > change should cause an error; if it does, I'd like to know why :-) > Yes, I had made a mistake: if we make the change I suggested, that actually changes the values that get set in the "tab_index" variable, which is used in the next line below. Changing those values led to an invalid tab index being specified in the call to GetTab below, causing a crash. Sorry about that! http://codereview.chromium.org/7748032/diff/3005/chrome/test/functional/chrom... File chrome/test/functional/chromeos_browser.py (right): http://codereview.chromium.org/7748032/diff/3005/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:22: self.assertEqual('chrome://newtab/') Remove this entire line.
http://codereview.chromium.org/7748032/diff/3005/chrome/test/functional/chrom... File chrome/test/functional/chromeos_browser.py (right): http://codereview.chromium.org/7748032/diff/3005/chrome/test/functional/chrom... chrome/test/functional/chromeos_browser.py:22: self.assertEqual('chrome://newtab/') On 2011/08/26 02:12:45, dennis_jeffrey wrote: > Remove this entire line. Done. |
