Pull isolated perf output files for all retries instead of clobbering

The Android test runner, when retries were enabled, previously would overwrite the isolated-script-test-perf-output JSON file with the latest try, which means that all previous results would be lost. Now it will still write to the provided 'file-name.json' first, but then for following tries it will write to 'file-name_1.json', 'file-name_2.json', etc., which can allow to salvage the results in post-processing.

Unrelated change in behavior due to refactoring: --app-data-file previously used this naming scheme for outputs: ['file-name_0.ext', 'file-name_1.ext', ...]; now it's ['file-name.ext', 'file-name_1.ext', ...].

Bug: 755660
Change-Id: Id8ecb506ef91504b925178b433544faa3db8c476
Reviewed-on: https://chromium-review.googlesource.com/c/1371426
Commit-Queue: Oleh Prypin <oprypin@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615525}

build/android/pylib/local/device/local_device_gtest_run.py

@@ -69,19 +69,12 @@ class _NullContextManager(object):
     pass
 
 
-# TODO(jbudorick): Move this inside _ApkDelegate once TestPackageApk is gone.
-def PullAppFilesImpl(device, package, files, directory):
-  device_dir = device.GetApplicationDataDirectory(package)
-  host_dir = os.path.join(directory, str(device))
-  for f in files:
-    device_file = posixpath.join(device_dir, f)
-    host_file = os.path.join(host_dir, *f.split(posixpath.sep))
-    host_file_base, ext = os.path.splitext(host_file)
-    for i in itertools.count():
-      host_file = '%s_%d%s' % (host_file_base, i, ext)
-      if not os.path.exists(host_file):
-        break
-    device.PullFile(device_file, host_file)
+def _GenerateSequentialFileNames(filename):
+  """Infinite generator of names: 'name.ext', 'name_1.ext', 'name_2.ext', ..."""
+  yield filename
+  base, ext = os.path.splitext(filename)
+  for i in itertools.count(1):
+    yield '%s_%d%s' % (base, i, ext)
 
 
 def _ExtractTestsFromFilter(gtest_filter):
@@ -204,7 +197,15 @@ class _ApkDelegate(object):
       return device.ReadFile(stdout_file.name).splitlines()
 
   def PullAppFiles(self, device, files, directory):
-    PullAppFilesImpl(device, self._package, files, directory)
+    device_dir = device.GetApplicationDataDirectory(self._package)
+    host_dir = os.path.join(directory, str(device))
+    for f in files:
+      device_file = posixpath.join(device_dir, f)
+      host_file = os.path.join(host_dir, *f.split(posixpath.sep))
+      for host_file in _GenerateSequentialFileNames(host_file):
+        if not os.path.exists(host_file):
+          break
+      device.PullFile(device_file, host_file)
 
   def Clear(self, device):
     device.ClearApplicationState(self._package, permissions=self._permissions)
@@ -291,6 +292,11 @@ class LocalDeviceGtestRun(local_device_test_run.LocalDeviceTestRun):
     elif self._test_instance.exe_dist_dir:
       self._delegate = _ExeDelegate(self, self._test_instance.exe_dist_dir,
                                     self._env.tool)
+    if self._test_instance.isolated_script_test_perf_output:
+      self._test_perf_output_filenames = _GenerateSequentialFileNames(
+          self._test_instance.isolated_script_test_perf_output)
+    else:
+      self._test_perf_output_filenames = itertools.repeat(None)
     # pylint: enable=redefined-variable-type
     self._crashes = set()
     self._servers = collections.defaultdict(list)
@@ -474,6 +480,8 @@ class LocalDeviceGtestRun(local_device_test_run.LocalDeviceTestRun):
       timeout = None
     if self._test_instance.store_tombstones:
       tombstones.ClearAllTombstones(device)
+    test_perf_output_filename = next(self._test_perf_output_filenames)
+
     with device_temp_file.DeviceTempFile(
         adb=device.adb,
         dir=self._delegate.ResultsDirectory(device),
@@ -485,8 +493,7 @@ class LocalDeviceGtestRun(local_device_test_run.LocalDeviceTestRun):
         with (contextlib_ext.Optional(
             device_temp_file.DeviceTempFile(
                 adb=device.adb, dir=self._delegate.ResultsDirectory(device)),
-            self._test_instance.isolated_script_test_perf_output)
-            ) as isolated_script_test_perf_output:
+            test_perf_output_filename)) as isolated_script_test_perf_output:
 
           flags = list(self._test_instance.flags)
           if self._test_instance.enable_xml_result_parsing:
@@ -495,7 +502,7 @@ class LocalDeviceGtestRun(local_device_test_run.LocalDeviceTestRun):
           if self._test_instance.gs_test_artifacts_bucket:
             flags.append('--test_artifacts_dir=%s' % test_artifacts_dir.name)
 
-          if self._test_instance.isolated_script_test_perf_output:
+          if test_perf_output_filename:
             flags.append('--isolated_script_test_perf_output=%s'
                          % isolated_script_test_perf_output.name)
 
@@ -537,11 +544,10 @@ class LocalDeviceGtestRun(local_device_test_run.LocalDeviceTestRun):
                   str(e))
               gtest_xml = None
 
-          if self._test_instance.isolated_script_test_perf_output:
+          if test_perf_output_filename:
             try:
-              device.PullFile(
-                  isolated_script_test_perf_output.name,
-                  self._test_instance.isolated_script_test_perf_output)
+              device.PullFile(isolated_script_test_perf_output.name,
+                              test_perf_output_filename)
             except device_errors.CommandFailedError as e:
               logging.warning(
                   'Failed to pull chartjson results %s: %s',