Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8477044:
Introduce Pyauto tests for Quasar (Google Talk Extension). (Closed)
|
Created:
9 years, 1 month ago by wud Modified:
9 years ago CC:
chromium-reviews, Nirnimesh, John Grabowski, anantha, dyu1, Paweł Hajdan Jr., dennis_jeffrey Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionIntroduce Pyauto tests for Quasar (Google Talk Extension).
Not including in continous build until confident that flakiness is addressed.
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 87
Messages
Total messages: 7 (0 generated)
@wud. FYI, you need to publish so that the codereview email gets sent out.
Ah, thank you. Published. On Wed, Nov 9, 2011 at 2:19 PM, <nirnimesh@chromium.org> wrote: > @wud. FYI, you need to publish so that the codereview email gets sent out. > > http://codereview.chromium.**org/8477044/<http://codereview.chromium.org/8477... >
I haven't reviewed the complete code yet but I wanted to send the comments I have so far. 1. It would be better to find someone on quasar time to review the js file 2. I've pointed out several python style issues. Please apply them throughout the CL Thanks http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/jsutils.js (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/jsutils.js:1: // Copyright 2011 Google Inc. All Rights Reserved. Switch to Chromium header http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/pyauto_quasar.py (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/pyauto_quasar.py:7: Setup for Quasar Pyauto tests. Move this to previous line. (then leave a blank line) Then the rest http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/pyauto_quasar.py:8: Copied from chrome/test/funcational/media/pyauto_media.py. funcational -> functional http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/pyauto_quasar.py:21: sys.path.append(os.path.normpath(os.path.join( You don't need anything below this http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/pyauto_quasar.py:37: import pyauto unused http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/quasar_base_test.py (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:20: need 2 blank lines at global scope http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:21: class BaseTest(pyauto.PyUITest): BaseTest is too generic. Rename to QuasarBaseTest http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:28: url = 'http://www.corp.google.com/~wud/pyauto/quasar.crx' It might be better to checkin the extension somewhere in chrome/test/data, so that it's versioned. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:40: def Uninstall(self): I recommend renaming to make it explicit what is getting uninstalled. UninstallGTalkExtension or UninstallQuasarExtension http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:43: if extension: merge with previous line http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:46: def GetExtensionInfo(self): This sounds very close to the pyauto call GetExtensionsInfo() Rename to GetQuasarExtInfo? http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:48: info = None 48-54 can be written as: extensions = [x for x in self.GetExtensionsInfo() if x['name'] == 'Google Talk'] assert len(extensions) == 1, 'Got more than one Google Talk extension' return extensions[0] http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:51: self.assertTrue(info == None, are you sure you want to assert in this getter? If you do, use python's assert keyword. self.assertTrue comes from the python unittest module and it's used to signify test failure. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:54: return info; remove ; http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:56: def RunInMole(self, js, mole_index = 0): remove space around = (Repeat elsewhere) http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:57: """Execute javascript in a chat mole.""" Explain args http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:110: return self.assertTrue(self.WaitUntil( assertTrue does not return You want to return the output of WaitUntil, in which case you could directly use WaitUntil instead. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:123: print "Tab not found: %s" % tab use logging.info / logging.debug instead of print (Repeat elsewhere) http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:126: try: WHy? http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:144: print js + " ===> " + out.encode("utf-8") prefer ' over " (Repeat elsewhere) http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:146: def _GetTabInfo(self, url_query, index = 0): rename |index| to |windex| (window index) http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:150: for win in windows: you can directly get all tabs for the given window windows[index]['tabs'] http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:154: if (i == index): remove parens http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:157: return None; remove ; http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:159: def _DownloadFile(self, url, filename): this is the same as urllib.urlretrieve http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:160: """Download a file.""" Explain args (Repeat elsewhere for all non-trivial methods & functions) http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:161: # NOTE(wud): Using the pyauto download file API currently produces a NOTE(wud) -> Note http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:173: jsfile = open(os.path.dirname(__file__) + '/jsutils.js', 'r') the 2nd arg is redundant http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:184: self.injectedJs_ = None use varnames_with_underscores Prefix _ http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/test_basic.py (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:7: This module contains the basic set of sanity tests run on the Break this up into one-line of title (then a blank line) then more description http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:8: Quasar extension (http://go/quasar). Create a page on dev.chromium.org and link it here instead? http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:28: def Debug(self, text): Since this blocks, rename to something clearer? Prompt? http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:30: if not isinstance(text, basestring): I don't think this check is necessary. text = str(text) will work either way http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:37: # Download and install the extension. comment is redundant http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:46: 'Failed to get background view: views = %s.' % it's customary to use named args wherever possible in this case msg='Failed to get ..' (Repeat this for all self.assertTrue calls) http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:57: self.assertTrue('ok' == result, use assertEqual('ok', result, msg='...')
Haven't finished reviewing yet... In general, it's good practice to have all the JS code in the extension itself not some generic utility functions. By making all test dependencies explicit on the JS side (instead of referring to some obscure code embedded in python files), it will be much easier to modify the extension without breaking the tests. It also makes the python side cleaner. (Obviously there are exceptions such a hardcoded username/password). http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/test_basic.py (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:17: import tempfile not alpha. Please run gpylint. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:55: # TODO(wud): replace after click BA icon is supported. Is there a reason you don't start the app from the NTP page? http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:143: time.sleep(1) Fix? http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:284: exc_type, exc_value, exc_traceback = sys.exc_info() Can you verify this doesn't cause a circular reference? http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:289: time.sleep(5) Why is sleep required here? Are expecting transient server/network issues?
Thanks for the initial review-- and for teaching me so much about Python style. I moved the files to "gtalk/" instead of "quasar" for clarity, but I'm having trouble re-uploading to this issue, so the new CL is currently at: http://codereview.chromium.org/8528012/ http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/jsutils.js (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/jsutils.js:1: // Copyright 2011 Google Inc. All Rights Reserved. On 2011/11/09 23:54:05, Nirnimesh wrote: > Switch to Chromium header Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/pyauto_quasar.py (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/pyauto_quasar.py:7: Setup for Quasar Pyauto tests. On 2011/11/09 23:54:05, Nirnimesh wrote: > Move this to previous line. > (then leave a blank line) > Then the rest Done, here and elsewhere. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/pyauto_quasar.py:8: Copied from chrome/test/funcational/media/pyauto_media.py. On 2011/11/09 23:54:05, Nirnimesh wrote: > funcational -> functional Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/pyauto_quasar.py:21: sys.path.append(os.path.normpath(os.path.join( On 2011/11/09 23:54:05, Nirnimesh wrote: > You don't need anything below this Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/pyauto_quasar.py:37: import pyauto On 2011/11/09 23:54:05, Nirnimesh wrote: > unused Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/quasar_base_test.py (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:20: On 2011/11/09 23:54:05, Nirnimesh wrote: > need 2 blank lines at global scope Done, here and elsewhere. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:21: class BaseTest(pyauto.PyUITest): On 2011/11/09 23:54:05, Nirnimesh wrote: > BaseTest is too generic. Rename to QuasarBaseTest Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:28: url = 'http://www.corp.google.com/~wud/pyauto/quasar.crx' On 2011/11/09 23:54:05, Nirnimesh wrote: > It might be better to checkin the extension somewhere in chrome/test/data, so > that it's versioned. Good idea! Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:40: def Uninstall(self): On 2011/11/09 23:54:05, Nirnimesh wrote: > I recommend renaming to make it explicit what is getting uninstalled. > UninstallGTalkExtension or UninstallQuasarExtension Done. Ended up renaming everything to "GTalk" instead of "Quasar" for clarity. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:40: def Uninstall(self): On 2011/11/09 23:54:05, Nirnimesh wrote: > I recommend renaming to make it explicit what is getting uninstalled. > UninstallGTalkExtension or UninstallQuasarExtension Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:43: if extension: On 2011/11/09 23:54:05, Nirnimesh wrote: > merge with previous line Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:46: def GetExtensionInfo(self): On 2011/11/09 23:54:05, Nirnimesh wrote: > This sounds very close to the pyauto call GetExtensionsInfo() > Rename to GetQuasarExtInfo? Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:48: info = None On 2011/11/09 23:54:05, Nirnimesh wrote: > 48-54 can be written as: > > extensions = [x for x in self.GetExtensionsInfo() if x['name'] == 'Google Talk'] > assert len(extensions) == 1, 'Got more than one Google Talk extension' > return extensions[0] Thanks, much cleaner! http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:51: self.assertTrue(info == None, On 2011/11/09 23:54:05, Nirnimesh wrote: > are you sure you want to assert in this getter? > > If you do, use python's assert keyword. > self.assertTrue comes from the python unittest module and it's used to signify > test failure. Just want to ensure at most one Talk extension is running at any time. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:54: return info; On 2011/11/09 23:54:05, Nirnimesh wrote: > remove ; Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:56: def RunInMole(self, js, mole_index = 0): On 2011/11/09 23:54:05, Nirnimesh wrote: > remove space around = > (Repeat elsewhere) Done. Here and elsewhere. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:57: """Execute javascript in a chat mole.""" On 2011/11/09 23:54:05, Nirnimesh wrote: > Explain args Done. Here and elsewhere. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:110: return self.assertTrue(self.WaitUntil( On 2011/11/09 23:54:05, Nirnimesh wrote: > assertTrue does not return > You want to return the output of WaitUntil, in which case you could directly use > WaitUntil instead. Removed return value. It felt cleaner to me to use this helper method to avoid defining matches outside the WaitUntilCondition call (with just WaitUntil, is there a clean way to do this?): # Wait for the roster iframe to load. self.WaitUntilCondition( lambda: self.RunInRoster('window.location.href'), lambda url: url and '/notifierclient' in url, 'Timed out waiting for /notifierclient url') http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:123: print "Tab not found: %s" % tab On 2011/11/09 23:54:05, Nirnimesh wrote: > use logging.info / logging.debug instead of print > (Repeat elsewhere) Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:126: try: On 2011/11/09 23:54:05, Nirnimesh wrote: > WHy? Sometimes, the test would throw a JSONInterfaceError and fail for JS calls within "WaitUntil" expected to fail initially. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:144: print js + " ===> " + out.encode("utf-8") On 2011/11/09 23:54:05, Nirnimesh wrote: > prefer ' over " > (Repeat elsewhere) Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:146: def _GetTabInfo(self, url_query, index = 0): On 2011/11/09 23:54:05, Nirnimesh wrote: > rename |index| to |windex| (window index) actually, i don't think "index" corresponds to the window index here-- it corresponds to index of the array of matching tabs that have "url_query" has a substring. Updated doc. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:150: for win in windows: On 2011/11/09 23:54:05, Nirnimesh wrote: > you can directly get all tabs for the given window > windows[index]['tabs'] Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:154: if (i == index): On 2011/11/09 23:54:05, Nirnimesh wrote: > remove parens Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:157: return None; On 2011/11/09 23:54:05, Nirnimesh wrote: > remove ; Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:159: def _DownloadFile(self, url, filename): On 2011/11/09 23:54:05, Nirnimesh wrote: > this is the same as urllib.urlretrieve removed. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:160: """Download a file.""" On 2011/11/09 23:54:05, Nirnimesh wrote: > Explain args > (Repeat elsewhere for all non-trivial methods & functions) Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:161: # NOTE(wud): Using the pyauto download file API currently produces a On 2011/11/09 23:54:05, Nirnimesh wrote: > NOTE(wud) -> Note removed. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:173: jsfile = open(os.path.dirname(__file__) + '/jsutils.js', 'r') On 2011/11/09 23:54:05, Nirnimesh wrote: > the 2nd arg is redundant Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:184: self.injectedJs_ = None On 2011/11/09 23:54:05, Nirnimesh wrote: > use varnames_with_underscores > Prefix _ Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/test_basic.py (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:7: This module contains the basic set of sanity tests run on the On 2011/11/09 23:54:05, Nirnimesh wrote: > Break this up into one-line of title > (then a blank line) > then more description Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:8: Quasar extension (http://go/quasar). On 2011/11/09 23:54:05, Nirnimesh wrote: > Create a page on http://dev.chromium.org and link it here instead? Linked to public extension page instead. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:17: import tempfile On 2011/11/10 00:20:18, frankf wrote: > not alpha. Please run gpylint. Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:28: def Debug(self, text): On 2011/11/09 23:54:05, Nirnimesh wrote: > Since this blocks, rename to something clearer? > Prompt? Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:30: if not isinstance(text, basestring): On 2011/11/09 23:54:05, Nirnimesh wrote: > I don't think this check is necessary. > text = str(text) will work either way good point. :) http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:37: # Download and install the extension. On 2011/11/09 23:54:05, Nirnimesh wrote: > comment is redundant Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:55: # TODO(wud): replace after click BA icon is supported. On 2011/11/10 00:20:18, frankf wrote: > Is there a reason you don't start the app from the NTP page? Our extension is not on the new tab page. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:57: self.assertTrue('ok' == result, On 2011/11/09 23:54:05, Nirnimesh wrote: > use assertEqual('ok', result, msg='...') Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:143: time.sleep(1) On 2011/11/10 00:20:18, frankf wrote: > Fix? Still not yet. This doesn't seem to happen outside of PyAuto. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:284: exc_type, exc_value, exc_traceback = sys.exc_info() On 2011/11/10 00:20:18, frankf wrote: > Can you verify this doesn't cause a circular reference? Done. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:289: time.sleep(5) On 2011/11/10 00:20:18, frankf wrote: > Why is sleep required here? Are expecting transient server/network issues? Not really, removed.
http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/jsutils.js (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/jsutils.js:12: $VIEW = function(query) { Please document these functions. It goes a long way for anyone maintaining it later. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/quasar_base_test.py (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:33: 'Failed to download Quasar extension') nit: period at end of msg, also in other places. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:37: self.assertTrue(extension, 'Failed to install Quasar extension') You no longer need to explicitly test for this as the verification is done in GetExtensionInfo. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/quasar_base_test.py:139: if not isinstance(value, basestring): don't need the if statement. http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... File chrome/test/functional/quasar/test_basic.py (right): http://codereview.chromium.org/8477044/diff/6007/chrome/test/functional/quasa... chrome/test/functional/quasar/test_basic.py:55: # TODO(wud): replace after click BA icon is supported. Browser action hook is now implemented: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/functional/extens... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
