Android GTest Runner: Create flags & test list files only when required

Android GTest Runner: Create flags & test list files only when required

BUG=540857
Committed: https://crrev.com/f9bf706c7d09b2b16aca8618d24a66e8cdc70973
Cr-Commit-Position: refs/heads/master@{#355225}

[agrieve, 2015-10-20 01:47:47]

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

[commit-bot: I haz the power, 2015-10-20 01:48:28]

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

[commit-bot: I haz the power, 2015-10-20 02:48:07]

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

[commit-bot: I haz the power, 2015-10-20 02:48:07]

Dry run: This issue passed the CQ dry run.

[agrieve, 2015-10-20 17:31:11]

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

[agrieve, 2015-10-20 17:31:12]

❦

[jbudorick, 2015-10-20 17:41:23]

https://codereview.chromium.org/1411153003/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1411153003/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:128:
command_line_file = None
I'll be honest, this is a lot messier than the previous version :(

It would be nice to be able to continue using DeviceTempFile as a context
manager, though doing so would involving writing a some sort of conditional
context manager that does X or Y depending on some condition Z.

[Yaron, 2015-10-20 18:03:02]

https://codereview.chromium.org/1411153003/diff/1/testing/android/native_test...
File testing/android/native_test/native_test_launcher.cc (right):

https://codereview.chromium.org/1411153003/diff/1/testing/android/native_test...
testing/android/native_test/native_test_launcher.cc:87: args.push_back("_");
Just wondering if this breaks a possible dev flow: what if I want to use the
default file for setting flags? Does test_runner overwrite it always so that's
not possible?

[jbudorick, 2015-10-20 18:08:50]

https://codereview.chromium.org/1411153003/diff/1/testing/android/native_test...
File testing/android/native_test/native_test_launcher.cc (right):

https://codereview.chromium.org/1411153003/diff/1/testing/android/native_test...
testing/android/native_test/native_test_launcher.cc:87: args.push_back("_");
On 2015/10/20 18:03:02, Yaron wrote:
> Just wondering if this breaks a possible dev flow: what if I want to use the
> default file for setting flags? Does test_runner overwrite it always so that's
> not possible?

The test runner currently doesn't overwrite the file, but it does provide its
own. The else branch would be taken here, and you'd never hit this line. (I
think the if branch in the current code has been dead code for about a month.)

After this CL, the default file still won't be used, but the test runner
sometimes won't pass a command-line file of its own, in which case we hit this
code path.

[agrieve, 2015-10-20 18:11:07]

https://codereview.chromium.org/1411153003/diff/1/testing/android/native_test...
File testing/android/native_test/native_test_launcher.cc (right):

https://codereview.chromium.org/1411153003/diff/1/testing/android/native_test...
testing/android/native_test/native_test_launcher.cc:87: args.push_back("_");
On 2015/10/20 18:03:02, Yaron wrote:
> Just wondering if this breaks a possible dev flow: what if I want to use the
> default file for setting flags? Does test_runner overwrite it always so that's
> not possible?

Yeah, tried to code search this as much as I could, but it seems that
test_runner.py is *the* way to run gunit tests, and it always specifies a flag
file.

Certainly looking for guidance though if there is another way to run them.

[jbudorick, 2015-10-20 18:14:53]

On 2015/10/20 18:11:07, agrieve wrote:
>
https://codereview.chromium.org/1411153003/diff/1/testing/android/native_test...
> File testing/android/native_test/native_test_launcher.cc (right):
> 
>
https://codereview.chromium.org/1411153003/diff/1/testing/android/native_test...
> testing/android/native_test/native_test_launcher.cc:87: args.push_back("_");
> On 2015/10/20 18:03:02, Yaron wrote:
> > Just wondering if this breaks a possible dev flow: what if I want to use the
> > default file for setting flags? Does test_runner overwrite it always so
that's
> > not possible?

Also, note that this possible dev workflow didn't work before, either. The test
runner used to write to that file path all the time:
https://codereview.chromium.org/1359333003/diff/20001/build/android/pylib/gte...

You can (once again, as of https://codereview.chromium.org/1416763003/) pass
arguments to the test runner as -a/--test-arguments, though.

> 
> Yeah, tried to code search this as much as I could, but it seems that
> test_runner.py is *the* way to run gunit tests, and it always specifies a flag
> file.
> 
> Certainly looking for guidance though if there is another way to run them.

Nope.

[agrieve, 2015-10-20 18:36:55]

https://codereview.chromium.org/1411153003/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1411153003/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:128:
command_line_file = None
On 2015/10/20 17:41:23, jbudorick wrote:
> I'll be honest, this is a lot messier than the previous version :(
> 
> It would be nice to be able to continue using DeviceTempFile as a context
> manager, though doing so would involving writing a some sort of conditional
> context manager that does X or Y depending on some condition Z.

Did some googling and came up with:
https://docs.python.org/3/library/contextlib.html#simplifying-support-for-sin...
(full background: http://bugs.python.org/issue10049)

Sadly, we're not using python3, so added a no-op context manager as a private
helper.

[jbudorick, 2015-10-20 21:17:29]

lgtm

https://codereview.chromium.org/1411153003/diff/20001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1411153003/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:49: class
_NullContextManager(object):
I may wind up extracting this at some point should it prove useful elsewhere.

https://codereview.chromium.org/1411153003/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:137:
command_line_file = _NullContextManager()
hm, not quite what I had in mind but this is fine too.

[Yaron, 2015-10-21 01:33:29]

lgtm

[agrieve, 2015-10-21 01:40:01]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-10-21 01:40:24]

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

[commit-bot: I haz the power, 2015-10-21 02:48:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-21 02:49:02]

Patchset 2 (id:??) landed as
https://crrev.com/f9bf706c7d09b2b16aca8618d24a66e8cdc70973
Cr-Commit-Position: refs/heads/master@{#355225}