Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23726004:
Add test_server_setup.py and add support for additional flags. (Closed)
|
Created:
7 years, 3 months ago by nyquist Modified:
7 years, 3 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd test_server_setup.py and add support for additional flags.
Whenever a host driven test need to setup a server as part of a test,
the command line needs of the application to be instrumented needs to
change.
This CL adds a helper class for setting up a test server, and also adds
support for tests to setup additional flags (typically the URL for how
to connect to the test server).
Reverted in: http://crrev.com/220769
Relanding after re-upload to the same patch.
BUG=272584
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220750
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221122
Patch Set 1 #
Total comments: 21
Patch Set 2 : Addressed comments, gpylint #Patch Set 3 : Additional typo fix #
Total comments: 14
Patch Set 4 : Addressed new round of comments. #Patch Set 5 : Fixed permissions #
Messages
Total messages: 18 (0 generated)
frankf/craigdh: PTAL Depends on: https://codereview.chromium.org/23467004/ And is needed by (a future version of): https://codereview.chromium.org/22914014/
https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_case.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_case.py:114: additional_flags: A list of addition flags to add to the command line. nit: addition -> additional https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_server_setup.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:55: python_path = '$PYTHONPATH:' + ':'.join(abs_dirs) just modify os.environ https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:63: test_server = subprocess.Popen(server_flags, env={'PYTHONPATH': python_path}) We've had issues with bugs in Python 2.x's subprocess modules lead to major flakiness in the past. There are a variety of workarounds implemented in pylib/cmd_helper.py and I've made the replacement public in the FlagChanger patch so you can just replace this subprocess.Popen() with cmd_helper.Call(). https://codereview.chromium.org/23726004/diff/1/build/android/pylib/instrumen... File build/android/pylib/instrumentation/test_runner.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/instrumen... build/android/pylib/instrumentation/test_runner.py:84: if len(additional_flags): no need for len(), just "if additional_flags:"
https://codereview.chromium.org/23726004/diff/1/build/android/pylib/constants.py File build/android/pylib/constants.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/constants... build/android/pylib/constants.py:103: NET_TEST_SERVER_PATH = 'net/tools/testserver/testserver.py' Group this with lines 90-91 and use the same naming convention https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_server_setup.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:15: PYTHONPATH_DIRS = [ Add a leading underscore since these are private. Same below. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:31: run gpylint. 2 blank lines between top-level declarations. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:32: def SetUpTestServer(adb, shard_index, test_server_port, test_server_path): So it makes sense to make this into a class and encapsulate the test_server object below. You can then add a kill method to this class. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:38: test_server_port: port to run the test server on. Capital first letter. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/instrumen... File build/android/pylib/instrumentation/test_runner.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/instrumen... build/android/pylib/instrumentation/test_runner.py:55: ports_to_forward, additional_flags=[]): use None instead of empty list when piping this through and add a check for it on line 84.
addressed or responded to all comments. PTAL https://codereview.chromium.org/23726004/diff/1/build/android/pylib/constants.py File build/android/pylib/constants.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/constants... build/android/pylib/constants.py:103: NET_TEST_SERVER_PATH = 'net/tools/testserver/testserver.py' On 2013/08/29 00:07:19, frankf wrote: > Group this with lines 90-91 and use the same naming convention Done. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_case.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_case.py:114: additional_flags: A list of addition flags to add to the command line. On 2013/08/29 00:06:46, craigdh wrote: > nit: addition -> additional Done. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_server_setup.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:15: PYTHONPATH_DIRS = [ On 2013/08/29 00:07:19, frankf wrote: > Add a leading underscore since these are private. Same below. Done. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:31: On 2013/08/29 00:07:19, frankf wrote: > run gpylint. 2 blank lines between top-level declarations. Done. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:32: def SetUpTestServer(adb, shard_index, test_server_port, test_server_path): On 2013/08/29 00:07:19, frankf wrote: > So it makes sense to make this into a class and encapsulate the test_server > object below. You can then add a kill method to this class. Done. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:38: test_server_port: port to run the test server on. On 2013/08/29 00:07:19, frankf wrote: > Capital first letter. Done. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:55: python_path = '$PYTHONPATH:' + ':'.join(abs_dirs) On 2013/08/29 00:06:46, craigdh wrote: > just modify os.environ Uses os.environ now, but does not edit it here, since it should only be applied to the sub process. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:63: test_server = subprocess.Popen(server_flags, env={'PYTHONPATH': python_path}) On 2013/08/29 00:06:46, craigdh wrote: > We've had issues with bugs in Python 2.x's subprocess modules lead to major > flakiness in the past. There are a variety of workarounds implemented in > pylib/cmd_helper.py and I've made the replacement public in the FlagChanger > patch so you can just replace this subprocess.Popen() with cmd_helper.Call(). There is no waiting for output here, so that does not work. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/instrumen... File build/android/pylib/instrumentation/test_runner.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/instrumen... build/android/pylib/instrumentation/test_runner.py:55: ports_to_forward, additional_flags=[]): On 2013/08/29 00:07:19, frankf wrote: > use None instead of empty list when piping this through and add a check for it > on line 84. Done. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/instrumen... build/android/pylib/instrumentation/test_runner.py:84: if len(additional_flags): On 2013/08/29 00:06:46, craigdh wrote: > no need for len(), just "if additional_flags:" Done.
lgtm w/ nit. Please wait for frankf. https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... File build/android/pylib/host_driven/test_server_setup.py (right): https://codereview.chromium.org/23726004/diff/1/build/android/pylib/host_driv... build/android/pylib/host_driven/test_server_setup.py:63: test_server = subprocess.Popen(server_flags, env={'PYTHONPATH': python_path}) On 2013/08/29 23:59:43, nyquist wrote: > On 2013/08/29 00:06:46, craigdh wrote: > > We've had issues with bugs in Python 2.x's subprocess modules lead to major > > flakiness in the past. There are a variety of workarounds implemented in > > pylib/cmd_helper.py and I've made the replacement public in the FlagChanger > > patch so you can just replace this subprocess.Popen() with cmd_helper.Call(). > > There is no waiting for output here, so that does not work. Ah, you're right, I thought it was returning a Popen object. You're fine with subprocess for now, if we see issues we can add a Popen replacement to cmd_helper. https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... File build/android/pylib/host_driven/test_server.py (right): https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:74: # Note the colon after $PYTHONPATH. This appends the list of nit: update comment, or perhaps just remove it?
lgtm after fixing these https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/const... File build/android/pylib/constants.py (right): https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/const... build/android/pylib/constants.py:96: TEST_SYNC_SERVER_PATH = 'sync/tools/testserver/sync_testserver.py' So these constants only needed to be used by modules importing TestServer right? So it makes more sense to move them there. You can just leave a copy for the ones currently referenced downstream and do clean up later. https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... File build/android/pylib/host_driven/test_server.py (right): https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:1: #!/usr/bin/python Remove the shebang, this module is only imported. https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:47: """Sets up a Python driven test server on the host machine. s/python driven/host driven/g https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:62: Returns: Update this https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:85: server_flags = ['python', os.path.join(src_dir, test_server_path), server_flags -> cmd https://codereview.chromium.org/23726004/diff/16001/chrome/android/host_drive... File chrome/android/host_driven_tests/DummyTest.py (right): https://codereview.chromium.org/23726004/diff/16001/chrome/android/host_drive... chrome/android/host_driven_tests/DummyTest.py:5: """Python-driven java_tests which exercise dummy functionality. same here, "host-driven"
All done. https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/const... File build/android/pylib/constants.py (right): https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/const... build/android/pylib/constants.py:96: TEST_SYNC_SERVER_PATH = 'sync/tools/testserver/sync_testserver.py' On 2013/08/30 18:22:43, frankf wrote: > So these constants only needed to be used by modules importing TestServer right? > So it makes more sense to move them there. You can just leave a copy for the > ones currently referenced downstream and do clean up later. Done. Also reverted my other change to this patch. Keeping port numbers here to have them all in one place so there is no port collisions. https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... File build/android/pylib/host_driven/test_server.py (right): https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:1: #!/usr/bin/python On 2013/08/30 18:22:43, frankf wrote: > Remove the shebang, this module is only imported. Done. https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:47: """Sets up a Python driven test server on the host machine. On 2013/08/30 18:22:43, frankf wrote: > s/python driven/host driven/g Done. https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:62: Returns: On 2013/08/30 18:22:43, frankf wrote: > Update this Done. Removed "Returns" since this is now a constructor. https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:74: # Note the colon after $PYTHONPATH. This appends the list of On 2013/08/30 17:55:50, craigdh wrote: > nit: update comment, or perhaps just remove it? Done. Removed. https://codereview.chromium.org/23726004/diff/16001/build/android/pylib/host_... build/android/pylib/host_driven/test_server.py:85: server_flags = ['python', os.path.join(src_dir, test_server_path), On 2013/08/30 18:22:43, frankf wrote: > server_flags -> cmd Done. https://codereview.chromium.org/23726004/diff/16001/chrome/android/host_drive... File chrome/android/host_driven_tests/DummyTest.py (right): https://codereview.chromium.org/23726004/diff/16001/chrome/android/host_drive... chrome/android/host_driven_tests/DummyTest.py:5: """Python-driven java_tests which exercise dummy functionality. On 2013/08/30 18:22:43, frankf wrote: > same here, "host-driven" Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nyquist@chromium.org/23726004/25001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nyquist@chromium.org/23726004/25001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
This CL only changes android-related files. Adding NOTRY=true since only linux_rel keeps failing on an unrelated test.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nyquist@chromium.org/23726004/25001
Message was sent while issue was closed.
Change committed as 220750
Message was sent while issue was closed.
On 2013/09/01 22:54:48, I haz the power (commit-bot) wrote: > Change committed as 220750 Looks like this is causing the linux cros bots to fail: http://chromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20Chromi...
Message was sent while issue was closed.
On 2013/09/02 00:20:57, achuith.bhandarkar wrote: > On 2013/09/01 22:54:48, I haz the power (commit-bot) wrote: > > Change committed as 220750 > > Looks like this is causing the linux cros bots to fail: > http://chromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%2520Chro... Reverted: crrev.com/220769
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nyquist@chromium.org/23726004/52001
Message was sent while issue was closed.
Change committed as 221122 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
