Add shared preference file modification option to instrumentation test runner

Add shared preference file modification option to instrumentation test runner

Adds an --edit-shared-prefs option to the instrumentation test runner that
can be used to edit shared preference files before a test is run.

BUG=671373
Review-Url: https://codereview.chromium.org/2688913003
Cr-Commit-Position: refs/heads/master@{#451084}
Committed: https://chromium.googlesource.com/chromium/src/+/654a5af38665d42fdcddafaa0a5f85810d5fdacc

[bsheedy, 2017-02-10 18:38:20]

Description was changed from

==========
Add shared preference file modification option to instrumentation test runner

Adds an --edit-shared-prefs option to the instrumentation test runner that
can be used to edit shared preference files before a test is run.

This is related to the change in

BUG=671373
==========

to

==========
Add shared preference file modification option to instrumentation test runner

Adds an --edit-shared-prefs option to the instrumentation test runner that
can be used to edit shared preference files before a test is run.

This is related to the change in
https://chromium-review.googlesource.com/c/440475/

BUG=671373
==========

[bsheedy, 2017-02-10 18:42:55]

bsheedy@chromium.org changed reviewers:
+ jbudorick@chromium.org

[bsheedy, 2017-02-10 18:42:56]

PTAL, feel free to hand off to someone else if more appropriate/too busy.

[bsheedy, 2017-02-15 22:05:18]

Description was changed from

==========
Add shared preference file modification option to instrumentation test runner

Adds an --edit-shared-prefs option to the instrumentation test runner that
can be used to edit shared preference files before a test is run.

This is related to the change in
https://chromium-review.googlesource.com/c/440475/

BUG=671373
==========

to

==========
Add shared preference file modification option to instrumentation test runner

Adds an --edit-shared-prefs option to the instrumentation test runner that
can be used to edit shared preference files before a test is run.

BUG=671373
==========

[bsheedy, 2017-02-15 22:08:44]

bsheedy@chromium.org changed reviewers:
+ dtrainor@chromium.org

[bsheedy, 2017-02-15 22:08:45]

+dtrainor for addition of files under chrome/android

[jbudorick, 2017-02-15 22:38:42]

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/ins...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/ins...
build/android/pylib/instrumentation/instrumentation_test_instance.py:691: def
unicode_to_str(data):
Can we do this with one of the optional arguments to json.load? (encoding?)

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/ins...
build/android/pylib/instrumentation/instrumentation_test_instance.py:701:
filepath = os.path.abspath(os.path.join(
Remove this after the realpath change mentioned in test_runner.py.

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

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:124:
if type(value) is bool:
nit: use isinstance for all of these.

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:126:
elif type(value) is str:
isinstance(value, basestring) here

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:133:
logging.warning("Given invalid value type %s for key %s",
Should this be fatal? It seems bad to be running w/o an intended preference, and
a warning will be ~ invisible in most cases.

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
build/android/test_runner.py:403: group.add_argument('--shared-prefs-file',
dest='shared_prefs_file',
nit: add type=os.path.realpath.

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
build/android/test_runner.py:408: '[{'
nit: how does this appear in the actual --help text? It's nicely formatted here,
but I'm not sure that'll carry over.

[bsheedy, 2017-02-15 22:54:52]

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
build/android/test_runner.py:403: group.add_argument('--shared-prefs-file',
dest='shared_prefs_file',
On 2017/02/15 22:38:42, jbudorick wrote:
> nit: add type=os.path.realpath.

I initially did this, but then it appends the current directory to the front,
e.g. if we're in //third_party and run the test runner with --shared-prefs-file
chrome/android/example.json, the path will end up as
//third_party/chrome/android/example.json. If it's fine to have a relative path
from wherever the script is being run instead of a path relative to the source
root directory, then I can make this change.

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
build/android/test_runner.py:408: '[{'
On 2017/02/15 22:38:42, jbudorick wrote:
> nit: how does this appear in the actual --help text? It's nicely formatted
here,
> but I'm not sure that'll carry over.

It doesn't look nicely formatted when --help is used. Manually formatting with
newlines doesn't work unless you set an option in the ArgumentParser that says
not to do any automatic formatting, but then all the other help texts become
long lines of text. I can just remove the example if that's preferable to a
not-so-nicely-formatted example.

[bsheedy, 2017-02-16 00:14:39]

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/ins...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/ins...
build/android/pylib/instrumentation/instrumentation_test_instance.py:691: def
unicode_to_str(data):
On 2017/02/15 22:38:41, jbudorick wrote:
> Can we do this with one of the optional arguments to json.load? (encoding?)

Yes and no. Setting the encoding argument is only meant to be used if the file
is encoded in something besides UTF-8, and so doesn't do anything in this case.
You can specify the object_hook argument to do the re-encoding during json.load
instead of afterwards, but that doesn't result in less code or anything. See
http://stackoverflow.com/questions/956867/how-to-get-string-objects-instead-o...

[bsheedy, 2017-02-16 00:28:00]

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

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:124:
if type(value) is bool:
On 2017/02/15 22:38:42, jbudorick wrote:
> nit: use isinstance for all of these.

Done.

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:126:
elif type(value) is str:
On 2017/02/15 22:38:41, jbudorick wrote:
> isinstance(value, basestring) here

Done.

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:133:
logging.warning("Given invalid value type %s for key %s",
On 2017/02/15 22:38:41, jbudorick wrote:
> Should this be fatal? It seems bad to be running w/o an intended preference,
and
> a warning will be ~ invisible in most cases.

Done.

[David Trainor- moved to gerrit, 2017-02-16 00:42:15]

chrome/android json files lgtm

[jbudorick, 2017-02-16 02:57:09]

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
build/android/test_runner.py:403: group.add_argument('--shared-prefs-file',
dest='shared_prefs_file',
On 2017/02/15 22:54:52, bsheedy wrote:
> On 2017/02/15 22:38:42, jbudorick wrote:
> > nit: add type=os.path.realpath.
> 
> I initially did this, but then it appends the current directory to the front,
> e.g. if we're in //third_party and run the test runner with
--shared-prefs-file
> chrome/android/example.json, the path will end up as
> //third_party/chrome/android/example.json. If it's fine to have a relative
path
> from wherever the script is being run instead of a path relative to the source
> root directory, then I can make this change.

Yeah, that's fine. I think you're more likely to run it w/ a path relative to
your current directory than one relative to src if you're using a relative
directory. The recipes will give it an absolute path.

(realpath also handles symlinks, which is a nice bonus.)

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
build/android/test_runner.py:408: '[{'
On 2017/02/15 22:54:52, bsheedy wrote:
> On 2017/02/15 22:38:42, jbudorick wrote:
> > nit: how does this appear in the actual --help text? It's nicely formatted
> here,
> > but I'm not sure that'll carry over.
> 
> It doesn't look nicely formatted when --help is used. Manually formatting with
> newlines doesn't work unless you set an option in the ArgumentParser that says
> not to do any automatic formatting, but then all the other help texts become
> long lines of text. I can just remove the example if that's preferable to a
> not-so-nicely-formatted example.

eh, I think it's ok for now.

https://codereview.chromium.org/2688913003/diff/40001/chrome/android/shared_p...
File
chrome/android/shared_preference_files/vr_cardboard_skipdon_setupcomplete.json
(right):

https://codereview.chromium.org/2688913003/diff/40001/chrome/android/shared_p...
chrome/android/shared_preference_files/vr_cardboard_skipdon_setupcomplete.json:1:
[
nit: I think these should have a /test/ directory in their path somewhere to
make it clear that they're specifically files for use in testing. Otherwise,
it's not clear that that's the case.

[bsheedy, 2017-02-16 18:04:50]

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/ins...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/2688913003/diff/20001/build/android/pylib/ins...
build/android/pylib/instrumentation/instrumentation_test_instance.py:701:
filepath = os.path.abspath(os.path.join(
On 2017/02/15 22:38:41, jbudorick wrote:
> Remove this after the realpath change mentioned in test_runner.py.

Done.

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
build/android/test_runner.py:403: group.add_argument('--shared-prefs-file',
dest='shared_prefs_file',
On 2017/02/16 02:57:09, jbudorick wrote:
> On 2017/02/15 22:54:52, bsheedy wrote:
> > On 2017/02/15 22:38:42, jbudorick wrote:
> > > nit: add type=os.path.realpath.
> > 
> > I initially did this, but then it appends the current directory to the
front,
> > e.g. if we're in //third_party and run the test runner with
> --shared-prefs-file
> > chrome/android/example.json, the path will end up as
> > //third_party/chrome/android/example.json. If it's fine to have a relative
> path
> > from wherever the script is being run instead of a path relative to the
source
> > root directory, then I can make this change.
> 
> Yeah, that's fine. I think you're more likely to run it w/ a path relative to
> your current directory than one relative to src if you're using a relative
> directory. The recipes will give it an absolute path.
> 
> (realpath also handles symlinks, which is a nice bonus.)

Done.

https://codereview.chromium.org/2688913003/diff/20001/build/android/test_runn...
build/android/test_runner.py:408: '[{'
On 2017/02/16 02:57:08, jbudorick wrote:
> On 2017/02/15 22:54:52, bsheedy wrote:
> > On 2017/02/15 22:38:42, jbudorick wrote:
> > > nit: how does this appear in the actual --help text? It's nicely formatted
> > here,
> > > but I'm not sure that'll carry over.
> > 
> > It doesn't look nicely formatted when --help is used. Manually formatting
with
> > newlines doesn't work unless you set an option in the ArgumentParser that
says
> > not to do any automatic formatting, but then all the other help texts become
> > long lines of text. I can just remove the example if that's preferable to a
> > not-so-nicely-formatted example.
> 
> eh, I think it's ok for now.

Done.

https://codereview.chromium.org/2688913003/diff/40001/chrome/android/shared_p...
File
chrome/android/shared_preference_files/vr_cardboard_skipdon_setupcomplete.json
(right):

https://codereview.chromium.org/2688913003/diff/40001/chrome/android/shared_p...
chrome/android/shared_preference_files/vr_cardboard_skipdon_setupcomplete.json:1:
[
On 2017/02/16 02:57:09, jbudorick wrote:
> nit: I think these should have a /test/ directory in their path somewhere to
> make it clear that they're specifically files for use in testing. Otherwise,
> it's not clear that that's the case.

Done.

[jbudorick, 2017-02-16 18:07:43]

lgtm

[bsheedy, 2017-02-16 18:16:14]

The CQ bit was checked by bsheedy@chromium.org

[bsheedy, 2017-02-16 18:16:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2688913003/#ps60001
(title: "Address more comments")

[commit-bot: I haz the power, 2017-02-16 18:16:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 19:39:03]

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

[commit-bot: I haz the power, 2017-02-16 19:39:05]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[bsheedy, 2017-02-16 19:44:13]

The CQ bit was checked by bsheedy@chromium.org

[commit-bot: I haz the power, 2017-02-16 19:45:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 20:25:06]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487274253292140,
"parent_rev": "6dfef54be7c38029b0d1a34676a9d8022d34bb42", "commit_rev":
"654a5af38665d42fdcddafaa0a5f85810d5fdacc"}

[commit-bot: I haz the power, 2017-02-16 20:25:48]

Description was changed from

==========
Add shared preference file modification option to instrumentation test runner

Adds an --edit-shared-prefs option to the instrumentation test runner that
can be used to edit shared preference files before a test is run.

BUG=671373
==========

to

==========
Add shared preference file modification option to instrumentation test runner

Adds an --edit-shared-prefs option to the instrumentation test runner that
can be used to edit shared preference files before a test is run.

BUG=671373

Review-Url: https://codereview.chromium.org/2688913003
Cr-Commit-Position: refs/heads/master@{#451084}
Committed:
https://chromium.googlesource.com/chromium/src/+/654a5af38665d42fdcddafaa0a5f...
==========

[commit-bot: I haz the power, 2017-02-16 20:25:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/654a5af38665d42fdcddafaa0a5f...