Remove the android_isolate_path option from LocalGTestTest.

Remove the android_isolate_path option from LocalGTestTest.

android_isolate_path was used for specifying src/-side isolate files for
particular test suites. They're now (1) generated at build time and (2)
handled entirely on the src/-side.

BUG=633333
Committed: https://chromium.googlesource.com/chromium/tools/build/+/a661277f9f72d06f313dba599b4e6a4a7dd9887f

[jbudorick, 2016-08-09 15:38:32]

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

[Nico, 2016-08-09 16:32:50]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-08-09 16:32:51]

lgtm

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

https://codereview.chromium.org/2226753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/steps.py:322: # isolated Android
tests, rather than using custom solution in Android
does this comment still makes sense? (i think so, but not sure)

[jbudorick, 2016-08-09 16:39:26]

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

[jbudorick, 2016-08-09 16:39:26]

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

https://codereview.chromium.org/2226753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/steps.py:322: # isolated Android
tests, rather than using custom solution in Android
On 2016/08/09 16:32:51, Nico (away until Mon Aug 22) wrote:
> does this comment still makes sense? (i think so, but not sure)

I'm not sure. We're not even using isolate module in the non-android case, but
it'd still be nice to not have separate android paths in here.

+sergiyb: what was your original intention w/ this TODO?

[Sergiy Byelozyorov, 2016-08-09 16:56:40]

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

https://codereview.chromium.org/2226753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/steps.py:322: # isolated Android
tests, rather than using custom solution in Android
On 2016/08/09 16:39:26, jbudorick wrote:
> On 2016/08/09 16:32:51, Nico (away until Mon Aug 22) wrote:
> > does this comment still makes sense? (i think so, but not sure)
> 
> I'm not sure. We're not even using isolate module in the non-android case, but
> it'd still be nice to not have separate android paths in here.
> 
> +sergiyb: what was your original intention w/ this TODO?

I think someone actually fixed it and did not remove this comment, see original
CL: https://codereview.chromium.org/892463002. There we were passing
isolate_file_path to the test laucnher. OTH, I am not 100% sure that this logic
wasn't just moved elsewhere. I'd search for the CL that removed
isolate_file_path to see whether the author removed it for good or actually just
moved it.

[jbudorick, 2016-08-09 17:01:25]

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

https://codereview.chromium.org/2226753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/steps.py:327:
kwargs['isolate_file_path'] = isolate_path
sergiyb: ...are you referring to this?

[Sergiy Byelozyorov, 2016-08-09 18:22:49]

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

https://codereview.chromium.org/2226753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/steps.py:327:
kwargs['isolate_file_path'] = isolate_path
On 2016/08/09 17:01:25, jbudorick wrote:
> sergiyb: ...are you referring to this?

Ahem. Gotta look 10cm to the right (in a side-by-side diff) once in a while :-).
Yes, I mean this. I think this comment can now be removed.

[jbudorick, 2016-08-09 19:12:52]

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

https://codereview.chromium.org/2226753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/steps.py:327:
kwargs['isolate_file_path'] = isolate_path
On 2016/08/09 18:22:49, Sergiy Byelozyorov wrote:
> On 2016/08/09 17:01:25, jbudorick wrote:
> > sergiyb: ...are you referring to this?
> 
> Ahem. Gotta look 10cm to the right (in a side-by-side diff) once in a while
:-).
> Yes, I mean this. I think this comment can now be removed.

Great, thanks. Removed the TODO.

[Dirk Pranke, 2016-08-09 23:52:16]

https://codereview.chromium.org/2226753003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right):

https://codereview.chromium.org/2226753003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:1607:
steps.GTestTest('gfx_unittests', android_tool='asan'),
why are these even still listed here, rather than in the source-side files?
because of the android_tool arg?

[jbudorick, 2016-08-09 23:55:28]

https://codereview.chromium.org/2226753003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right):

https://codereview.chromium.org/2226753003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:1607:
steps.GTestTest('gfx_unittests', android_tool='asan'),
On 2016/08/09 23:52:16, Dirk Pranke wrote:
> why are these even still listed here, rather than in the source-side files?
> because of the android_tool arg?

AFAICT yes, because the gtest generator doesn't take android_tool at the moment.
I could change that and move these src/-side in a subsequent CL if you'd like.

[Dirk Pranke, 2016-08-10 00:47:55]

On 2016/08/09 23:55:28, jbudorick wrote:
>
https://codereview.chromium.org/2226753003/diff/20001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right):
> 
>
https://codereview.chromium.org/2226753003/diff/20001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:1607:
> steps.GTestTest('gfx_unittests', android_tool='asan'),
> On 2016/08/09 23:52:16, Dirk Pranke wrote:
> > why are these even still listed here, rather than in the source-side files?
> > because of the android_tool arg?
> 
> AFAICT yes, because the gtest generator doesn't take android_tool at the
moment.
> I could change that and move these src/-side in a subsequent CL if you'd like.

That would be great. Specifying tests build-side needs to go away.

lgtm in the meantime.

[jbudorick, 2016-08-10 00:48:41]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2016-08-10 00:48:41]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2226753003/#ps20001
(title: "thakis/sergiyb comment")

[commit-bot: I haz the power, 2016-08-10 00:48:51]

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-08-10 00:52:29]

Description was changed from

==========
Remove the android_isolate_path option from LocalGTestTest.

android_isolate_path was used for specifying src/-side isolate files for
particular test suites. They're now (1) generated at build time and (2)
handled entirely on the src/-side.

BUG=633333
==========

to

==========
Remove the android_isolate_path option from LocalGTestTest.

android_isolate_path was used for specifying src/-side isolate files for
particular test suites. They're now (1) generated at build time and (2)
handled entirely on the src/-side.

BUG=633333

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

[commit-bot: I haz the power, 2016-08-10 00:52:30]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/tools/build/+/a661277f9f72d06f313d...