Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2985013002:
[test_runner] support relative paths on argument --shared-prefs-file (Closed)
|
Created:
3 years, 5 months ago by shenghuazhang Modified:
3 years, 4 months ago Reviewers:
jbudorick CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[test_runner] support relative paths on argument --shared-prefs-file
Append checkout directory to head of path starting with '//'.
BUG=740131
Review-Url: https://codereview.chromium.org/2985013002
Cr-Commit-Position: refs/heads/master@{#489872}
Committed: https://chromium.googlesource.com/chromium/src/+/da447a43bfb18dec31388d70a788746b090a316b
Patch Set 1 #
Total comments: 3
Patch Set 2 : Change arg by type function #
Total comments: 5
Patch Set 3 : nit blank line #Patch Set 4 : rebase #Messages
Total messages: 26 (12 generated)
shenghuazhang@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2985013002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2985013002/diff/1/build/android/test_runner.p... build/android/test_runner.py:423: dest='shared_prefs_file', type=os.path.abspath, Type realpath changes value "//home/..." to "/home/...", so set it to abspath here, and handle the realpath below.
https://codereview.chromium.org/2985013002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2985013002/diff/1/build/android/test_runner.p... build/android/test_runner.py:423: dest='shared_prefs_file', type=os.path.abspath, On 2017/07/25 20:44:45, shenghuazhang wrote: > Type realpath changes value "//home/..." to "/home/...", so set it to abspath > here, and handle the realpath below. I think you're on the right track here, but you can do this more concisely by taking advantage of the fact that what we pass to type doesn't have to be an actual type -- it can be a function (as abspath and realpath are). If you write a function that takes a string, does the same manipulation you've got in the if block below, and then returns the result, you should be able to use that here and reuse it later for other path arguments.
https://codereview.chromium.org/2985013002/diff/1/build/android/test_runner.py File build/android/test_runner.py (right): https://codereview.chromium.org/2985013002/diff/1/build/android/test_runner.p... build/android/test_runner.py:423: dest='shared_prefs_file', type=os.path.abspath, On 2017/07/25 21:07:46, jbudorick wrote: > On 2017/07/25 20:44:45, shenghuazhang wrote: > > Type realpath changes value "//home/..." to "/home/...", so set it to abspath > > here, and handle the realpath below. > > I think you're on the right track here, but you can do this more concisely by > taking advantage of the fact that what we pass to type doesn't have to be an > actual type -- it can be a function (as abspath and realpath are). If you write > a function that takes a string, does the same manipulation you've got in the if > block below, and then returns the result, you should be able to use that here > and reuse it later for other path arguments. Thanks for this suggestion! This should be much more clean and reusable.
lgtm w/ q & nit https://codereview.chromium.org/2985013002/diff/20001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/2985013002/diff/20001/build/android/test_runn... build/android/test_runner.py:56: arg[2:].replace('/', os.sep))) curious: what's the reasoning behind this replacement? https://codereview.chromium.org/2985013002/diff/20001/build/android/test_runn... build/android/test_runner.py:58: nit: +1 blank line
https://codereview.chromium.org/2985013002/diff/20001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/2985013002/diff/20001/build/android/test_runn... build/android/test_runner.py:56: arg[2:].replace('/', os.sep))) On 2017/07/25 21:30:34, jbudorick wrote: > curious: what's the reasoning behind this replacement? I was actually not too sure if the os specific separator is necessary here, just saw a similar situation using the replacement: https://codesearch.chromium.org/chromium/build/scripts/slave/recipe_modules/c... https://codereview.chromium.org/2985013002/diff/20001/build/android/test_runn... build/android/test_runner.py:58: On 2017/07/25 21:30:34, jbudorick wrote: > nit: +1 blank line Done.
https://codereview.chromium.org/2985013002/diff/20001/build/android/test_runn... File build/android/test_runner.py (right): https://codereview.chromium.org/2985013002/diff/20001/build/android/test_runn... build/android/test_runner.py:56: arg[2:].replace('/', os.sep))) On 2017/07/25 21:38:25, shenghuazhang wrote: > On 2017/07/25 21:30:34, jbudorick wrote: > > curious: what's the reasoning behind this replacement? > > I was actually not too sure if the os specific separator is necessary here, just > saw a similar situation using the replacement: > > https://codesearch.chromium.org/chromium/build/scripts/slave/recipe_modules/c... sgtm
The CQ bit was checked by shenghuazhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2985013002/#ps40001 (title: "nit blank line")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/07/26 00:19:01, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) :( this is a flake for which I'm working on a fix.
The CQ bit was checked by shenghuazhang@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by shenghuazhang@chromium.org
The CQ bit was checked by shenghuazhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2985013002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shenghuazhang@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501106912217560,
"parent_rev": "b1af7ffb1f92f8ca46c32e330a6f715fc1bed0a7", "commit_rev":
"da447a43bfb18dec31388d70a788746b090a316b"}
Message was sent while issue was closed.
Description was changed from ========== [test_runner] support relative paths on argument --shared-prefs-file Append checkout directory to head of path starting with '//'. BUG=740131 ========== to ========== [test_runner] support relative paths on argument --shared-prefs-file Append checkout directory to head of path starting with '//'. BUG=740131 Review-Url: https://codereview.chromium.org/2985013002 Cr-Commit-Position: refs/heads/master@{#489872} Committed: https://chromium.googlesource.com/chromium/src/+/da447a43bfb18dec31388d70a788... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/da447a43bfb18dec31388d70a788... |
