WebRTC: Add android_junit_tests.

WebRTC: Add android_junit_tests.

BUG=647390
Committed: https://chromium.googlesource.com/chromium/tools/build/+/597c27e7cd712b9a07dd091a44f22af5b957c54a

[ehmaldonado_chromium, 2016-09-23 09:43:32]

ehmaldonado@chromium.org changed reviewers:
+ kjellander@chromium.org

[ehmaldonado_chromium, 2016-09-23 09:43:33]

In progress. I'd still need to add it to other builders and maybe some trybots.

What would you think of something like this? Is it OK to run it as an
AndroidTest or would it be better to run it as a WebRTCTest?

[ehmaldonado_chromium, 2016-09-23 09:45:10]

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/webrtc/steps.py (right):

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/steps.py:9: for test in
api.additional_tests.get('webrtc_tests', []):
Maybe have just one of these for now?

[kjellander_chromium, 2016-09-23 19:24:25]

Sorry for the slow review here. I think it's better to add the new test as 
WebRTCTest on the already existing Android Builder, and making them the
builder_tester bot_type instead.

Now that I think of it, I think this approach will only work for the swarming
builder though, but in that case I think it's fine to wait until we've rolled
that out (which we should be able to do next week)

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/webrtc/api.py (left):

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/api.py:135:
self.ANDROID_INSTRUMENTATION_TESTS)
+ ('android_junit_tests'),

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/webrtc/api.py (right):

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/api.py:75: self.additional_tests)
Since a 'builder_tester' can both build (+upload to google storage) and test. I
think we should convert these bots to builder_testers instead of adding this
additional configuration of 'additional_tests'

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/webrtc/builders.py (right):

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/builders.py:398: 'bot_type': 'builder',
Let's change the bot_type to 'builder_tester' instead (don't bother renaming the
bot, since that's a lot of extra work everywhere).

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/builders.py:419: 'bot_type': 'builder',
same here (builder_tester)

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/builders.py:435: 'bot_type': 'builder',
same here (builder_tester)

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/builders.py:446: 'bot_type': 'builder',
same here (builder_tester)

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/builders.py:468: 'bot_type': 'builder',
same here (builder_tester)

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/builders.py:483: 'bot_type': 'builder',
same here (builder_tester)

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/builders.py:495: 'bot_type': 'builder',
same here (builder_tester)

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/webrtc/steps.py (left):

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/webrtc/steps.py:55: 
Then just add this here (inside test_suite == 'android'):
tests.append(WebRTCTest('android_junit_tests', revision,
enable_swarming=enable_swarming))

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipes/w...
File
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_builder.json
(right):

https://codereview.chromium.org/2354363006/diff/20001/scripts/slave/recipes/w...
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_builder.json:311:
"cmd": [
This looks wrong - all the device-handling steps are added to the builder, which
doesn't have any device connected.

[ehmaldonado_chromium, 2016-09-24 06:00:02]

On 2016/09/23 19:24:25, kjellander_chromium wrote:
> Sorry for the slow review here. I think it's better to add the new test as 
> WebRTCTest on the already existing Android Builder, and making them the
> builder_tester bot_type instead.

The problem with making them builder_tester is that they will run all the tests,
which I don't think we want, unless we specify another test_suite or something.

[kjellander_chromium, 2016-09-26 14:08:44]

On 2016/09/24 06:00:02, ehmaldonado_chromium wrote:
> On 2016/09/23 19:24:25, kjellander_chromium wrote:
> > Sorry for the slow review here. I think it's better to add the new test as 
> > WebRTCTest on the already existing Android Builder, and making them the
> > builder_tester bot_type instead.
> 
> The problem with making them builder_tester is that they will run all the
tests,
> which I don't think we want, unless we specify another test_suite or
something.

You're right. My bad. I still like the idea of another test suite instead then.
Like an 'android_linux' test suite (and maybe rename the other one to
'android_device').

[ehmaldonado_chromium, 2016-09-26 14:32:31]

Patchset #3 (id:40001) has been deleted

[ehmaldonado_chromium, 2016-09-26 14:34:26]

Patchset #3 (id:60001) has been deleted

[ehmaldonado_chromium, 2016-09-26 14:40:27]

Patchset #3 (id:80001) has been deleted

[ehmaldonado_chromium, 2016-09-26 14:43:01]

On 2016/09/26 14:08:44, kjellander_chromium wrote:
> On 2016/09/24 06:00:02, ehmaldonado_chromium wrote:
> > On 2016/09/23 19:24:25, kjellander_chromium wrote:
> > > Sorry for the slow review here. I think it's better to add the new test as

> > > WebRTCTest on the already existing Android Builder, and making them the
> > > builder_tester bot_type instead.
> > 
> > The problem with making them builder_tester is that they will run all the
> tests,
> > which I don't think we want, unless we specify another test_suite or
> something.
> 
> You're right. My bad. I still like the idea of another test suite instead
then.
> Like an 'android_linux' test suite (and maybe rename the other one to
> 'android_device').

PTAL

[ehmaldonado_chromium, 2016-09-26 14:43:26]

On 2016/09/26 14:43:01, ehmaldonado_chromium wrote:
> On 2016/09/26 14:08:44, kjellander_chromium wrote:
> > On 2016/09/24 06:00:02, ehmaldonado_chromium wrote:
> > > On 2016/09/23 19:24:25, kjellander_chromium wrote:
> > > > Sorry for the slow review here. I think it's better to add the new test
as
> 
> > > > WebRTCTest on the already existing Android Builder, and making them the
> > > > builder_tester bot_type instead.
> > > 
> > > The problem with making them builder_tester is that they will run all the
> > tests,
> > > which I don't think we want, unless we specify another test_suite or
> > something.
> > 
> > You're right. My bad. I still like the idea of another test suite instead
> then.
> > Like an 'android_linux' test suite (and maybe rename the other one to
> > 'android_device').
> 
> PTAL

(It's still missing other bots)

[kjellander_chromium, 2016-09-27 05:22:29]

I like this approach! It also demonstrates the power of the recipe expectations
now that the code is getting complex with so many different combinations
possible.

Go ahead and add this for more builders now (at least Debug+Release 32+64 bit).
Since this is essentially running just in a JVM on the desktop machine I'm
wondering how many configurations make sense. I suspect the actual code that
gets run is identical for ARM/ARM64/x86/x64/MIPS. Can you investigate / compare?

https://codereview.chromium.org/2354363006/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/webrtc/api.py (right):

https://codereview.chromium.org/2354363006/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/webrtc/api.py:213: self.c.TEST_SUITE !=
'android_linux' and
Hmm, I don't like this duplication.

Can you introduce a boolean variable so these three criteria are checked only
once? Like:
run_android_device_steps = (self.m.chromium.c.TARGET_PLATFORM == 'android' and
self.c.TEST_SUITE != 'android_linux' and not self.c.enable_swarming)

Then you check that boolean at line 212 and 220.

[ehmaldonado_chromium, 2016-09-27 07:13:51]

On 2016/09/27 05:22:29, kjellander_chromium wrote:
> I like this approach! It also demonstrates the power of the recipe
expectations
> now that the code is getting complex with so many different combinations
> possible.
> 
> Go ahead and add this for more builders now (at least Debug+Release 32+64
bit).
> Since this is essentially running just in a JVM on the desktop machine I'm
> wondering how many configurations make sense. I suspect the actual code that
> gets run is identical for ARM/ARM64/x86/x64/MIPS. Can you investigate /
compare?
> 
>
https://codereview.chromium.org/2354363006/diff/140001/scripts/slave/recipe_m...
> File scripts/slave/recipe_modules/webrtc/api.py (right):
> 
>
https://codereview.chromium.org/2354363006/diff/140001/scripts/slave/recipe_m...
> scripts/slave/recipe_modules/webrtc/api.py:213: self.c.TEST_SUITE !=
> 'android_linux' and
> Hmm, I don't like this duplication.
> 
> Can you introduce a boolean variable so these three criteria are checked only
> once? Like:
> run_android_device_steps = (self.m.chromium.c.TARGET_PLATFORM == 'android' and
> self.c.TEST_SUITE != 'android_linux' and not self.c.enable_swarming)
> 
> Then you check that boolean at line 212 and 220.

PTAL
I didn't add the test to the non-swarming trybots, because there's no separation
between builder and tester.
How would you test if the code that gets run is the same for
ARM/ARM64/x86/x64/MIPS? Just investigating the BUILD.gn files?

[kjellander_chromium, 2016-09-27 10:26:28]

On 2016/09/27 07:13:51, ehmaldonado_chromium wrote:
> On 2016/09/27 05:22:29, kjellander_chromium wrote:
> > I like this approach! It also demonstrates the power of the recipe
> expectations
> > now that the code is getting complex with so many different combinations
> > possible.
> > 
> > Go ahead and add this for more builders now (at least Debug+Release 32+64
> bit).
> > Since this is essentially running just in a JVM on the desktop machine I'm
> > wondering how many configurations make sense. I suspect the actual code that
> > gets run is identical for ARM/ARM64/x86/x64/MIPS. Can you investigate /
> compare?
> > 
> >
>
https://codereview.chromium.org/2354363006/diff/140001/scripts/slave/recipe_m...
> > File scripts/slave/recipe_modules/webrtc/api.py (right):
> > 
> >
>
https://codereview.chromium.org/2354363006/diff/140001/scripts/slave/recipe_m...
> > scripts/slave/recipe_modules/webrtc/api.py:213: self.c.TEST_SUITE !=
> > 'android_linux' and
> > Hmm, I don't like this duplication.
> > 
> > Can you introduce a boolean variable so these three criteria are checked
only
> > once? Like:
> > run_android_device_steps = (self.m.chromium.c.TARGET_PLATFORM == 'android'
and
> > self.c.TEST_SUITE != 'android_linux' and not self.c.enable_swarming)
> > 
> > Then you check that boolean at line 212 and 220.
> 
> PTAL
> I didn't add the test to the non-swarming trybots, because there's no
separation
> between builder and tester.
> How would you test if the code that gets run is the same for
> ARM/ARM64/x86/x64/MIPS? Just investigating the BUILD.gn files?

I was more thinking about building the different configs (target_cpu) and check
the size (and possibly md5 checksum) of the build artifact (I assume it's
out/Default/lib.java/webrtc/android_junit_tests.jar?)
If it's identical, they are running the same code.

[ehmaldonado_chromium, 2016-09-27 14:07:01]

On 2016/09/27 10:26:28, kjellander_chromium wrote:
> On 2016/09/27 07:13:51, ehmaldonado_chromium wrote:
> > On 2016/09/27 05:22:29, kjellander_chromium wrote:
> > > I like this approach! It also demonstrates the power of the recipe
> > expectations
> > > now that the code is getting complex with so many different combinations
> > > possible.
> > > 
> > > Go ahead and add this for more builders now (at least Debug+Release 32+64
> > bit).
> > > Since this is essentially running just in a JVM on the desktop machine I'm
> > > wondering how many configurations make sense. I suspect the actual code
that
> > > gets run is identical for ARM/ARM64/x86/x64/MIPS. Can you investigate /
> > compare?
> > > 
> > >
> >
>
https://codereview.chromium.org/2354363006/diff/140001/scripts/slave/recipe_m...
> > > File scripts/slave/recipe_modules/webrtc/api.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2354363006/diff/140001/scripts/slave/recipe_m...
> > > scripts/slave/recipe_modules/webrtc/api.py:213: self.c.TEST_SUITE !=
> > > 'android_linux' and
> > > Hmm, I don't like this duplication.
> > > 
> > > Can you introduce a boolean variable so these three criteria are checked
> only
> > > once? Like:
> > > run_android_device_steps = (self.m.chromium.c.TARGET_PLATFORM == 'android'
> and
> > > self.c.TEST_SUITE != 'android_linux' and not self.c.enable_swarming)
> > > 
> > > Then you check that boolean at line 212 and 220.
> > 
> > PTAL
> > I didn't add the test to the non-swarming trybots, because there's no
> separation
> > between builder and tester.
> > How would you test if the code that gets run is the same for
> > ARM/ARM64/x86/x64/MIPS? Just investigating the BUILD.gn files?
> 
> I was more thinking about building the different configs (target_cpu) and
check
> the size (and possibly md5 checksum) of the build artifact (I assume it's
> out/Default/lib.java/webrtc/android_junit_tests.jar?)
> If it's identical, they are running the same code.

PTAL

[kjellander_chromium, 2016-09-28 06:58:21]

lgtm

[ehmaldonado_chromium, 2016-09-28 07:00:10]

The CQ bit was checked by ehmaldonado@chromium.org

[commit-bot: I haz the power, 2016-09-28 07:00:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 07:02:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 07:02:38]

Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/31886a94eb79f710)

[ehmaldonado_chromium, 2016-09-28 07:07:44]

The CQ bit was checked by ehmaldonado@chromium.org

[ehmaldonado_chromium, 2016-09-28 07:07:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@chromium.org
Link to the patchset: https://codereview.chromium.org/2354363006/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-28 07:07:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 07:11:44]

Description was changed from

==========
WebRTC: Add android_junit_tests.

BUG=647390
==========

to

==========
WebRTC: Add android_junit_tests.

BUG=647390

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/597c27e7cd712b9a07dd...
==========

[commit-bot: I haz the power, 2016-09-28 07:11:47]

Committed patchset #8 (id:200001) as
https://chromium.googlesource.com/chromium/tools/build/+/597c27e7cd712b9a07dd...

[ehmaldonado_chromium, 2016-09-28 07:28:09]

A revert of this CL (patchset #8 id:200001) has been created in
https://codereview.chromium.org/2373283002/ by ehmaldonado@chromium.org.

The reason for reverting is: Breaks buildbots
https://build.chromium.org/p/client.webrtc/builders/Android32%20Builder%20%28....