Move android buildbot test logic into python

Move android buildbot test logic into python

Our bash scripts were getting complicated an unwieldy. In this
commit I port the test logic into python, as a precursor to
moving the rest of the file.

I also move the step selection logic into python which offers the following advantages:
- Less files to maintain, easier to change something globally.
- Paves way to moving most factory_properties into 'slave_properties' argument I added.  This will let us change parameters like buildtype (make vs. ninja) without a waterfall restart.  This also moves configuration into a single place.

Also:
- Refactor test runners to not require envsetup.
- Adding test report step

BUG=153707, 163476, 168518
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175698

[Isaac (away), 2012-12-24 21:49:19]

Moved to new issue number as I've managed to break rietveld on the old one.

[Isaac (away), 2012-12-27 19:25:42]

+Marcus

[frankf, 2012-12-27 19:57:46]

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
File build/android/buildbot/bb_run_tests.py (right):

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:37: 'ContentShellTest',
There's a lot of redundancy here. Why can't you derive everything from the apk
itself based on the convention we use.

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:92: RunCmd(['build/android/run_tests.py']
+ args)
If we're adding the complexity of another wrapper, we should at least get the
benefit of simplified/consistant test scripts by removing all the buildbot
annotations out of them. So, run_tests.py should not be maintaining a list of
default suites but instead be called  multiple times using -s parameter.

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:110:
buildbot_report.PrintNamedStep('Instrumentation tests for %s' % test.name)
Will crbug.com/161993 be addressed in a follow up CL? We want a clear convention
for step names since they're used in bunch of places including the flakiness
dashboard server.

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:120:
RunCmd(['build/android/run_instrumentation_tests.py'] + args)
Again, run_instrumentation_tests.py won't produce a second step right?

[Isaac (away), 2012-12-27 22:23:03]

frankf to cc

Some responses inline.

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
File build/android/buildbot/bb_run_tests.py (right):

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:37: 'ContentShellTest',
On 2012/12/27 19:57:47, frankf wrote:
> There's a lot of redundancy here. Why can't you derive everything from the apk
> itself based on the convention we use.

I am porting bash code to python.  If we want to simplify logic we can do that
in a follow up CL.

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:92: RunCmd(['build/android/run_tests.py']
+ args)
On 2012/12/27 19:57:47, frankf wrote:
> If we're adding the complexity of another wrapper, we should at least get the
> benefit of simplified/consistant test scripts by removing all the buildbot
> annotations out of them. So, run_tests.py should not be maintaining a list of
> default suites but instead be called  multiple times using -s parameter.

This is a pure port - bash -> python.  We are not adding complexity.  Moving the
annotations here (or to bb_run_step) is a good idea but that can be done in a
follow up CL.

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:110:
buildbot_report.PrintNamedStep('Instrumentation tests for %s' % test.name)
On 2012/12/27 19:57:47, frankf wrote:
> Will crbug.com/161993 be addressed in a follow up CL? We want a clear
convention
> for step names since they're used in bunch of places including the flakiness
> dashboard server.

That is a separate bug, not directly related to this.  In both bash and in this
new python code, we create an instrumentation test step and run the
instrumentation tests.  We need to remove the step the
run_intrumentation_tests.py code creates, but I won't be doing that here.

The step name is a placeholder until I came up with a better name.  I'm thinking

'%s tests' % test.name, i.e. "ContentShell tests"

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:120:
RunCmd(['build/android/run_instrumentation_tests.py'] + args)
On 2012/12/27 19:57:47, frankf wrote:
> Again, run_instrumentation_tests.py won't produce a second step right?

See above.  Pure port / wrapper changes.  It is complicated to modify
run_instrumentation_tests as that is called by our internal bots in a different
context.

[frankf, 2012-12-27 22:33:26]

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
File build/android/buildbot/bb_run_tests.py (right):

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:92: RunCmd(['build/android/run_tests.py']
+ args)
OK, let's keep this CL simple, but it seems we're on the same page wrt to how
annotations should be done.

On 2012/12/27 22:23:03, Isaac wrote:
> On 2012/12/27 19:57:47, frankf wrote:
> > If we're adding the complexity of another wrapper, we should at least get
the
> > benefit of simplified/consistant test scripts by removing all the buildbot
> > annotations out of them. So, run_tests.py should not be maintaining a list
of
> > default suites but instead be called  multiple times using -s parameter.
> 
> This is a pure port - bash -> python.  We are not adding complexity.  Moving
the
> annotations here (or to bb_run_step) is a good idea but that can be done in a
> follow up CL.

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:110:
buildbot_report.PrintNamedStep('Instrumentation tests for %s' % test.name)
Let's avoid spaces. I've proposed a convention in that bug which is more inline
with the naming convention for other test types used in Chromium.

On 2012/12/27 22:23:03, Isaac wrote:
> On 2012/12/27 19:57:47, frankf wrote:
> > Will crbug.com/161993 be addressed in a follow up CL? We want a clear
> convention
> > for step names since they're used in bunch of places including the flakiness
> > dashboard server.
> 
> That is a separate bug, not directly related to this.  In both bash and in
this
> new python code, we create an instrumentation test step and run the
> instrumentation tests.  We need to remove the step the
> run_intrumentation_tests.py code creates, but I won't be doing that here.
> 
> The step name is a placeholder until I came up with a better name.  I'm
thinking
> 
> '%s tests' % test.name, i.e. "ContentShell tests"

[Isaac (away), 2012-12-27 22:55:20]

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
File build/android/buildbot/bb_run_tests.py (right):

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:110:
buildbot_report.PrintNamedStep('Instrumentation tests for %s' % test.name)
On 2012/12/27 22:33:26, frankf wrote:
> Let's avoid spaces. I've proposed a convention in that bug which is more
inline
> with the naming convention for other test types used in Chromium.

OK, I took a look.  I think the word instrumentation is redundant, given that is
the only type of test running on ContentShell, test shell, etc.  How about
'%s_tests' % test.name?

[frankf, 2012-12-27 23:06:10]

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
File build/android/buildbot/bb_run_tests.py (right):

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:110:
buildbot_report.PrintNamedStep('Instrumentation tests for %s' % test.name)
It's needed to remove ambiguity in other contexts. I took a quick look at the
flakiness dashboard, there's something called test_shell_tests in webkit for
example. 

On 2012/12/27 22:55:20, Isaac wrote:
> On 2012/12/27 22:33:26, frankf wrote:
> > Let's avoid spaces. I've proposed a convention in that bug which is more
> inline
> > with the naming convention for other test types used in Chromium.
> 
> OK, I took a look.  I think the word instrumentation is redundant, given that
is
> the only type of test running on ContentShell, test shell, etc.  How about
> '%s_tests' % test.name?

[Isaac (away), 2012-12-28 05:31:48]

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
File build/android/buildbot/bb_run_tests.py (right):

https://chromiumcodereview.appspot.com/11666023/diff/2003/build/android/build...
build/android/buildbot/bb_run_tests.py:110:
buildbot_report.PrintNamedStep('Instrumentation tests for %s' % test.name)
On 2012/12/27 23:06:10, frankf wrote:
> It's needed to remove ambiguity in other contexts. I took a quick look at the
> flakiness dashboard, there's something called test_shell_tests in webkit for
> example. 
> 
> On 2012/12/27 22:55:20, Isaac wrote:
> > On 2012/12/27 22:33:26, frankf wrote:
> > > Let's avoid spaces. I've proposed a convention in that bug which is more
> > inline
> > > with the naming convention for other test types used in Chromium.
> > 
> > OK, I took a look.  I think the word instrumentation is redundant, given
that
> is
> > the only type of test running on ContentShell, test shell, etc.  How about
> > '%s_tests' % test.name?
> 

The instrumentation tests use ChromiumTestShell, which is so named to
distinguish itself from webkit test shell.  Let's see what the other reviewers
think.

[xusydoc (do not use), 2013-01-04 17:09:21]

In general it looks good, but I'm in tune enough with the android side of things
to give approval.

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bb_...
File build/android/buildbot/bb_bot_steps.py (right):

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bb_...
build/android/buildbot/bb_bot_steps.py:181: if __name__ == '__main__':
2 spaces

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bb_...
File build/android/buildbot/bb_run_tests.py (right):

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bb_...
build/android/buildbot/bb_run_tests.py:20: TESTING = 'BUILDBOT_TESTING' in
os.environ
2 spaces between top-level defines

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bb_...
build/android/buildbot/bb_run_tests.py:33: INSTRUMENTATION_TESTS =
dict((suite.name, suite) for suite in [
same

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bb_...
build/android/buildbot/bb_run_tests.py:51: VALID_TESTS = set(['ui', 'unit',
'webkit', 'webkit_layout'])
same

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bb_...
build/android/buildbot/bb_run_tests.py:241: if __name__ == '__main__':
same

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bui...
File build/android/buildbot/buildbot_functions.sh (right):

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bui...
build/android/buildbot/buildbot_functions.sh:25: --slave-properties=*)
I feel like this is better handled by factory properties, but I know you're
doing a port of android code here. Let's revisit this once it's in production.

[Isaac (away), 2013-01-04 17:21:37]

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bui...
File build/android/buildbot/buildbot_functions.sh (right):

https://codereview.chromium.org/11666023/diff/2003/build/android/buildbot/bui...
build/android/buildbot/buildbot_functions.sh:25: --slave-properties=*)
On 2013/01/04 17:09:21, xusydoc (do not use) wrote:
> I feel like this is better handled by factory properties, but I know you're
> doing a port of android code here. Let's revisit this once it's in production.

SG. This part isn't from a port actually. The issue with factory properties is
they require a master restart. One other benefit for android is that slave props
can be stored in the same repository as the rest of the code, simplifying some
config change that in the past have required synchronized multi-project commits.

[Isaac (away), 2013-01-05 22:37:55]

OK let's do it this way:

Chase: Could you do the review for buildbot / python style / long term infra
goals?
Yaron and Marcus to CC: FYI

[cmp, 2013-01-09 02:16:27]

lgtm with nits

xusydoc, this seems like a decent stopgap that we could use for a mid-level
solution between our current master configs and some potentially better/eventual
world.

https://codereview.chromium.org/11666023/diff/45001/build/android/buildbot/te...
File build/android/buildbot/tests/bb_run_bot_test.py (right):

https://codereview.chromium.org/11666023/diff/45001/build/android/buildbot/te...
build/android/buildbot/tests/bb_run_bot_test.py:8: import subprocess
sort alphanum

https://codereview.chromium.org/11666023/diff/45001/build/android/buildbot/te...
build/android/buildbot/tests/bb_run_bot_test.py:14: def main():
insert empty line before line 14

https://codereview.chromium.org/11666023/diff/45001/build/android/buildbot/te...
build/android/buildbot/tests/bb_run_bot_test.py:27: if __name__ == '__main__':
insert extra empty line before line 27

[Isaac (away), 2013-01-09 03:10:15]

Done and submitting.

https://codereview.chromium.org/11666023/diff/45001/build/android/buildbot/te...
File build/android/buildbot/tests/bb_run_bot_test.py (right):

https://codereview.chromium.org/11666023/diff/45001/build/android/buildbot/te...
build/android/buildbot/tests/bb_run_bot_test.py:8: import subprocess
On 2013/01/09 02:16:28, cmp wrote:
> sort alphanum

Done.

https://codereview.chromium.org/11666023/diff/45001/build/android/buildbot/te...
build/android/buildbot/tests/bb_run_bot_test.py:14: def main():
On 2013/01/09 02:16:28, cmp wrote:
> insert empty line before line 14

Done.

https://codereview.chromium.org/11666023/diff/45001/build/android/buildbot/te...
build/android/buildbot/tests/bb_run_bot_test.py:27: if __name__ == '__main__':
On 2013/01/09 02:16:28, cmp wrote:
> insert extra empty line before line 27

Done.  Looks like I forgot to run gpylint on this.  Re-ran it on the other new
files.

[Isaac (away), 2013-01-09 03:50:15]

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

[bulach, 2013-01-09 16:15:20]

just want to say THANKS!!!! long live python and for a 2013 with less bash
gotchas! :)