[Andoid] Threaded TestRunner creation and SetUp and TearDown calls.

[Andoid] Threaded TestRunner creation and SetUp and TearDown calls.

Create TestRunners only once and only call SetUp and TearDown once.

BUG=176325, 168889
TEST=pylib/utils/raiser_thread_unittest.py, pylib/base/shard_unittest.py, run_tests.py

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

[craigdh, 2013-02-22 00:57:42]

ptal.

https://codereview.chromium.org/12317059/diff/1/build/android/pylib/utils/rer...
File build/android/pylib/utils/reraiser_thread_unittest.py (right):

https://codereview.chromium.org/12317059/diff/1/build/android/pylib/utils/rer...
build/android/pylib/utils/reraiser_thread_unittest.py:5: """Unittests for
shard.py."""
should be reraiser_thread.py

[nilesh, 2013-02-22 01:08:07]

I think we need both Setup and SetupOnce.
Setup (called before each call to RunTest) will need ClearApplicationState (or
kill the test application explicitly if a test times out), since we are not
uninstalling the test application here.

[craigdh, 2013-02-22 01:24:56]

On 2013/02/22 01:08:07, nileshagrawal1 wrote:
> I think we need both Setup and SetupOnce.
> Setup (called before each call to RunTest) will need ClearApplicationState (or
> kill the test application explicitly if a test times out), since we are not
> uninstalling the test application here.

I agree that some test types might need per-test setup or cleanup, but this can
be done in RunTest(test).

In this particular case, unittests only need to clear state once as each "test"
is an arbitrary number of actual unittests anyway.

[nilesh, 2013-02-22 01:33:58]

On 2013/02/22 01:24:56, craigdh wrote:
> On 2013/02/22 01:08:07, nileshagrawal1 wrote:
> > I think we need both Setup and SetupOnce.
> > Setup (called before each call to RunTest) will need ClearApplicationState
(or
> > kill the test application explicitly if a test times out), since we are not
> > uninstalling the test application here.
> 
> I agree that some test types might need per-test setup or cleanup, but this
can
> be done in RunTest(test).

Sounds good.
> 
> In this particular case, unittests only need to clear state once as each
"test"
> is an arbitrary number of actual unittests anyway.

[nilesh, 2013-02-22 02:00:29]

https://codereview.chromium.org/12317059/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12317059/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:131: runners = _CreateRunners(runner_factory,
set(devices))
I think the numbers of runners we create should always be less than the number
of tests here.

[frankf, 2013-02-22 02:42:55]

https://codereview.chromium.org/12317059/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12317059/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:97: threads.JoinAll()
For all these JoinAll invokations, we need to think about what type of
exceptions can be reraised and whether we want to recover from them (i.e. device
failures).

[craigdh, 2013-02-22 17:24:20]

https://codereview.chromium.org/12317059/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12317059/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:97: threads.JoinAll()
On 2013/02/22 02:42:55, frankf wrote:
> For all these JoinAll invokations, we need to think about what type of
> exceptions can be reraised and whether we want to recover from them (i.e.
device
> failures).

I thought about it a bit. Here's what's currently handled and some thoughts:

- Here the device failures are handled on the child thread (see _SetUp). A
failure just means we don't create a TestRunner for that device.
- Should device failures be ignored in _TearDownRunners?
- Device failures in _RunAllTests are handled by the retry logic, but that could
throw out valid results that happened before the exception. In the future we may
want to have retries be handled right on the threads by putting them back into
the queue and keeping track of how many times each test has been tried. That's a
bit out of scope for this change, however.

https://codereview.chromium.org/12317059/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:131: runners = _CreateRunners(runner_factory,
set(devices))
On 2013/02/22 02:00:29, nileshagrawal1 wrote:
> I think the numbers of runners we create should always be less than the number
> of tests here.

There's an issue, though, if devices fail. I think it's good to have any extras
available. The setup happens in parallel so there shouldn't be any additional
cost.

The situation I imagine goes like this: You have three devices attached but only
run one test. If the device you choose becomes unresponsive halfway though the
test you don't want to fail, you want to switch over to another device. Seems
like we might as well set them up in parallel ahead of time, as then there is no
time cost or extra logic in bringing up another device.

[craigdh, 2013-02-22 23:23:13]

> - Device failures in _RunAllTests are handled by the retry logic, but that
could
> throw out valid results that happened before the exception. In the future we
may
> want to have retries be handled right on the threads by putting them back into
> the queue and keeping track of how many times each test has been tried. That's
a
> bit out of scope for this change, however.

Frank convinced me to switch the retry logic in this cl. It works for me and the
unittests pass... but pta(very careful)l at the threading logic in shard.py.

[nilesh, 2013-02-22 23:50:40]

https://codereview.chromium.org/12317059/diff/2007/build/android/pylib/base/n...
File build/android/pylib/base/new_base_test_runner.py (right):

https://codereview.chromium.org/12317059/diff/2007/build/android/pylib/base/n...
build/android/pylib/base/new_base_test_runner.py:80: """Run once before all
tests are run."""
As discussed please move ClearApplicationState (in gtest runner) to RunTest, so
that we clear the application state and kill the test activity before running a
test. We will also need to call it with the right package name.

[craigdh, 2013-02-23 01:13:18]

On 2013/02/22 23:50:40, nileshagrawal1 wrote:
>
https://codereview.chromium.org/12317059/diff/2007/build/android/pylib/base/n...
> File build/android/pylib/base/new_base_test_runner.py (right):
> 
>
https://codereview.chromium.org/12317059/diff/2007/build/android/pylib/base/n...
> build/android/pylib/base/new_base_test_runner.py:80: """Run once before all
> tests are run."""
> As discussed please move ClearApplicationState (in gtest runner) to RunTest,
so
> that we clear the application state and kill the test activity before running
a
> test. We will also need to call it with the right package name.

@nilesh: does this look right to you for the exe?

[craigdh, 2013-02-23 01:16:48]

On 2013/02/23 01:13:18, craigdh wrote:
> On 2013/02/22 23:50:40, nileshagrawal1 wrote:
> >
>
https://codereview.chromium.org/12317059/diff/2007/build/android/pylib/base/n...
> > File build/android/pylib/base/new_base_test_runner.py (right):
> > 
> >
>
https://codereview.chromium.org/12317059/diff/2007/build/android/pylib/base/n...
> > build/android/pylib/base/new_base_test_runner.py:80: """Run once before all
> > tests are run."""
> > As discussed please move ClearApplicationState (in gtest runner) to RunTest,
> so
> > that we clear the application state and kill the test activity before
running
> a
> > test. We will also need to call it with the right package name.
> 
> @nilesh: does this look right to you for the exe?

Or do we still need to call ClearApplicationData(apk) even when we don't have an
apk? It was getting called previously with the default chrome apk from constants
even in the exe case.

[nilesh, 2013-02-23 01:24:10]

https://codereview.chromium.org/12317059/diff/8003/build/android/pylib/gtest/...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/12317059/diff/8003/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:247:
self.adb.ClearApplicationState(self._test_apk_package_name)
I think the killAllblocking can be in else: 

Or even better if we let test_package_apk and test_package_executable implement
their own ClearApplicationState

https://codereview.chromium.org/12317059/diff/8003/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:286: self._ClearApplicationState()
I dont think we need it here now. The next line (StripAndCopyFiles) will
uninstall and install the package anyway.

[craigdh, 2013-02-25 20:05:13]

https://codereview.chromium.org/12317059/diff/8003/build/android/pylib/gtest/...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/12317059/diff/8003/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:247:
self.adb.ClearApplicationState(self._test_apk_package_name)
On 2013/02/23 01:24:10, nileshagrawal1 wrote:
> I think the killAllblocking can be in else: 
> 
> Or even better if we let test_package_apk and test_package_executable
implement
> their own ClearApplicationState

Done.

https://codereview.chromium.org/12317059/diff/8003/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:286: self._ClearApplicationState()
On 2013/02/23 01:24:10, nileshagrawal1 wrote:
> I dont think we need it here now. The next line (StripAndCopyFiles) will
> uninstall and install the package anyway.

Done.

[nilesh, 2013-02-25 21:27:49]

lgtm with nit.

https://codereview.chromium.org/12317059/diff/12007/build/android/pylib/gtest...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/12317059/diff/12007/build/android/pylib/gtest...
build/android/pylib/gtest/test_runner.py:166: self._test_apk_package_name =
test_apk_package_name
Nit: This is not used anymore.

[frankf, 2013-02-25 22:22:57]

As discussed offline, it's much simpler to use two lists for scheduled tests,
and in-progress tests instead of using lower-level primitives luck lock and
event.

Also, please add comments for all corner cases.

[frankf, 2013-02-26 00:57:07]

https://codereview.chromium.org/12317059/diff/4002/build/android/pylib/base/s...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12317059/diff/4002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:39: self._incomplete_count = len(self._items)
incomplete_count -> tests_in_progress

https://codereview.chromium.org/12317059/diff/4002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:40: self._can_pop = threading.Event()
This event is signalling two things 1) when an item is added to the queue 2)
when there's no tests_in_progress which hurts readability.

https://codereview.chromium.org/12317059/diff/4002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:41: self._can_pop.set()
what if items is empty?

[craigdh, 2013-02-26 01:23:16]

ptal.

https://codereview.chromium.org/12317059/diff/4002/build/android/pylib/base/s...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12317059/diff/4002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:39: self._incomplete_count = len(self._items)
On 2013/02/26 00:57:07, frankf wrote:
> incomplete_count -> tests_in_progress

Done.

https://codereview.chromium.org/12317059/diff/4002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:40: self._can_pop = threading.Event()
On 2013/02/26 00:57:07, frankf wrote:
> This event is signalling two things 1) when an item is added to the queue 2)
> when there's no tests_in_progress which hurts readability. 

Improved naming and added comments.

https://codereview.chromium.org/12317059/diff/4002/build/android/pylib/base/s...
build/android/pylib/base/shard.py:41: self._can_pop.set()
On 2013/02/26 00:57:07, frankf wrote:
> what if items is empty?

It got cleared if someone went to pop and found the queue empty. However, this
wasn't very clear so I changed the initialization to use self.add().

[frankf, 2013-02-26 01:32:31]

lgtm

https://codereview.chromium.org/12317059/diff/11013/build/android/pylib/gtest...
File build/android/pylib/gtest/test_package_apk.py (right):

https://codereview.chromium.org/12317059/diff/11013/build/android/pylib/gtest...
build/android/pylib/gtest/test_package_apk.py:76: def
ClearApplicationState(self):
nit: mark all overrides in both packages.

[craigdh, 2013-02-28 18:19:39]

Committed patchset #14 manually as r185276 (presubmit successful).