[Offline Pages] Improvements on test harness scripts.

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

 #!/usr/bin/env python
 #
 # Copyright (c) 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 #
 #
 # This script is used to run OfflinePageSavePageLaterEvaluationTests.
 # The test will try to call SavePageLater on the list provided as the input,
 # and generate results of the background offlining. Then it will pull the
 # results to the output directory.
 #
 # Example Steps:
 # 1. Build chrome_public_test_apk
 # 2. Prepare a list of urls.
 # 3. Run the script (use -d when you have more than one device connected.)
 #   run_offline_page_evaluation_test.py --output-directory
 #   ~/offline_eval_short_output/ --user-requested=true -use-test-scheduler=true
 #   $CHROME_SRC/out/Default ~/offline_eval_urls.txt
 # 4. Check the results in the output directory.
 
 import argparse
 import os
 import shutil
 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
 CONFIG_FILENAME = 'test_config'
 CONFIG_TEMPLATE = """\
 IsUserRequested = {is_user_requested}
 UseTestScheduler = {use_test_scheduler}
 ScheduleBatchSize = {schedule_batch_size}
 """
 
@@ skipping 12 matching lines @@
       help='Testing as user-requested urls. Default option.')
   parser.add_argument(
       '--not-user-requested',
       dest='user_request',
       action='store_false',
       help='Testing as not user-requested urls.')
   parser.add_argument(
       '--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,
       dest='schedule_batch_size',
       help='Number of pages to be queued after previous batch completes.')
   parser.add_argument(
       '-v',
       '--verbose',
       dest='verbose',
       action='store_true',
       help='Make test runner verbose.')
   parser.add_argument(
       '-d',
       '--device',
       type=str,
       dest='device_id',
       help='Specify which device to be used. See \'adb devices\'.')
   parser.add_argument('build_output_dir', help='Path to build directory.')
   parser.add_argument(
       'test_urls_file', help='Path to input file with urls to be tested.')
   parser.set_defaults(
       output_dir=os.path.expanduser('~/offline_eval_output'),
       user_request=DEFAULT_USER_REQUEST,
       user_test_scheduler=DEFAULT_USE_TEST_SCHEDULER,
       schedule_batch_size=DEFAULT_BATCH_SIZE,
       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)
 
   if extra_args:
     print 'Unknown args: ' + ', '.join(
         extra_args) + '. Please check and run again.'
     return
 
   build_dir_path = os.path.abspath(
       os.path.join(os.getcwd(), options.build_output_dir))
   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()
 
   # Create the output directory for results, and have a copy of test config
   # there.
   if not os.path.exists(options.output_dir):
     print 'Creating output directory for results... ' + options.output_dir
     os.makedirs(options.output_dir)
   with open(config_output_path, 'w') as config:
     config.write(
         CONFIG_TEMPLATE.format(
             is_user_requested=options.user_request,
             use_test_scheduler=options.use_test_scheduler,
             schedule_batch_size=options.schedule_batch_size))
 
   print 'Uploading config file and input file onto the device.'
   subprocess.call(
       get_adb_command(
           ['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',
       'OfflinePageSavePageLaterEvaluationTest.testFailureRate'
   ]
   if options.verbose:
     test_runner_cmd += ['-v']
   subprocess.call(test_runner_cmd)
 
   print 'Fetching results from device...'
   archive_dir = os.path.join(options.output_dir, 'archives/')
   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',
           options.output_dir
       ]))
   subprocess.call(
       get_adb_command([
           'pull', external_dir + '/paquete/offline_eval_logs.txt',
           options.output_dir
       ]))
   print 'Test finished!'
 
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv[1:]))