Use a single Python Forwarder instance for each base test runner instance.

Use a single Python Forwarder instance for each base test runner instance.

This is achieved by having BaseTestRunner instantiate a single Forwarder
instance and inject it into SpawningServer.

This makes the test server spawner instance use an already configured Forwarder
instance for every single unit test rather than instantiating Forwarder every
time (which also meant unnecessarily running 'adb forward' for every single
unit test).

Note that this CL also makes Forwarder thread-safe. This is due to the fact
that the test server spawner does its work on a worker thread and not to the
fact that a single Forwarder instance can be shared accross multiple shards as
one could think.

BUG=229685, 239014
R=bulach@chromium.org, craigdh@chromium.org, frankf@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200500

[Philippe, 2013-05-14 17:58:06]

FYI, this depends on https://codereview.chromium.org/15008004.

[bulach, 2013-05-14 18:21:53]

lgtm but please wait for frankf and craig, I haven't been changing this in a
while...

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
build/android/pylib/forwarder.py:184: pass
we should chat about this :)
there's Telemetry
(tools/telemetry/telemetry/core/chrome/android_browser_backend.py) and also the
higher level perf sharded (build/android/bb_run_sharded_steps.py)... I'm not
entirely sure they're doing the right thing anymore.

[Philippe, 2013-05-15 08:32:23]

Thanks Marcus! I'm waiting for Frank and Craig.

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
build/android/pylib/forwarder.py:184: pass
On 2013/05/14 18:21:53, bulach wrote:
> we should chat about this :)
> there's Telemetry
> (tools/telemetry/telemetry/core/chrome/android_browser_backend.py) and also
the
> higher level perf sharded (build/android/bb_run_sharded_steps.py)... I'm not
> entirely sure they're doing the right thing anymore.

Yeah, I will update them in a separate CL if you agree. Note that Close() has
been a no-op for a while. |self._device_process| was never set.

[Philippe, 2013-05-15 15:37:37]

On 2013/05/15 08:32:23, Philippe wrote:
> Thanks Marcus! I'm waiting for Frank and Craig.
> 
>
https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
> File build/android/pylib/forwarder.py (right):
> 
>
https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
> build/android/pylib/forwarder.py:184: pass
> On 2013/05/14 18:21:53, bulach wrote:
> > we should chat about this :)
> > there's Telemetry
> > (tools/telemetry/telemetry/core/chrome/android_browser_backend.py) and also
> the
> > higher level perf sharded (build/android/bb_run_sharded_steps.py)... I'm not
> > entirely sure they're doing the right thing anymore.
> 
> Yeah, I will update them in a separate CL if you agree. Note that Close() has
> been a no-op for a while. |self._device_process| was never set.

Frank/Craig: Ping :)

[frankf, 2013-05-15 17:24:30]

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ba...
File build/android/pylib/base/base_test_runner.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ba...
build/android/pylib/base/base_test_runner.py:187: for i in xrange(0, 3):
Could you explain this magic and the sleep below? What can cause a failure to
launch test spawner server?

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ch...
File build/android/pylib/chrome_test_server_spawner.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ch...
build/android/pylib/chrome_test_server_spawner.py:109: build_type: 'Release' or
'Debug'.
Update the docstring

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
build/android/pylib/forwarder.py:132: def UnmapDevicePort(self, device_port):
Please add docstring for all methods here.

[Philippe, 2013-05-15 17:57:41]

Thanks Frank!

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ba...
File build/android/pylib/base/base_test_runner.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ba...
build/android/pylib/base/base_test_runner.py:187: for i in xrange(0, 3):
On 2013/05/15 17:24:30, frankf wrote:
> Could you explain this magic and the sleep below? What can cause a failure to
> launch test spawner server?

No idea, I'm not the author here. I would be tempted to remove the for loop and
the sleep. This looks flaky.

Also IsHttpServerConnectable() looks less than ideal. Ideally
spawning_server.Start() should block until the server is ready rather than
requiring the client to implement a flaky logic (I'm sure there is a reason
behind this though).

But I would prefer to address these things in a separate CL. The current CL is
already impactful enough.

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ch...
File build/android/pylib/chrome_test_server_spawner.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ch...
build/android/pylib/chrome_test_server_spawner.py:109: build_type: 'Release' or
'Debug'.
On 2013/05/15 17:24:30, frankf wrote:
> Update the docstring

Done.

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/fo...
build/android/pylib/forwarder.py:132: def UnmapDevicePort(self, device_port):
On 2013/05/15 17:24:30, frankf wrote:
> Please add docstring for all methods here.

Done.

[frankf, 2013-05-15 18:00:35]

lgtm

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ba...
File build/android/pylib/base/base_test_runner.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/1/build/android/pylib/ba...
build/android/pylib/base/base_test_runner.py:187: for i in xrange(0, 3):
sg. Please add a TODO.

On 2013/05/15 17:57:41, Philippe wrote:
> On 2013/05/15 17:24:30, frankf wrote:
> > Could you explain this magic and the sleep below? What can cause a failure
to
> > launch test spawner server?
> 
> No idea, I'm not the author here. I would be tempted to remove the for loop
and
> the sleep. This looks flaky.
> 
> Also IsHttpServerConnectable() looks less than ideal. Ideally
> spawning_server.Start() should block until the server is ready rather than
> requiring the client to implement a flaky logic (I'm sure there is a reason
> behind this though).
> 
> But I would prefer to address these things in a separate CL. The current CL is
> already impactful enough.

[craigdh, 2013-05-15 18:34:00]

https://chromiumcodereview.appspot.com/15151003/diff/17001/build/android/pyli...
File build/android/pylib/forwarder.py (right):

https://chromiumcodereview.appspot.com/15151003/diff/17001/build/android/pyli...
build/android/pylib/forwarder.py:175: tool: Tool class to use to get wrapper, if
necessary, for executing the
nit: This comment cuts off and isn't very clear.

[craigdh, 2013-05-15 18:34:29]

On 2013/05/15 18:34:00, craigdh wrote:
>
https://chromiumcodereview.appspot.com/15151003/diff/17001/build/android/pyli...
> File build/android/pylib/forwarder.py (right):
> 
>
https://chromiumcodereview.appspot.com/15151003/diff/17001/build/android/pyli...
> build/android/pylib/forwarder.py:175: tool: Tool class to use to get wrapper,
if
> necessary, for executing the
> nit: This comment cuts off and isn't very clear.

but otherwise lgtm :)

[Philippe, 2013-05-16 09:24:22]

On 2013/05/15 18:34:29, craigdh wrote:
> On 2013/05/15 18:34:00, craigdh wrote:
> >
>
https://chromiumcodereview.appspot.com/15151003/diff/17001/build/android/pyli...
> > File build/android/pylib/forwarder.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/15151003/diff/17001/build/android/pyli...
> > build/android/pylib/forwarder.py:175: tool: Tool class to use to get
wrapper,
> if
> > necessary, for executing the
> > nit: This comment cuts off and isn't very clear.
> 
> but otherwise lgtm :)

Thanks guys!

[Philippe, 2013-05-16 09:27:48]

Committed patchset #3 manually as r200500 (presubmit successful).