[Offline Pages] Improvements on test harness scripts.

chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py

Index: chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py
diff --git a/chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py b/chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py
index dafd00944188b10c568b8cf5e4b56ab6a2140b9d..d239fb9c2c9e037c23649b1fc055f33049c5c7a2 100755
--- a/chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py
+++ b/chrome/browser/android/offline_pages/evaluation/run_offline_page_evaluation_test.py
@@ -26,7 +26,7 @@ import subprocess
 import sys
 
 DEFAULT_USER_REQUEST = True
-DEFAULT_USE_TEST_SCHEDULER = False
+DEFAULT_USE_TEST_SCHEDULER = True
 # 0 means the batch would be the whole list of urls.
 DEFAULT_BATCH_SIZE = 0
 DEFAULT_VERBOSE = False
@@ -59,12 +59,12 @@ def main(args):
       '--use-test-scheduler',
       dest='use_test_scheduler',
       action='store_true',
-      help='Use test scheduler to avoid real scheduling')
+      help='Use test scheduler to avoid real scheduling. Default option.')
   parser.add_argument(
       '--not-use-test-scheduler',
       dest='use_test_scheduler',
       action='store_false',
-      help='Use GCMNetworkManager for scheduling. Default option.')
+      help='Use GCMNetworkManager for scheduling.')
   parser.add_argument(
       '--batch-size',
       type=int,
@@ -93,9 +93,11 @@ def main(args):
       verbose=DEFAULT_VERBOSE)
 
   def get_adb_command(args):
+    adb_path = os.path.join(os.getcwd(),

[Pete Williamson, 2016/12/15 01:46:15]

This hardcodes the assumption that we are being run from the chromium/src
directory.  Do we already have a limitation that we have to run from there?  If
not, it might be good not to introduce a limitation.  If so, then this is fine
as is.

Also, do we know for sure this is the right version of ADB to use? 
We could end up with the device updating, and the version of adb under
third_party not getting updated right away, and we might want to be able to
override it.  If we don't know for sure, it might be better not to make this
change, but just to recommend the use of this ADB on our path in the
instructions doc.

[dougarnett, 2016/12/15 17:50:15]

I think this is much more sound - to use the same adb version for push/pull that
the test is already wired to use.

Perhaps would be good to add a comment to this fact here.

[romax, 2016/12/15 20:43:45]

There isn't a limit for the directory where you start the script, as long as you
give a correct build directory. And I just made another change to use relative
path when looking for adb. So it would not be an issue anymore.

As for the version of ADB, I personally would stick to the third_party one *for
now* since it's the only one we can make sure that we're using the same version.
I'm looking into options to change all adb usage to use catapult/devil in
third_party. I think that package is more reliable to use.

+                            'third_party/android_tools/sdk/platform-tools/adb')
     if options.device_id != None:
-      return ['adb', '-s', options.device_id] + args
-    return ['adb'] + args
+      return [adb_path, '-s', options.device_id] + args
+    return [adb_path] + args
 
   # Get the arguments and several paths.
   options, extra_args = parser.parse_known_args(args)
@@ -110,6 +112,8 @@ def main(args):
   test_runner_path = os.path.join(build_dir_path,
                                   'bin/run_chrome_public_test_apk')
   config_output_path = os.path.join(options.output_dir, CONFIG_FILENAME)
+  # In case adb server is not started
+  subprocess.call(get_adb_command(['start-server']))
   external_dir = subprocess.check_output(
       get_adb_command(['shell', 'echo', '$EXTERNAL_STORAGE'])).strip()
 
@@ -131,11 +135,14 @@ def main(args):
           ['push', config_output_path, external_dir + '/paquete/test_config']))
   subprocess.call(
       get_adb_command([
-          'push', options.test_urls_file,
-          '/sdcard/paquete/offline_eval_urls.txt'
+          'push', options.test_urls_file, external_dir +
+          '/paquete/offline_eval_urls.txt'

[Pete Williamson, 2016/12/15 01:46:15]

Does this require a corresponding change in the script to put pages at the root
instead of under /sdcard ?

[dougarnett, 2016/12/15 17:50:15]

This is just fixing an instance that was missed in previous conversion to use
$EXTERNAL_STORAGE path instead of hardcoded /sdcard path. 

[romax, 2016/12/15 20:43:45]

yeah all the path related with the one we use on the device should be fetched
from the $EXTERNAL_STORAGE env var on device and this change is fixing a bug
that I forgot to use external_dir... and just to clarify it putting pages into
'external_dir' + '/paquete/' not the root directory :)

       ]))
-  print 'Start running test...'
-
+  print 'Start running test with following configurations:'
+  print CONFIG_TEMPLATE.format(
+      is_user_requested=options.user_request,
+      use_test_scheduler=options.use_test_scheduler,
+      schedule_batch_size=options.schedule_batch_size)
   # Run test
   test_runner_cmd = [
       test_runner_path, '-f',
@@ -150,8 +157,8 @@ def main(args):
   if os.path.exists(archive_dir):
     shutil.rmtree(archive_dir)
   subprocess.call(
-      get_adb_command(
-          ['pull', external_dir + '/paquete/archives', archive_dir]))
+      get_adb_command(['pull', external_dir + '/paquete/archives', archive_dir
+                      ]))
   subprocess.call(
       get_adb_command([
           'pull', external_dir + '/paquete/offline_eval_results.txt',