Clean up Android gtest runners.

Clean up Android gtest runners.

- Remove obsolete rebaseline option
- Fix a bug with reporting skipped tests
- Fix some style issues

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

[frankf, 2012-12-12 22:46:06]

https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py#ne...
build/android/run_tests.py:423: option_parser.add_option('-p',
dest='performance_test',
@bulach: What's the history behind this option. It's not used on any bots. Do we
want to keep it?

[Yaron, 2012-12-13 00:04:34]

lgtm

https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py#ne...
build/android/run_tests.py:232: not not self.log_dump_name,
cute

[bulach, 2012-12-13 14:55:21]

thanks, overall lgtm,  but I have one suggestion below:

https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py#ne...
build/android/run_tests.py:213: then filters it again using the diabled list on
the host.
I dislike this combination :)
the while + try is tricky enough, and there's no need to make it even more
complex by combining with the list from the host here...
keep this function as only getting things from the device itself, then create
another wrapper to filter the list..

also, in general, such changes could be made in separate patches, that is, one
for removing the baseline option, another one for improving these other bits..

https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py#ne...
build/android/run_tests.py:423: option_parser.add_option('-p',
dest='performance_test',
On 2012/12/12 22:46:06, frankf wrote:
> @bulach: What's the history behind this option. It's not used on any bots. Do
we
> want to keep it?

I tried some archeology in the other trees, but no much luck... :( it could've
been back from when we ran page_cyclers as c++ unit tests...
I can't find any reference now, so feel free to dump it.

[craigdh, 2012-12-13 18:08:46]

On 2012/12/13 14:55:21, bulach wrote:
> thanks, overall lgtm,  but I have one suggestion below:
> 
> https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py
> File build/android/run_tests.py (right):
> 
>
https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py#ne...
> build/android/run_tests.py:213: then filters it again using the diabled list
on
> the host.
> I dislike this combination :)
> the while + try is tricky enough, and there's no need to make it even more
> complex by combining with the list from the host here...
> keep this function as only getting things from the device itself, then create
> another wrapper to filter the list..
> 
> also, in general, such changes could be made in separate patches, that is, one
> for removing the baseline option, another one for improving these other bits..
> 
>
https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py#ne...
> build/android/run_tests.py:423: option_parser.add_option('-p',
> dest='performance_test',
> On 2012/12/12 22:46:06, frankf wrote:
> > @bulach: What's the history behind this option. It's not used on any bots.
Do
> we
> > want to keep it?
> 
> I tried some archeology in the other trees, but no much luck... :( it could've
> been back from when we ran page_cyclers as c++ unit tests...
> I can't find any reference now, so feel free to dump it.

lgtm.

[frankf, 2012-12-13 19:54:04]

https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py
File build/android/run_tests.py (right):

https://codereview.chromium.org/11557016/diff/1/build/android/run_tests.py#ne...
build/android/run_tests.py:213: then filters it again using the diabled list on
the host.
Done.

On 2012/12/13 14:55:21, bulach wrote:
> I dislike this combination :)
> the while + try is tricky enough, and there's no need to make it even more
> complex by combining with the list from the host here...
> keep this function as only getting things from the device itself, then create
> another wrapper to filter the list..
> 
> also, in general, such changes could be made in separate patches, that is, one
> for removing the baseline option, another one for improving these other bits..

[bulach, 2012-12-14 10:36:10]

hey frank, one important note below... I still think it'd probably be best to
split into different patches, one for just removing the rebaseline option, the
other one for cleanup on getting the list of tests... wdyt?

https://codereview.chromium.org/11557016/diff/11002/build/android/run_tests.py
File build/android/run_tests.py (left):

https://codereview.chromium.org/11557016/diff/11002/build/android/run_tests.p...
build/android/run_tests.py:225: return all_tests, available_devices
this was the really important bit... :)

https://codereview.chromium.org/11557016/diff/11002/build/android/run_tests.py
File build/android/run_tests.py (right):

https://codereview.chromium.org/11557016/diff/11002/build/android/run_tests.p...
build/android/run_tests.py:207: self.tests = self.all_tests
what is the difference between tests and all_tests?
at the sharder level, it seems better to have "all_tests" rather than "tests",
I'm just not sure it needs both..

also, and this is really important: when obtaining the tests, we'd drop out sick
devices... ilevy says it greatly reduced the number of flukes in the bots.
this patch changes that behavior... :(

[frankf, 2012-12-14 19:12:40]

Thanks Marcus. I'll make sure that future CLs are smaller in scope.

https://codereview.chromium.org/11557016/diff/11002/build/android/run_tests.py
File build/android/run_tests.py (left):

https://codereview.chromium.org/11557016/diff/11002/build/android/run_tests.p...
build/android/run_tests.py:225: return all_tests, available_devices
As discussed offline, we need a better approach to device flakiness. We also
don't do it systematically by testing all devices, rather than breaking on the
first successull run.

Since this CL was doing too much, here's a partial revert here:
https://codereview.chromium.org/11573038/

On 2012/12/14 10:36:11, bulach wrote:
> this was the really important bit... :)

https://codereview.chromium.org/11557016/diff/11002/build/android/run_tests.py
File build/android/run_tests.py (right):

https://codereview.chromium.org/11557016/diff/11002/build/android/run_tests.p...
build/android/run_tests.py:207: self.tests = self.all_tests
self.tests get reassigned on retries (in base_test_sharder.py). We want the
original list to pass to LogFull. Take a look at the bogus TESTS_TO_RUN here:

http://master.chrome.corp.google.com:8000/builders/android/builds/594/steps/i...

On 2012/12/14 10:36:11, bulach wrote:
> what is the difference between tests and all_tests?
> at the sharder level, it seems better to have "all_tests" rather than "tests",
> I'm just not sure it needs both..
> 
> also, and this is really important: when obtaining the tests, we'd drop out
sick
> devices... ilevy says it greatly reduced the number of flukes in the bots.
> this patch changes that behavior... :(