Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10821015:
Initial checkin of the me2me pyauto automation: (Closed)
|
Created:
8 years, 5 months ago by yihongg1 Modified:
8 years, 4 months ago CC:
chromium-reviews, dennis_jeffrey, anantha, dyu1, Nirnimesh Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionInitial checkin of the me2me pyauto automation
- Modified chromoting.py to enable me2me automation
- Added chromoting_helper.py to handle install, uninstall, enable, disable, changepin which requires admin privilege
- Added chromoting.base.py as the base for all chromoting test cases
- Modified chromoting_basic, renamed it it2me_basic and moved it under chromoting dir
- Added auth, me2me_enable, me2me_connect test
- Added a cert and a private key for signing host on mac
- Added mock_pref_pane.* files to mock the pref pane for different scenarios
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151717
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151760
Patch Set 1 #
Total comments: 28
Patch Set 2 : #Patch Set 3 : #
Total comments: 53
Patch Set 4 : #
Total comments: 55
Patch Set 5 : #
Total comments: 25
Patch Set 6 : #Patch Set 7 : #Messages
Total messages: 19 (0 generated)
LGTM for basic chromoting functionality. +garykac for the Mac prefpane code. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/PYAUTO_... File chrome/test/functional/PYAUTO_TESTS (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/PYAUTO_... chrome/test/functional/PYAUTO_TESTS:288: '-chromoting.it2me_basic', Should this line be '-chromoting', to block all the chromoting tests? My guess is that none of them will run if the chromoting host won't run. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/PYAUTO_... chrome/test/functional/PYAUTO_TESTS:485: '-chromoting.it2me_basic', Just '-chromoting'? http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/auth.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/auth.py:8: os.sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) Add a comment explaining what this does, and why it appears in the middle of the import list. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/chromoting_base.py:12: """Chromoting pyatuo test base class.""" pyatuo -> pyauto http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/it2me_basic.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/it2me_basic.py:8: os.sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) Add a comment explaining this line. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/me2me_connect.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/me2me_connect.py:27: self.host.GetMe2MeStarted() Maybe "StartMe2Me", for consistency with Connect/Disconnect/Reconnect Me2Me? http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... File chrome/test/pyautolib/chromoting.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting.py:38: Returns: True or False if condition not satisfied. It's not clear whether True means that the condition is or isn't satisfied. Maybe something like: Returns: True if the condition is not satisfied. http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting.py:256: # Mis-mtaching pins mtaching -> matching
http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/mock_pref_pane_enable (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/mock_pref_pane_enable:11: SERVICE_NAME = "org.chromium.chromoting" This is pretty much exactly the same as mock_pref_pane_disable and it shares some code with _changepin as well. Can this code be shared so that future bugfixes only need to be made in one place?
I took a look at this CL. There's a lot of python style issues. Please go through the notes for python style at http://dev.chromium.org/developers/coding-style and apply to this CL. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/PYAUTO_... File chrome/test/functional/PYAUTO_TESTS (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/PYAUTO_... chrome/test/functional/PYAUTO_TESTS:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. Remove ":" from the CL's title http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/auth.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/auth.py:15: Need 2 lines at global scope http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... File chrome/test/pyautolib/chromoting.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting.py:44: """ Executes JavaScript and wait for remoting app mode equal to the given mode. 80 chars http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting.py:54: """ Executes JavaScript and wait for remoting app major mode equal to the given mode. 80 chars http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... File chrome/test/pyautolib/chromoting_helper.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting_helper.py:14: chromoting_test_dir = "chrome/test/functional/chromoting" use os.path.join
Thanks for reviewing the code! Fixed the generic coding style issues, combined mock_pref_pane code based on Gary's suggestion, and fixed other specific comments. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/PYAUTO_... File chrome/test/functional/PYAUTO_TESTS (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/PYAUTO_... chrome/test/functional/PYAUTO_TESTS:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/07/31 20:15:10, Nirnimesh wrote: > Remove ":" from the CL's title Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/PYAUTO_... chrome/test/functional/PYAUTO_TESTS:288: '-chromoting.it2me_basic', We are only enabling chromoting.it2me_basic on continuous right now. This is to disable that test on continuous and chromeos. We don't need to include others because we didn't add those in the first place anyway. On 2012/07/30 22:44:24, simonmorris wrote: > Should this line be '-chromoting', to block all the chromoting tests? My guess > is that none of them will run if the chromoting host won't run. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/PYAUTO_... chrome/test/functional/PYAUTO_TESTS:485: '-chromoting.it2me_basic', Same as above explanation. On 2012/07/30 22:44:24, simonmorris wrote: > Just '-chromoting'? http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/auth.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/auth.py:8: os.sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) On 2012/07/30 22:44:24, simonmorris wrote: > Add a comment explaining what this does, and why it appears in the middle of the > import list. Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/auth.py:15: On 2012/07/31 20:15:10, Nirnimesh wrote: > Need 2 lines at global scope Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/chromoting_base.py:12: """Chromoting pyatuo test base class.""" On 2012/07/30 22:44:24, simonmorris wrote: > pyatuo -> pyauto Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/it2me_basic.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/it2me_basic.py:8: os.sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) On 2012/07/30 22:44:24, simonmorris wrote: > Add a comment explaining this line. Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/me2me_connect.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/me2me_connect.py:27: self.host.GetMe2MeStarted() On 2012/07/30 22:44:24, simonmorris wrote: > Maybe "StartMe2Me", for consistency with Connect/Disconnect/Reconnect Me2Me? Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... File chrome/test/functional/chromoting/mock_pref_pane_enable (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/functional/chromot... chrome/test/functional/chromoting/mock_pref_pane_enable:11: SERVICE_NAME = "org.chromium.chromoting" On 2012/07/31 00:25:11, garykac wrote: > This is pretty much exactly the same as mock_pref_pane_disable and it shares > some code with _changepin as well. > > Can this code be shared so that future bugfixes only need to be made in one > place? Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... File chrome/test/pyautolib/chromoting.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting.py:38: Returns: True or False if condition not satisfied. On 2012/07/30 22:44:24, simonmorris wrote: > It's not clear whether True means that the condition is or isn't satisfied. > Maybe something like: > > Returns: True if the condition is not satisfied. Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting.py:44: """ Executes JavaScript and wait for remoting app mode equal to the given mode. On 2012/07/31 20:15:10, Nirnimesh wrote: > 80 chars Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting.py:54: """ Executes JavaScript and wait for remoting app major mode equal to the given mode. On 2012/07/31 20:15:10, Nirnimesh wrote: > 80 chars Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting.py:256: # Mis-mtaching pins On 2012/07/30 22:44:24, simonmorris wrote: > mtaching -> matching Done. http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... File chrome/test/pyautolib/chromoting_helper.py (right): http://codereview.chromium.org/10821015/diff/1/chrome/test/pyautolib/chromoti... chrome/test/pyautolib/chromoting_helper.py:14: chromoting_test_dir = "chrome/test/functional/chromoting" On 2012/07/31 20:15:10, Nirnimesh wrote: > use os.path.join Done.
More coding style fixes.
I've pointed out some common design/style problems. Please fix them throughout the CL since I've only pointed them out once. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... File chrome/test/functional/chromoting/auth.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/auth.py:47: need 2 lines at global scope http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:1: #!/usr/bin/env python This file need not be executable http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:18: """Ensures Chrome is launched with some custom flags. Replace this placeholder string with description of what you're doing here; indicate why it's necessary for chromoting tests. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:22: remove blank line http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:28: super(ChromotingBase, self).setUp() prefer pyauto.PyUITest.setUp(self) for consistency http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:30: self.client_local = (self.remote == None) are these vars used in child classes? If so declare them in the class. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... File chrome/test/functional/chromoting/me2me_enable.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_enable.py:9: os.sys.path.insert( This should be done in chromoting_base.py Look at how gtalk tests work (chrome/test/functional/gtalk) http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_enable.py:44: pyauto_functional.Main() If you follow my comment above, this will change to sys.exit(chromoting_base.Main()) http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... File chrome/test/functional/chromoting/mock_pref_pane.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:16: _SERVICE_NAME = "org.chromium.chromoting" use ' instead of " for consistency here and everywhere. Do not use globals. Move it inside the right functions. Use classes if necessary. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:42: tool_script = "/Library/PrivilegedHelperTools/%s.me2me.sh" % _SERVICE_NAME This is for mac only. what about other platforms? Either make it work for all platforms, or clearly call out so and make placeholder for improvements for other platforms. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:71: for i in range(1, 10): i -> _ since i is unused http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:75: time.sleep(2) Sleeping is not allowed, since it makes tests flaky (often on a slow machine). Wait for the right event instead. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:93: time.sleep(10) what is this sleep for? http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:95: return 0 remove http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting.py:11: def _GetHelperRunner(): Is there a reason these functions have to be global to this file? Can they be inside the class? http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting_helper.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:4: Docstring for this script please http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:12: pref_pane_dir = os.path.join("/Library", "PreferencePanes") This applies to Mac only. Clearly call out so. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:16: mock_pref_pane_python = os.path.join(os.getcwd(), "chrome", "test", chrome/test/pyautolib should not refer to chrome/test/functional. This indicates a layering violation http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:34: def main(): main() -> Main() main is unusually long. Please refactor it to a set of classes per platform and a base abstract class. The derived classes carry the implementation for that platform. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:35: """Chromoting helper script to install/uninstall/enable/disable host.""" on all platforms? http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:36: if sys.argv[1] == "install": The flow is too difficult to follow. Please refer to the refactoring comment in line 34 http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:133: print("Nothing to be done for enable") remove parens use ' instead of " throughout this file and all other files http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:135: print >> sys.stderr, ( remove space after >> http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:135: print >> sys.stderr, ( remove parens http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:136: "Invalid syntax") move to previous line
Thanks a lot for the detailed comments! http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... File chrome/test/functional/chromoting/auth.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/auth.py:47: On 2012/08/06 19:13:53, Nirnimesh wrote: > need 2 lines at global scope Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:1: #!/usr/bin/env python On 2012/08/06 19:13:53, Nirnimesh wrote: > This file need not be executable Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:18: """Ensures Chrome is launched with some custom flags. On 2012/08/06 19:13:53, Nirnimesh wrote: > Replace this placeholder string with description of what you're doing here; > indicate why it's necessary for chromoting tests. Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:22: On 2012/08/06 19:13:53, Nirnimesh wrote: > remove blank line Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:28: super(ChromotingBase, self).setUp() On 2012/08/06 19:13:53, Nirnimesh wrote: > prefer pyauto.PyUITest.setUp(self) > for consistency Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:30: self.client_local = (self.remote == None) On 2012/08/06 19:13:53, Nirnimesh wrote: > are these vars used in child classes? If so declare them in the class. Made change to put instance variables in __init__ http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... File chrome/test/functional/chromoting/me2me_enable.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_enable.py:9: os.sys.path.insert( On 2012/08/06 19:13:53, Nirnimesh wrote: > This should be done in chromoting_base.py > > Look at how gtalk tests work (chrome/test/functional/gtalk) Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_enable.py:44: pyauto_functional.Main() On 2012/08/06 19:13:53, Nirnimesh wrote: > If you follow my comment above, this will change to > sys.exit(chromoting_base.Main()) Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... File chrome/test/functional/chromoting/mock_pref_pane.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:16: _SERVICE_NAME = "org.chromium.chromoting" On 2012/08/06 19:13:53, Nirnimesh wrote: > use ' instead of " for consistency here and everywhere. > > Do not use globals. Move it inside the right functions. Use classes if > necessary. Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:42: tool_script = "/Library/PrivilegedHelperTools/%s.me2me.sh" % _SERVICE_NAME On 2012/08/06 19:13:53, Nirnimesh wrote: > This is for mac only. what about other platforms? > > Either make it work for all platforms, or clearly call out so and make This is mac only. > placeholder for improvements for other platforms. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:71: for i in range(1, 10): On 2012/08/06 19:13:53, Nirnimesh wrote: > i -> _ > since i is unused Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:75: time.sleep(2) On 2012/08/06 19:13:53, Nirnimesh wrote: > Sleeping is not allowed, since it makes tests flaky > (often on a slow machine). Wait for the right event instead. Agree sleep will cause flaky test in general. For this specific case, sleep here is to giving some time until the next get job id call. There is no event that we can wait on for this. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:93: time.sleep(10) On 2012/08/06 19:13:53, Nirnimesh wrote: > what is this sleep for? This is put here to wait until webapp gets notification originally. Tried the test without this and it is working. So I am removing this wait. http://codereview.chromium.org/10821015/diff/10002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:95: return 0 On 2012/08/06 19:13:53, Nirnimesh wrote: > remove Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting.py:11: def _GetHelperRunner(): On 2012/08/06 19:13:53, Nirnimesh wrote: > Is there a reason these functions have to be global to this file? Can they be > inside the class? Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting_helper.py (right): http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:4: On 2012/08/06 19:13:53, Nirnimesh wrote: > Docstring for this script please Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:12: pref_pane_dir = os.path.join("/Library", "PreferencePanes") On 2012/08/06 19:13:53, Nirnimesh wrote: > This applies to Mac only. Clearly call out so. Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:16: mock_pref_pane_python = os.path.join(os.getcwd(), "chrome", "test", On 2012/08/06 19:13:53, Nirnimesh wrote: > chrome/test/pyautolib should not refer to chrome/test/functional. This indicates > a layering violation This is to construct a bash script which contains suid-python mock_pref_pane.py to replace the actual pref_pane script on mac. I think this is fine. Please let me know if I misunderstand anything. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:34: def main(): On 2012/08/06 19:13:53, Nirnimesh wrote: > main() -> Main() > > main is unusually long. Please refactor it to a set of classes per platform and > a base abstract class. The derived classes carry the implementation for that > platform Put the majority code in ChromotingHelper and used XXXMac and XXXWindows to differentiate things that have to be done on different platforms. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:35: """Chromoting helper script to install/uninstall/enable/disable host.""" On 2012/08/06 19:13:53, Nirnimesh wrote: > on all platforms? Currently mainly mac and windows. install/uninstall/replace pref pane apply on mac. install/uninstall apply on windows. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:36: if sys.argv[1] == "install": On 2012/08/06 19:13:53, Nirnimesh wrote: > The flow is too difficult to follow. Please refer to the refactoring comment in > line 34 Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:133: print("Nothing to be done for enable") On 2012/08/06 19:13:53, Nirnimesh wrote: > remove parens > use ' instead of " throughout this file and all other files Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:135: print >> sys.stderr, ( On 2012/08/06 19:13:53, Nirnimesh wrote: > remove parens Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:135: print >> sys.stderr, ( On 2012/08/06 19:13:53, Nirnimesh wrote: > remove parens Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:135: print >> sys.stderr, ( On 2012/08/06 19:13:53, Nirnimesh wrote: > remove space after >> Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:135: print >> sys.stderr, ( On 2012/08/06 19:13:53, Nirnimesh wrote: > remove space after >> Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:135: print >> sys.stderr, ( On 2012/08/06 19:13:53, Nirnimesh wrote: > remove parens Done. http://codereview.chromium.org/10821015/diff/10002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:136: "Invalid syntax") On 2012/08/06 19:13:53, Nirnimesh wrote: > move to previous line Done.
This is looking much better. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/auth.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/auth.py:9: import sys system imports should go first local imports (ie chromoting_base should go after leaving a blank line) http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:10: os.sys.path.insert( Use sys.path.append. Move it inside a local function (see chrome/test/functional/gtalk/pyauto_gtalk.py) http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:28: self.client_local = (self.remote == None) Can all this be done in setUp instead? http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:29: self.host = self Clearly declare the host and client vars in the docstring (since you expect child classes to know about it) http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:38: def setUp(self): no-op. Remove http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:41: remove spurious blank lines http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/it2me_basic.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/it2me_basic.py:9: import sys system imports go on top. Fix in all files. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/me2me_connect.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_connect.py:30: self.UninstallHostDaemon() Call parent's tearDown http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_connect.py:63: sys.exit(chromoting_base.Main()) It's probably not necessary to wrap inside sys.exit. Main() already does that a few levels up. (Sorry for misleading before) http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/me2me_enable.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_enable.py:27: self.UninstallHostDaemon() Call parent's tearDown http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_enable.py:30: """Enables remote connections, exercising different pin conditions Make it a one liner. Details can go in next para http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/mock_pref_pane.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. Rename the file to contain 'mac' in it. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:5: """Mock pref pane for testing purpose.""" Add: "on Mac" http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:16: class MockPrefPane(object): Add 'Mac' to the name http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:99: need 2 blank lines http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting.py:105: def LogIn(self, email=None, password=None, otp=None, LogIn -> SignIn to match "SignOut" below http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting.py:328: time.sleep(2) Remove sleep. This will make the test very flaky on the bots. Please figure out alternatives. Repeat for all the sleep() calls http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting_helper.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:12: class ChromotingHelper(object): Create a base class with abstract methods: Install, Uninstall, Enable, Disable, ChangePin. Create 2 child classes: ChromotingHelperMac & ChromotingHelperWin where you implement the methods. main() just needs to create an instance of the right class (based on the platform) and call Install/Uninstall... on it. This way the platform-specific logic will be contained within its class instead of being littered all over in main() http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:13: """Chromting Helper class.""" Chromoting http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:13: """Chromting Helper class.""" Mention the purpose too http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:25: os.chdir(bin_dir) Why is this necessary? http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:30: subprocess.call('rm -f -R ' + host_dir, shell=True) use shutil.rmtree http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:33: subprocess.call('unzip remoting-me2me-host-mac.zip', shell=True) make the first arg a list. Remove shell=True Repeat elsewhere http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:41: 'functional', 'chromoting') Do not reach into the functional dir. If you need the data files in pyautolib, move them to pyautolib http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:135: """Main function to dispatch operations.""" assert IsMac() or IsWin() http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:156: print 'Nothing to be done for enable' enable -> sys.argv[1]
http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/auth.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/auth.py:9: import sys On 2012/08/08 19:46:10, Nirnimesh wrote: > system imports should go first > local imports (ie chromoting_base should go after leaving a blank line) Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:10: os.sys.path.insert( On 2012/08/08 19:46:10, Nirnimesh wrote: > Use sys.path.append. > > Move it inside a local function (see > chrome/test/functional/gtalk/pyauto_gtalk.py) Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:28: self.client_local = (self.remote == None) On 2012/08/08 19:46:10, Nirnimesh wrote: > Can all this be done in setUp instead? It is OK to put those in setUp. It is just better to put instance variable in __init__. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:29: self.host = self On 2012/08/08 19:46:10, Nirnimesh wrote: > Clearly declare the host and client vars in the docstring (since you expect > child classes to know about it) I am putting this in class docstring. Not sure if it is the right spot. Searched around but don't see good examples. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:38: def setUp(self): On 2012/08/08 19:46:10, Nirnimesh wrote: > no-op. Remove This calls PyUITest setUp. In this way, PyUITest doesn't need to be directly called in it2me_basic and we can also potentially add more setup steps. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:41: On 2012/08/08 19:46:10, Nirnimesh wrote: > remove spurious blank lines Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/it2me_basic.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/it2me_basic.py:9: import sys On 2012/08/08 19:46:10, Nirnimesh wrote: > system imports go on top. > > Fix in all files. Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/me2me_connect.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_connect.py:30: self.UninstallHostDaemon() On 2012/08/08 19:46:10, Nirnimesh wrote: > Call parent's tearDown Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_connect.py:63: sys.exit(chromoting_base.Main()) On 2012/08/08 19:46:10, Nirnimesh wrote: > It's probably not necessary to wrap inside sys.exit. Main() already does that a > few levels up. > > (Sorry for misleading before) Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/me2me_enable.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_enable.py:27: self.UninstallHostDaemon() On 2012/08/08 19:46:10, Nirnimesh wrote: > Call parent's tearDown Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/me2me_enable.py:30: """Enables remote connections, exercising different pin conditions On 2012/08/08 19:46:10, Nirnimesh wrote: > Make it a one liner. Details can go in next para Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/mock_pref_pane.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/08 19:46:10, Nirnimesh wrote: > Rename the file to contain 'mac' in it. Do I need to do this? Preference Pane is Mac specific concept and there is docstring to specific it is mac only already. I think there is no confusion anymore. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:5: """Mock pref pane for testing purpose.""" On 2012/08/08 19:46:10, Nirnimesh wrote: > Add: "on Mac" Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:16: class MockPrefPane(object): On 2012/08/08 19:46:10, Nirnimesh wrote: > Add 'Mac' to the name Same as the file name. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:99: On 2012/08/08 19:46:10, Nirnimesh wrote: > need 2 blank lines Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting.py:105: def LogIn(self, email=None, password=None, otp=None, On 2012/08/08 19:46:10, Nirnimesh wrote: > LogIn -> SignIn > > to match "SignOut" below Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting.py:328: time.sleep(2) On 2012/08/08 19:46:10, Nirnimesh wrote: > Remove sleep. This will make the test very flaky on the bots. Please figure out > alternatives. > > Repeat for all the sleep() calls Removed all but 1 in this file. See the comment on why the last cannot be removed. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting_helper.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:12: class ChromotingHelper(object): On 2012/08/08 19:46:10, Nirnimesh wrote: > Create a base class with abstract methods: Install, Uninstall, Enable, Disable, > ChangePin. > > Create 2 child classes: ChromotingHelperMac & ChromotingHelperWin where you > implement the methods. > > main() just needs to create an instance of the right class (based on the > platform) and call Install/Uninstall... on it. This way the platform-specific > logic will be contained within its class instead of being littered all over in > main() Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:13: """Chromting Helper class.""" On 2012/08/08 19:46:10, Nirnimesh wrote: > Chromoting Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:13: """Chromting Helper class.""" On 2012/08/08 19:46:10, Nirnimesh wrote: > Chromoting Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:13: """Chromting Helper class.""" On 2012/08/08 19:46:10, Nirnimesh wrote: > Chromoting Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:25: os.chdir(bin_dir) On 2012/08/08 19:46:10, Nirnimesh wrote: > Why is this necessary? Some of the steps (do_signing) below only works if the current dir is the bin_dir. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:30: subprocess.call('rm -f -R ' + host_dir, shell=True) On 2012/08/08 19:46:10, Nirnimesh wrote: > use shutil.rmtree Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:33: subprocess.call('unzip remoting-me2me-host-mac.zip', shell=True) On 2012/08/08 19:46:10, Nirnimesh wrote: > make the first arg a list. > Remove shell=True > > Repeat elsewhere Tried that and didn't work. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:41: 'functional', 'chromoting') On 2012/08/08 19:46:10, Nirnimesh wrote: > Do not reach into the functional dir. If you need the data files in pyautolib, > move them to pyautolib Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:135: """Main function to dispatch operations.""" On 2012/08/08 19:46:10, Nirnimesh wrote: > assert IsMac() or IsWin() Done. http://codereview.chromium.org/10821015/diff/14002/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:156: print 'Nothing to be done for enable' On 2012/08/08 19:46:10, Nirnimesh wrote: > enable -> sys.argv[1] Done.
A few more comments to go. http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:38: def setUp(self): On 2012/08/10 20:42:52, yihongg wrote: > On 2012/08/08 19:46:10, Nirnimesh wrote: > > no-op. Remove > > This calls PyUITest setUp. In this way, PyUITest doesn't need to be directly > called in it2me_basic and we can also potentially add more setup steps. Override it when you need to add more steps here. Until then, remove. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... File chrome/test/functional/chromoting/auth.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/auth.py:6: """Chromoting authentication related test cases.""" You don't need this docstring, since the class below covers it. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:20: import chromoting can this go before pyauto? http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:23: from pyauto_functional import Main move this near import pyauto_functional http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:29: Instance variable: Replace with: The following member vars can be used in child classes. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:31: host: The chromoting host side append: , and instance of XYZ http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:32: client: The chromoting client side ditto http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:49: def setUp(self): remove no-op http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:53: def tearDown(self): remove no-op. The parent's tearDown() will get called automatically http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... File chrome/test/functional/chromoting/mock_pref_pane.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:103: """Handles the mock pref pane actions.""" assert sys.platform == 'darwin' http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting.py:39: print '_ExecuteJavascript threw JSONInterfaceError' why is this necessary? http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting_helper.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:24: def UninstallHost(self, bin_dir): this method isn't abstract? http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:107: #subprocess.call('rm -f -R ' + host_dir, shell=True) remove commented out code http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:146: need another blank line
Nirnimesh, See the fixes/replies. Thanks for reviewing this again! Yihong http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/14002/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:38: def setUp(self): On 2012/08/14 22:09:30, Nirnimesh wrote: > On 2012/08/10 20:42:52, yihongg wrote: > > On 2012/08/08 19:46:10, Nirnimesh wrote: > > > no-op. Remove > > > > This calls PyUITest setUp. In this way, PyUITest doesn't need to be directly > > called in it2me_basic and we can also potentially add more setup steps. > > Override it when you need to add more steps here. Until then, remove. Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... File chrome/test/functional/chromoting/auth.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/auth.py:6: """Chromoting authentication related test cases.""" On 2012/08/14 22:09:30, Nirnimesh wrote: > You don't need this docstring, since the class below covers it. Is it OK to keep this? Pylint complains if there is no doc string here. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... File chrome/test/functional/chromoting/chromoting_base.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:23: from pyauto_functional import Main On 2012/08/14 22:09:30, Nirnimesh wrote: > move this near import pyauto_functional Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:29: Instance variable: On 2012/08/14 22:09:30, Nirnimesh wrote: > Replace with: The following member vars can be used in child classes. Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:31: host: The chromoting host side On 2012/08/14 22:09:30, Nirnimesh wrote: > append: , and instance of XYZ Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:32: client: The chromoting client side On 2012/08/14 22:09:30, Nirnimesh wrote: > ditto Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:49: def setUp(self): On 2012/08/14 22:09:30, Nirnimesh wrote: > remove no-op Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/chromoting_base.py:53: def tearDown(self): On 2012/08/14 22:09:30, Nirnimesh wrote: > remove no-op. > The parent's tearDown() will get called automatically Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... File chrome/test/functional/chromoting/mock_pref_pane.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/functional/chr... chrome/test/functional/chromoting/mock_pref_pane.py:103: """Handles the mock pref pane actions.""" On 2012/08/14 22:09:30, Nirnimesh wrote: > assert sys.platform == 'darwin' Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting.py:39: print '_ExecuteJavascript threw JSONInterfaceError' On 2012/08/14 22:09:30, Nirnimesh wrote: > why is this necessary? As why we have sleep() in the following. When ExecuteJavascript is called right after reload() or some click(), it will time out and throw JSONInterfaceError. If I don't catch, the test stops on exception. Currently to make the test more reliable, I catch this exception and retry. http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... File chrome/test/pyautolib/chromoting_helper.py (right): http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:24: def UninstallHost(self, bin_dir): On 2012/08/14 22:09:30, Nirnimesh wrote: > this method isn't abstract? Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:107: #subprocess.call('rm -f -R ' + host_dir, shell=True) On 2012/08/14 22:09:30, Nirnimesh wrote: > remove commented out code Done. http://codereview.chromium.org/10821015/diff/10005/chrome/test/pyautolib/chro... chrome/test/pyautolib/chromoting_helper.py:146: On 2012/08/14 22:09:30, Nirnimesh wrote: > need another blank line Done.
LGTM Please make sure that it does not break on the pyauto chromium bots after committing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yihongg@chromium.org/10821015/5011
Change committed as 151717
Fixed the checkpers error and AttributeError.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yihongg@chromium.org/10821015/2021
Change committed as 151760 |
