Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8528012:
Introduce Pyauto tests for Quasar (Google Talk Extension). (Closed)
|
Created:
9 years, 1 month ago by wud Modified:
8 years, 10 months ago CC:
chromium-reviews, Nirnimesh, John Grabowski, Erik does not do reviews, mihaip+watch_chromium.org, Aaron Boodman, anantha, dyu1, Paweł Hajdan Jr., dennis_jeffrey Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
Description
Introduce Pyauto tests for Quasar (Google Talk Extension).
Not including in continous build until confident that flakiness is addressed.
(Old issue at: http://codereview.chromium.org/8477044/; having trouble re-attaching)
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 74
Patch Set 6 : '' #
Total comments: 6
Patch Set 7 : '' #
Total comments: 8
Patch Set 8 : '' #
Total comments: 2
Patch Set 9 : '' #
Messages
Total messages: 13 (0 generated)
(Note: I'm lost the old gcl change and I'm having trouble uploading to the old issue, so this is the new one with the latest changes.)
http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/gtalk_base_test.py (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:14: import random unused http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:15: import sys unused http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:16: import tempfile unused http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:17: import urllib2 unused http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:19: import pyauto_gtalk # must preceed pyauto leave another space before # http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:26: you should define _injected_js = None here. And then you don't need to override __init__() in line 257 below. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:32: os.path.join(self.DataDir(), 'extensions', 'quasar', 'quasar.crx')) change dirname to gtalk? http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:34: 'Failed to find GTalk extension') Use named params: msg='...' Also, it would be nice to include extension_path in the message http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:38: self.assertTrue(extension, 'Failed to install GTalk extension') msg='Failed to...' (repeat everywhere else) http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:42: """Uninstall the GTalk extension (if exists)""" exists -> present http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:44: if extension: self.UninstallExtensionById(extension['id']) move self.UninstallExtensionById(extension['id']) to next line http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:153: self.assertTrue(self.WaitUntil( This is better written as: self.assertTrue(self.WaitUntil(func, expect_retval=result), msg=msg) The difference compared to lambda: result == func() is that using expect_retval will produce nicer logs like: Got value XYZ. Expecting ABC. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:163: """ assert callable(matches) http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:176: return 'window.domAutomationController.send(' + \ this will look cleaner as a heredoc. return """ window.domAutomationController.send( (function(){... ... """ % (arg1, arg2) http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:202: except pyauto_error.JSONInterfaceError as e: It's not a good idea to catch this. This represents that something really went wrong (like malformed javascript) -- and in most cases waits for ~30 secs before yielding. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:216: if not isinstance(value, basestring): not necessary. out = str(value) will work regardless http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:218: out = out[:300] 218-220 can be: re.sub('\s', '', out[:300]) Also, you should probably use something else as the replacement string. maybe ';'? http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:238: tab['windex'] = win['index'] what is this for? http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:246: if self._injected_js == None: s/==/is/ http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:247: jsfile = open(os.path.dirname(__file__) + '/jsutils.js') 247-250 can be written as: self._injected_js = open(os.path.dirname(__file__) + '/jsutils.js').read() http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:253: def action_max_timeout_ms(self): what is this for? http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/jsutils.js (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/jsutils.js:6: * @fileoverview JS utilities automatically injected by Quasar PyAuto tests. s/Quasar/GTalk/ http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/pyauto_gtalk.py (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/pyauto_gtalk.py:8: Copied from chrome/test/functional/media/pyauto_media.py. Remove this line. It's not a plain copy anymore. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/pyauto_gtalk.py:13: import tempfile unused http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/test_basic.py (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:20: import pyauto_gtalk # must preceed pyauto need another space before # http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:24: class TestInstall(gtalk_base_test.GTalkBaseTest): Per convention, class name should end with Test InstallTest http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:44: extension_id = extension['id'], no spaces needed around = when passing named args http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:56: # TODO(wud): replace after click BA icon is supported. Reference crbug.com/97342 http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:62: self.assertTrue(self.WaitUntil(lambda: bool(self.GetViewerInfo())), I don't think casting to bool is necessary WaitUntil(self.GetViewerInfo, ..) should do fine http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:271: """Run tests for basic functionality in GTalk with retries.""" why retry? http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:290: logging.info('Retring...') Retrying http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:293: leave another blank line here
http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/gtalk_base_test.py (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:185: tab: The data object for the tab. more descriptive variable name. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:193: logging.debug("Tab not found: %s" % tab) Be consistent with quotes (All ' or all ") http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/jsutils.js (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/jsutils.js:13: Please add comments describing what these functions do. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/pyauto_gtalk.py (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/pyauto_gtalk.py:23: Two blank lines between top-level definition
Sorry for all the style issues; thanks for the teachings! http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/gtalk_base_test.py (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:14: import random On 2011/11/11 01:01:21, Nirnimesh wrote: > unused Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:15: import sys On 2011/11/11 01:01:21, Nirnimesh wrote: > unused Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:16: import tempfile On 2011/11/11 01:01:21, Nirnimesh wrote: > unused Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:17: import urllib2 On 2011/11/11 01:01:21, Nirnimesh wrote: > unused Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:19: import pyauto_gtalk # must preceed pyauto On 2011/11/11 01:01:21, Nirnimesh wrote: > leave another space before # Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:26: On 2011/11/11 01:01:21, Nirnimesh wrote: > you should define > _injected_js = None > > here. And then you don't need to override __init__() in line 257 below. Ah, thanks! http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:32: os.path.join(self.DataDir(), 'extensions', 'quasar', 'quasar.crx')) On 2011/11/11 01:01:21, Nirnimesh wrote: > change dirname to gtalk? Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:34: 'Failed to find GTalk extension') On 2011/11/11 01:01:21, Nirnimesh wrote: > Use named params: msg='...' > Also, it would be nice to include extension_path in the message Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:38: self.assertTrue(extension, 'Failed to install GTalk extension') On 2011/11/11 01:01:21, Nirnimesh wrote: > msg='Failed to...' > (repeat everywhere else) Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:42: """Uninstall the GTalk extension (if exists)""" On 2011/11/11 01:01:21, Nirnimesh wrote: > exists -> present Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:44: if extension: self.UninstallExtensionById(extension['id']) On 2011/11/11 01:01:21, Nirnimesh wrote: > move self.UninstallExtensionById(extension['id']) to next line Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:153: self.assertTrue(self.WaitUntil( On 2011/11/11 01:01:21, Nirnimesh wrote: > This is better written as: > self.assertTrue(self.WaitUntil(func, expect_retval=result), msg=msg) > > The difference compared to lambda: result == func() is that using expect_retval > will produce nicer logs like: > > Got value XYZ. Expecting ABC. Thanks. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:163: """ On 2011/11/11 01:01:21, Nirnimesh wrote: > assert callable(matches) Done. Also added for func. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:176: return 'window.domAutomationController.send(' + \ On 2011/11/11 01:01:21, Nirnimesh wrote: > this will look cleaner as a heredoc. > > return """ > window.domAutomationController.send( > (function(){... > ... > """ % (arg1, arg2) Thanks. Didn't know about heredoc. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:185: tab: The data object for the tab. On 2011/11/11 01:35:03, frankf wrote: > more descriptive variable name. Updated description. The variable name "tab", refers to the same used in "ExecuteJavascriptInTab"... what would you suggest? http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:193: logging.debug("Tab not found: %s" % tab) On 2011/11/11 01:35:03, frankf wrote: > Be consistent with quotes (All ' or all ") Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:202: except pyauto_error.JSONInterfaceError as e: On 2011/11/11 01:01:21, Nirnimesh wrote: > It's not a good idea to catch this. This represents that something really went > wrong (like malformed javascript) -- and in most cases waits for ~30 secs before > yielding. In some cases (e.g., I believe when an expected element doesn't exist yet). Since I use "WaitUntil" in conjunction with javascript calls, this is currently necessary to prevent the test from failing prematurely in a number of situations. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:216: if not isinstance(value, basestring): On 2011/11/11 01:01:21, Nirnimesh wrote: > not necessary. out = str(value) will work regardless Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:218: out = out[:300] On 2011/11/11 01:01:21, Nirnimesh wrote: > 218-220 can be: > > re.sub('\s', '', out[:300]) > Also, you should probably use something else as the replacement string. maybe > ';'? Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:238: tab['windex'] = win['index'] On 2011/11/11 01:01:21, Nirnimesh wrote: > what is this for? A reference to the windex used in _RunInTab. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:246: if self._injected_js == None: On 2011/11/11 01:01:21, Nirnimesh wrote: > s/==/is/ Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:247: jsfile = open(os.path.dirname(__file__) + '/jsutils.js') On 2011/11/11 01:01:21, Nirnimesh wrote: > 247-250 can be written as: > > self._injected_js = open(os.path.dirname(__file__) + '/jsutils.js').read() Thanks, done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:253: def action_max_timeout_ms(self): On 2011/11/11 01:01:21, Nirnimesh wrote: > what is this for? This is the default timeout used by WaitUntil. Setting this to 30000 makes the test not seem to loop forever for me. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/jsutils.js (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/jsutils.js:6: * @fileoverview JS utilities automatically injected by Quasar PyAuto tests. On 2011/11/11 01:01:21, Nirnimesh wrote: > s/Quasar/GTalk/ Thanks, done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/jsutils.js:13: On 2011/11/11 01:35:03, frankf wrote: > Please add comments describing what these functions do. Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/pyauto_gtalk.py (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/pyauto_gtalk.py:8: Copied from chrome/test/functional/media/pyauto_media.py. On 2011/11/11 01:01:21, Nirnimesh wrote: > Remove this line. It's not a plain copy anymore. Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/pyauto_gtalk.py:13: import tempfile On 2011/11/11 01:01:21, Nirnimesh wrote: > unused Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/pyauto_gtalk.py:23: On 2011/11/11 01:35:03, frankf wrote: > Two blank lines between top-level definition Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/test_basic.py (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:20: import pyauto_gtalk # must preceed pyauto On 2011/11/11 01:01:21, Nirnimesh wrote: > need another space before # Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:24: class TestInstall(gtalk_base_test.GTalkBaseTest): On 2011/11/11 01:01:21, Nirnimesh wrote: > Per convention, class name should end with Test > InstallTest Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:44: extension_id = extension['id'], On 2011/11/11 01:01:21, Nirnimesh wrote: > no spaces needed around = when passing named args Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:56: # TODO(wud): replace after click BA icon is supported. On 2011/11/11 01:01:21, Nirnimesh wrote: > Reference crbug.com/97342 Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:62: self.assertTrue(self.WaitUntil(lambda: bool(self.GetViewerInfo())), On 2011/11/11 01:01:21, Nirnimesh wrote: > I don't think casting to bool is necessary > WaitUntil(self.GetViewerInfo, ..) should do fine Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:271: """Run tests for basic functionality in GTalk with retries.""" On 2011/11/11 01:01:21, Nirnimesh wrote: > why retry? To mitigate flakiness due to network issues. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:290: logging.info('Retring...') On 2011/11/11 01:01:21, Nirnimesh wrote: > Retrying Done. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/test_basic.py:293: On 2011/11/11 01:01:21, Nirnimesh wrote: > leave another blank line here Done.
I'm sorry this CL somehow slipped through the cracks. I have a couple of comments only. Thanks http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/gtalk_base_test.py (right): http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:202: except pyauto_error.JSONInterfaceError as e: On 2011/11/17 21:17:53, wud wrote: > On 2011/11/11 01:01:21, Nirnimesh wrote: > > It's not a good idea to catch this. This represents that something really went > > wrong (like malformed javascript) -- and in most cases waits for ~30 secs > before > > yielding. > > In some cases (e.g., I believe when an expected element doesn't exist yet). > Since I use "WaitUntil" in conjunction with javascript calls, this is currently > necessary to prevent the test from failing prematurely in a number of > situations. Such an issue is actually a symptom of js error. You could update the javascript code to first check for the dom elements before acting on them. http://codereview.chromium.org/8528012/diff/2008/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:253: def action_max_timeout_ms(self): On 2011/11/17 21:17:53, wud wrote: > On 2011/11/11 01:01:21, Nirnimesh wrote: > > what is this for? > > This is the default timeout used by WaitUntil. Setting this to 30000 makes the > test not seem to loop forever for me. I don't think you should override this. This controls the timeout for all automation calls. It defaults to ~25 secs anyway, so I don't see why a difference of 5 seconds would help you. If you want to increment the automation timeout, use ActionTimeoutChanger class. See chrome/test/functional/wifi_downloads.py for example http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/gtalk_base_test.py (right): http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:143: return None; nit: remove ; http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:205: try: I really think you should fix the javascript code to account for missing dom elements. Otherwise, every javascript error ends up causing a 25 secs delay in ExecuteJavascriptInTab() http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/pyauto_gtalk.py (right): http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... chrome/test/functional/gtalk/pyauto_gtalk.py:24: class Main(pyauto_functional.Main): This class is a no-op. You could directly use pyauto_functional.Main() in line 32.
I mistakenly commented on the old CL: http://codereview.chromium.org/8477044/. Please close this one.
Thanks! http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/gtalk_base_test.py (right): http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:143: return None; On 2011/11/30 23:57:36, Nirnimesh wrote: > nit: remove ; Done. http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... chrome/test/functional/gtalk/gtalk_base_test.py:205: try: On 2011/11/30 23:57:36, Nirnimesh wrote: > I really think you should fix the javascript code to account for missing dom > elements. > > Otherwise, every javascript error ends up causing a 25 secs delay in > ExecuteJavascriptInTab() Ok, I've removed the "except" and haven't been able to reproduce any problems yet. If they come up, I'll try to fix the JS. http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... File chrome/test/functional/gtalk/pyauto_gtalk.py (right): http://codereview.chromium.org/8528012/diff/8001/chrome/test/functional/gtalk... chrome/test/functional/gtalk/pyauto_gtalk.py:24: class Main(pyauto_functional.Main): On 2011/11/30 23:57:36, Nirnimesh wrote: > This class is a no-op. You could directly use pyauto_functional.Main() in line > 32. Ah, good point. Done.
http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... File chrome/test/functional/gtalk/pyauto_gtalk.py (right): http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... chrome/test/functional/gtalk/pyauto_gtalk.py:21: import pyauto_functional Actually, I meant: from pyauto_functional import Main if __name__ == '__main__': Main() Then test_basic.py need not know about pyauto_functional. It uses if __name__ == '__main__': pyauto_gtalk.Main() http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... File chrome/test/functional/gtalk/test_basic.py (right): http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... chrome/test/functional/gtalk/test_basic.py:20: import pyauto_functional this script need not know about pyauto_functional. Use pyauto_gtalk.Main() http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... chrome/test/functional/gtalk/test_basic.py:58: # See http://crbug.com/97342. Thanks to Frank, this issue has been fixed. (you could make the required changes in a followup CL) http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... chrome/test/functional/gtalk/test_basic.py:280: logging.info('Calling RunBasicFunctionalityTest. Try #{0}/{1}' Why not use the more commonly used form for string formatting: 'Calling RunBasicFunctionalityTest. Try #%s/%s' % (tries + 1, RETRIES)
http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... File chrome/test/functional/gtalk/pyauto_gtalk.py (right): http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... chrome/test/functional/gtalk/pyauto_gtalk.py:21: import pyauto_functional Ah, thanks! Done. On 2011/12/13 20:03:25, Nirnimesh wrote: > Actually, I meant: > > from pyauto_functional import Main > > > if __name__ == '__main__': > Main() > > > Then test_basic.py need not know about pyauto_functional. It uses > > if __name__ == '__main__': > pyauto_gtalk.Main() http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... File chrome/test/functional/gtalk/test_basic.py (right): http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... chrome/test/functional/gtalk/test_basic.py:20: import pyauto_functional On 2011/12/13 20:03:25, Nirnimesh wrote: > this script need not know about pyauto_functional. Use pyauto_gtalk.Main() Done. http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... chrome/test/functional/gtalk/test_basic.py:58: # See http://crbug.com/97342. On 2011/12/13 20:03:25, Nirnimesh wrote: > Thanks to Frank, this issue has been fixed. > (you could make the required changes in a followup CL) Thanks, updated comments. I'll take you up on your offer to punt this to a future CL. :) http://codereview.chromium.org/8528012/diff/16001/chrome/test/functional/gtal... chrome/test/functional/gtalk/test_basic.py:280: logging.info('Calling RunBasicFunctionalityTest. Try #{0}/{1}' Thanks, done. On 2011/12/13 20:03:25, Nirnimesh wrote: > Why not use the more commonly used form for string formatting: > > 'Calling RunBasicFunctionalityTest. Try #%s/%s' % (tries + 1, RETRIES)
LGTM http://codereview.chromium.org/8528012/diff/21001/chrome/test/functional/gtal... File chrome/test/functional/gtalk/gtalk_base_test.py (right): http://codereview.chromium.org/8528012/diff/21001/chrome/test/functional/gtal... chrome/test/functional/gtalk/gtalk_base_test.py:254: return 30000; nit: remove ;
Thanks for the review and for all the teachings! :) http://codereview.chromium.org/8528012/diff/21001/chrome/test/functional/gtal... File chrome/test/functional/gtalk/gtalk_base_test.py (right): http://codereview.chromium.org/8528012/diff/21001/chrome/test/functional/gtal... chrome/test/functional/gtalk/gtalk_base_test.py:254: return 30000; On 2011/12/13 21:06:40, Nirnimesh wrote: > nit: remove ; Done.
Please take a look at the comments I made in the old CL, for example we don't need to hack around browser actions anymore.
On 2011/12/13 22:00:20, frankf wrote:
> Please take a look at the comments I made in the old CL, for example we don't
> need to hack around browser actions anymore.
Thanks; addressed all comments except for using the BA icon.
For that, I seem to having some trouble getting the latest code ("gclient sync"
doesn't give me a version containing your "TriggerBrowserActionById" code).
I'm out for a couple weeks, so I'll take a look in January.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
