Make android's run_instrumentation_suite() use generated wrapper scripts

Make android's run_instrumentation_suite() use generated wrapper scripts

BUG=599919
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299718

[agrieve, 2016-04-02 00:44:54]

Description was changed from

==========
Make android's run_instrumentation_suite() use generated wrapper scripts

BUG=599919
==========

to

==========
Make android's run_instrumentation_suite() use generated wrapper scripts

BUG=599919
==========

[agrieve, 2016-04-02 00:44:54]

agrieve@chromium.org changed reviewers:
+ jbudorick@chromium.org

[agrieve, 2016-04-02 00:46:22]

On 2016/04/02 00:44:54, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:jbudorick@chromium.org

🎀 Did my best to search out all usages of this function, both here and upstream,
in order to make sure this will work without downstream changes.

[jbudorick, 2016-04-02 00:59:56]

jbudorick@chromium.org changed reviewers:
+ bpastene@chromium.org, martiniss@chromium.org, stip@chromium.org

[jbudorick, 2016-04-02 00:59:57]

+infra folks

Yeah, beware that this is used downstream.

This CL will be more than a little scary to land.

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_android/api.py:1245: script =
os.path.join('out', self.m.chromium.c.BUILD_CONFIG, 'bin',
this should instead be self.m.chromium.output_dir.join('bin', 'run_%s' %
wrapper_script_suite_name)

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/steps.py:1584: # TODO(agrieve): Are
these needed?
iirc yes

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipes/webrt...
File
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json
(right):

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipes/webrt...
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json:639:
"out/Release/bin/run_AppRTCDemoTest",
This doesn't seem right.

[ghost stip (do not use), 2016-04-03 00:39:23]

stoked to see more code get deleted

[agrieve, 2016-04-05 02:37:01]

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_android/api.py:1245: script =
os.path.join('out', self.m.chromium.c.BUILD_CONFIG, 'bin',
On 2016/04/02 00:59:57, jbudorick wrote:
> this should instead be self.m.chromium.output_dir.join('bin', 'run_%s' %
> wrapper_script_suite_name)

Done.

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/steps.py:1584: # TODO(agrieve): Are
these needed?
On 2016/04/02 00:59:57, jbudorick wrote:
> iirc yes

Updated comment to say that system_webview_shell_layout_test_apk should list
them as deps

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipes/webrt...
File
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json
(right):

https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipes/webrt...
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json:639:
"out/Release/bin/run_AppRTCDemoTest",
On 2016/04/02 00:59:57, jbudorick wrote:
> This doesn't seem right.

Agree it's weird, but looks to be defined here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...


Doesn't seem to be ported to GN, and I can't figure out how to generate the
target (it's not referenced by the root .gyp file)

[agrieve, 2016-04-05 02:42:53]

On 2016/04/05 02:37:01, agrieve wrote:
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
> File scripts/slave/recipe_modules/chromium_android/api.py (right):
> 
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/chromium_android/api.py:1245: script =
> os.path.join('out', self.m.chromium.c.BUILD_CONFIG, 'bin',
> On 2016/04/02 00:59:57, jbudorick wrote:
> > this should instead be self.m.chromium.output_dir.join('bin', 'run_%s' %
> > wrapper_script_suite_name)
> 
> Done.
> 
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
> File scripts/slave/recipe_modules/chromium_tests/steps.py (right):
> 
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/chromium_tests/steps.py:1584: # TODO(agrieve):
Are
> these needed?
> On 2016/04/02 00:59:57, jbudorick wrote:
> > iirc yes
> 
> Updated comment to say that system_webview_shell_layout_test_apk should list
> them as deps
> 
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipes/webrt...
> File
>
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json
> (right):
> 
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipes/webrt...
>
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json:639:
> "out/Release/bin/run_AppRTCDemoTest",
> On 2016/04/02 00:59:57, jbudorick wrote:
> > This doesn't seem right.
> 
> Agree it's weird, but looks to be defined here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> 
> Doesn't seem to be ported to GN, and I can't figure out how to generate the
> target (it's not referenced by the root .gyp file)

Also - I did attempt to inspect all downstream usages and make sure that this
change is compatible. Best I can say is that "I figure it is" though.

[agrieve, 2016-04-05 14:22:39]

On 2016/04/05 02:42:53, agrieve wrote:
> On 2016/04/05 02:37:01, agrieve wrote:
> >
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
> > File scripts/slave/recipe_modules/chromium_android/api.py (right):
> > 
> >
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
> > scripts/slave/recipe_modules/chromium_android/api.py:1245: script =
> > os.path.join('out', self.m.chromium.c.BUILD_CONFIG, 'bin',
> > On 2016/04/02 00:59:57, jbudorick wrote:
> > > this should instead be self.m.chromium.output_dir.join('bin', 'run_%s' %
> > > wrapper_script_suite_name)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
> > File scripts/slave/recipe_modules/chromium_tests/steps.py (right):
> > 
> >
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipe_module...
> > scripts/slave/recipe_modules/chromium_tests/steps.py:1584: # TODO(agrieve):
> Are
> > these needed?
> > On 2016/04/02 00:59:57, jbudorick wrote:
> > > iirc yes
> > 
> > Updated comment to say that system_webview_shell_layout_test_apk should list
> > them as deps
> > 
> >
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipes/webrt...
> > File
> >
>
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json
> > (right):
> > 
> >
>
https://codereview.chromium.org/1855663002/diff/1/scripts/slave/recipes/webrt...
> >
>
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json:639:
> > "out/Release/bin/run_AppRTCDemoTest",
> > On 2016/04/02 00:59:57, jbudorick wrote:
> > > This doesn't seem right.
> > 
> > Agree it's weird, but looks to be defined here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> > 
> > 
> > Doesn't seem to be ported to GN, and I can't figure out how to generate the
> > target (it's not referenced by the root .gyp file)
> 
> Also - I did attempt to inspect all downstream usages and make sure that this
> change is compatible. Best I can say is that "I figure it is" though.

CC'ing active sheriffs.

[jbudorick, 2016-04-05 14:29:30]

Did you update expectations between patchset 1 and 2? Not assigning to script in
api.chromium_android.test_runner _should_ have changed expectations...

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

https://codereview.chromium.org/1855663002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1245:
self.m.chromium.output_dir.join('bin', 'run_%s' %
script = self...

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

https://codereview.chromium.org/1855663002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/cronet/api.py:16: {
can we simply turn this into a list of strings instead of a list of dicts?

[agrieve, 2016-04-05 14:45:49]

Hmm, clearly didn't update them. I thought the presubmit upload check prevents
this though... Weird, or I'm wrong... Usually I'm wrong.

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

https://codereview.chromium.org/1855663002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_android/api.py:1245:
self.m.chromium.output_dir.join('bin', 'run_%s' %
On 2016/04/05 14:29:29, jbudorick wrote:
> script = self...

doh

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

https://codereview.chromium.org/1855663002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/cronet/api.py:16: {
On 2016/04/05 14:29:29, jbudorick wrote:
> can we simply turn this into a list of strings instead of a list of dicts?

No. Because of downstream (unless we used isinstance(), but I'd rather not)

[jbudorick, 2016-04-05 15:14:57]

jbudorick@chromium.org changed reviewers:
+ kjellander@chromium.org, phajdan.jr@chromium.org, sergeyberezin@chromium.org,
xunjieli@chromium.org

[jbudorick, 2016-04-05 15:14:59]

lgtm, but please wait for reviews from:

+kjellander for webrtc
+xunjieli for cronet
+phajdan.jr or +sergeyberezin for chromium_tests OWNERS

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

https://codereview.chromium.org/1855663002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/cronet/api.py:16: {
On 2016/04/05 14:45:49, agrieve wrote:
> On 2016/04/05 14:29:29, jbudorick wrote:
> > can we simply turn this into a list of strings instead of a list of dicts?
> 
> No. Because of downstream (unless we used isinstance(), but I'd rather not)

right, I forgot about the downstream cronet bots.

[Paweł Hajdan Jr., 2016-04-05 16:00:10]

LGTM w/optional comment

https://codereview.chromium.org/1855663002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1855663002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1623: name =
compile_targets[0]
It seems a bit suspicious we completely override name passed as parameter. Is
that intended? If yes, why not just remove name from list of parameters?
Otherwise, why not make this method respect it if set?

[agrieve, 2016-04-05 16:17:17]

https://codereview.chromium.org/1855663002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1855663002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1623: name =
compile_targets[0]
On 2016/04/05 16:00:10, Paweł Hajdan Jr. wrote:
> It seems a bit suspicious we completely override name passed as parameter. Is
> that intended? If yes, why not just remove name from list of parameters?
> Otherwise, why not make this method respect it if set?

Yeah, bit of a tough one.

I could make it work with keeping the name, and passing
self.compile_targets()[0] to run_instrumentation_suite. 

I thought doing the translation here would be nicer though, since it will make
the names be more consistent in the bot output (some places use Friendly Name,
some already use compile_target).

I can't change the signature to remove the parameter since it's used downstream.

[xunjieli, 2016-04-05 16:22:20]

cronet lgtm.

[martiniss, 2016-04-05 16:34:25]

The CQ bit was checked by martiniss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-05 16:34:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1855663002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1855663002/40001

[commit-bot: I haz the power, 2016-04-05 16:38:06]

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

[commit-bot: I haz the power, 2016-04-05 16:38:08]

Dry run: This issue passed the CQ dry run.

[martiniss, 2016-04-05 16:45:32]

I just did a CQ dry run; there's a new recipe engine cross repo trybot thing
which tries your change against build_internal, if that's what you're concerned
with breaking. Looks like it's doesn't have the right credentials though, so
didn't run the tests for build_internal. 

If you want to run those tests, feel free to IM me and I can walk you through
the commands to run.

[jbudorick, 2016-04-05 16:48:17]

On 2016/04/05 16:45:32, martiniss wrote:
> I just did a CQ dry run; there's a new recipe engine cross repo trybot thing
> which tries your change against build_internal, if that's what you're
concerned
> with breaking. Looks like it's doesn't have the right credentials though, so
> didn't run the tests for build_internal. 

Can't speak for Andrew, but I'm infinitely more concerned about this breaking
src than build_internal.

> 
> If you want to run those tests, feel free to IM me and I can walk you through
> the commands to run.

[kjellander_chromium, 2016-04-05 19:24:58]

This won't work for WebRTC until we spend time on getting these run scripts
working first.

https://codereview.chromium.org/1855663002/diff/40001/scripts/slave/recipes/w...
File
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json
(right):

https://codereview.chromium.org/1855663002/diff/40001/scripts/slave/recipes/w...
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json:639:
"[SLAVE_BUILD]/src/out/Release/bin/run_AppRTCDemoTest",
This doesn't work for WebRTC since this script doesn't exist for our
instrumentation tests, just the native ones. These are the generated scripts we
have today:
run_audio_codec_speed_tests 
run_common_audio_unittests 
run_modules_tests 
run_peerconnection_unittests 
run_system_wrappers_unittests 
run_tools_unittests 
run_voice_engine_unittests 
run_webrtc_perf_tests 
run_audio_decoder_unittests 
run_common_video_unittests 
run_modules_unittests 
run_rtc_unittests 
run_test_support_unittests 
run_video_engine_tests 
run_webrtc_nonparallel_tests 

I also noticed those scripts don't reference our own test runner:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/andro...
so they won't run.

Let's work together on resolving these issues. For now, can we just exclude the
WebRTC bots from this change?

[agrieve, 2016-04-05 19:47:49]

On 2016/04/05 19:24:58, kjellander (chromium) wrote:
> This won't work for WebRTC until we spend time on getting these run scripts
> working first.
> 
>
https://codereview.chromium.org/1855663002/diff/40001/scripts/slave/recipes/w...
> File
>
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json
> (right):
> 
>
https://codereview.chromium.org/1855663002/diff/40001/scripts/slave/recipes/w...
>
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json:639:
> "[SLAVE_BUILD]/src/out/Release/bin/run_AppRTCDemoTest",
> This doesn't work for WebRTC since this script doesn't exist for our
> instrumentation tests, just the native ones. These are the generated scripts
we
> have today:
> run_audio_codec_speed_tests 
> run_common_audio_unittests 
> run_modules_tests 
> run_peerconnection_unittests 
> run_system_wrappers_unittests 
> run_tools_unittests 
> run_voice_engine_unittests 
> run_webrtc_perf_tests 
> run_audio_decoder_unittests 
> run_common_video_unittests 
> run_modules_unittests 
> run_rtc_unittests 
> run_test_support_unittests 
> run_video_engine_tests 
> run_webrtc_nonparallel_tests 
> 
> I also noticed those scripts don't reference our own test runner:
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/andro...
> so they won't run.
> 
> Let's work together on resolving these issues. For now, can we just exclude
the
> WebRTC bots from this change?

Worked with martiniss to find that it didn't actually work with downstream due
to some targets not using compile_target for name. I've switched
run_instrumentation_suite to have two modes (explicit wrapper_script_suite_name,
or same as before). 

Now excluded webrtc from the change as well

[kjellander_chromium, 2016-04-05 19:57:12]

https://codereview.chromium.org/1855663002/diff/80001/scripts/slave/recipes/w...
File
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json
(left):

https://codereview.chromium.org/1855663002/diff/80001/scripts/slave/recipes/w...
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json:637:
"[SLAVE_BUILD]/src/build/android/adb_install_apk.py",
Hm, dropping the installation of the APK can't be good, right?
Or does the test runner actually take care of this for instrumentation tests as
well?

[agrieve, 2016-04-05 19:59:42]

https://codereview.chromium.org/1855663002/diff/80001/scripts/slave/recipes/w...
File
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json
(left):

https://codereview.chromium.org/1855663002/diff/80001/scripts/slave/recipes/w...
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json:637:
"[SLAVE_BUILD]/src/build/android/adb_install_apk.py",
On 2016/04/05 19:57:12, kjellander (chromium) wrote:
> Hm, dropping the installation of the APK can't be good, right?
> Or does the test runner actually take care of this for instrumentation tests
as
> well?

The test runner does now install it, but given the churn of this change already,
I'm going to revert removing this and post a separate patch for it.

[agrieve, 2016-04-05 20:05:33]

https://codereview.chromium.org/1855663002/diff/80001/scripts/slave/recipes/w...
File
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json
(left):

https://codereview.chromium.org/1855663002/diff/80001/scripts/slave/recipes/w...
scripts/slave/recipes/webrtc/standalone.expected/client_webrtc_android32_tests__l_nexus5_.json:637:
"[SLAVE_BUILD]/src/build/android/adb_install_apk.py",
On 2016/04/05 19:59:42, agrieve wrote:
> On 2016/04/05 19:57:12, kjellander (chromium) wrote:
> > Hm, dropping the installation of the APK can't be good, right?
> > Or does the test runner actually take care of this for instrumentation tests
> as
> > well?
> 
> The test runner does now install it, but given the churn of this change
already,
> I'm going to revert removing this and post a separate patch for it.

I've put it back.

[kjellander_chromium, 2016-04-05 20:09:10]

scripts/slave/recipes/webrtc: lgtm

[agrieve, 2016-04-05 23:45:41]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-04-05 23:45:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org, xunjieli@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1855663002/#ps100001
(title: "adb install")

[commit-bot: I haz the power, 2016-04-05 23:45:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1855663002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1855663002/100001

[commit-bot: I haz the power, 2016-04-05 23:51:55]

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

[commit-bot: I haz the power, 2016-04-05 23:51:56]

Try jobs failed on following builders:
  Build Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)

[agrieve, 2016-04-06 00:35:30]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-04-06 00:35:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@chromium.org, jbudorick@chromium.org, xunjieli@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1855663002/#ps120001
(title: "coverage :(")

[commit-bot: I haz the power, 2016-04-06 00:35:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1855663002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1855663002/120001

[commit-bot: I haz the power, 2016-04-06 00:43:00]

Description was changed from

==========
Make android's run_instrumentation_suite() use generated wrapper scripts

BUG=599919
==========

to

==========
Make android's run_instrumentation_suite() use generated wrapper scripts

BUG=599919

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299718
==========

[commit-bot: I haz the power, 2016-04-06 00:43:02]

Committed patchset #7 (id:120001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299718