[Android] Extract isolate logic from gtest setup to its own module.

[Android] Extract isolate logic from gtest setup to its own module.

BUG=428729
Committed: https://crrev.com/d4b611f0dd94886a27261987519f3cea88bb0f2a
Cr-Commit-Position: refs/heads/master@{#302191}

[jbudorick, 2014-10-30 14:51:35]

jbudorick@chromium.org changed reviewers:
+ klundberg@chromium.org, mikecase@chromium.org

[jbudorick, 2014-10-30 14:51:36]

klundberg: review
mikecase: FYI, since this conflicts with your instrumentation+isolate work

[klundberg, 2014-10-30 21:21:41]

Took a somewhat quick look and have a few minor questions.

I kinda wanted to ask for unit tests for this (being at an automation test
conference) but this is moving files around so I won't :-)

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
File build/android/pylib/utils/isolator.py (right):

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:58: if
os.path.exists(self._isolate_deps_dir):
What if _isolate_deps_dir isn't a directory? Will this still work or throw an
exception?

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:67: isolated_abs_path: The absolute path
to the .isolated file.
I read the doc for isolate/swarming and cannot decide whether you should explain
what .isolate and .isolated are used for here:-)
I think it would be nice with with an explanation here but I don't feel
strongly.

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:135: """Moves files from the output
directory to the top level.
"...top level" of what?

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:142: os.mkdir(paks_dir)
Do we have targets that don't have pak files?
If so, do we care about having an empty paks dir?
I assume not but just wanted to make sure.

[jbudorick, 2014-10-30 21:37:55]

Most of the logic in here is either hitting the file system directly or is
hitting isolate.py. Unit tests would be mostly mocks anyway :)

Hopefully, moving this out will make it easier for us to test other code,
though, since this is more easily mockable.

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
File build/android/pylib/utils/isolator.py (right):

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:58: if
os.path.exists(self._isolate_deps_dir):
On 2014/10/30 21:21:41, klundberg wrote:
> What if _isolate_deps_dir isn't a directory? Will this still work or throw an
> exception?

Survey says:

>>> i.Clear()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/isolator.py",
line 59, in Clear
  File "/usr/lib/python2.7/shutil.py", line 239, in rmtree
    onerror(os.listdir, path, sys.exc_info())
  File "/usr/lib/python2.7/shutil.py", line 237, in rmtree
    names = os.listdir(path)
OSError: [Errno 20] Not a directory:
'/usr/local/google/code/clankium/src/isolate_deps_dir'

Not sure what we'd want to do in this case. We currently fail in the same way:

I    0.002s Main  [host]> adb devices
W    0.019s Main  Found target
/usr/local/google/code/clankium/src/out/Debug/events_unittests_apk/events_unittests-debug.apk
I    0.019s Main  Did not find an isolate file for the test suite.
I    0.021s TimeoutThread-for-PushDataDeps(05a06cb3)  [host]> adb -s 05a06cb3
shell 'echo $EXTERNAL_STORAGE; echo %$?;'
Traceback (most recent call last):
  File "./build/android/test_runner.py", line 953, in <module>
    sys.exit(main())
  File "./build/android/test_runner.py", line 949, in main
    return command_option_parser.ParseAndExecute(option_parser)
  File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/command_option_parser.py",
line 75, in ParseAndExecute
    return cmd.run_command_func(argv[1], options, args, option_parser)
  File "./build/android/test_runner.py", line 855, in RunTestsCommand
    return _RunGTests(options, devices)
  File "./build/android/test_runner.py", line 601, in _RunGTests
    runner_factory, tests = gtest_setup.Setup(gtest_options, devices)
  File "/usr/local/google/code/clankium/src/build/android/pylib/gtest/setup.py",
line 335, in Setup
    PushDataDeps, test_options, test_package)
  File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/parallelizer.py",
line 240, in pMap
    r.pFinish(None)
  File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/parallelizer.py",
line 139, in pFinish
    self._objs.JoinAll()
  File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/reraiser_thread.py",
line 141, in JoinAll
    self._JoinAll(watcher)
  File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/reraiser_thread.py",
line 128, in _JoinAll
    thread.ReraiseIfException()
  File
"/usr/local/google/code/clankium/src/build/android/pylib/utils/reraiser_thread.py",
line 76, in run
    self._ret = self._func(*self._args, **self._kwargs)
  File "/usr/local/google/code/clankium/src/build/android/pylib/gtest/setup.py",
line 304, in PushDataDeps
    for p in os.listdir(constants.ISOLATE_DEPS_DIR)])
OSError: [Errno 20] Not a directory:
'/usr/local/google/code/clankium/src/isolate_deps_dir'

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:67: isolated_abs_path: The absolute path
to the .isolated file.
On 2014/10/30 21:21:41, klundberg wrote:
> I read the doc for isolate/swarming and cannot decide whether you should
explain
> what .isolate and .isolated are used for here:-)
> I think it would be nice with with an explanation here but I don't feel
> strongly.

I'm not sure the caller really needs to know what they are, but I guess I can
explain Isolate in a bit more detail either in here or in the class comments.

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:135: """Moves files from the output
directory to the top level.
On 2014/10/30 21:21:41, klundberg wrote:
> "...top level" of what?

... of self._isolate_deps_dir. I suppose I should add that.

https://codereview.chromium.org/690973002/diff/40001/build/android/pylib/util...
build/android/pylib/utils/isolator.py:142: os.mkdir(paks_dir)
On 2014/10/30 21:21:41, klundberg wrote:
> Do we have targets that don't have pak files?
> If so, do we care about having an empty paks dir?
> I assume not but just wanted to make sure.

Not sure / no.

[klundberg, 2014-10-30 22:24:57]

lgtm

[jbudorick, 2014-10-30 22:27:33]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-10-30 22:29:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/690973002/60001

[commit-bot: I haz the power, 2014-10-30 23:27:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-30 23:28:22]

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