Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1324293002:
Isolate ui_android_unittests. (Closed)
|
Created:
5 years, 3 months ago by ghost stip (do not use) Modified:
5 years, 3 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIsolate ui_android_unittests.
BUG=525873
Committed: https://crrev.com/5ae0f3cfa02de9fcb8f8ab33e6944b06fccf5cf0
Cr-Commit-Position: refs/heads/master@{#348650}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Update to latest master. #Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Whitespace fix. #Patch Set 5 : Move run_ui_android_unittests to proper isolate file. #
Messages
Total messages: 27 (7 generated)
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... File ui/android/ui_android_unittests_apk.isolate (right): https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... ui/android/ui_android_unittests_apk.isolate:7: '../../build/android/test_runner.py', I know this isn't published yet, but you might be interested in using the scripts we generate in <(PRODUCT_DIR)/bin/ instead of handrolling the test_runner.py invocation.
stip@chromium.org changed reviewers: + maruel@chromium.org
https://chromiumcodereview.appspot.com/1324293002/diff/1/ui/android/ui_androi... File ui/android/ui_android_unittests_apk.isolate (right): https://chromiumcodereview.appspot.com/1324293002/diff/1/ui/android/ui_androi... ui/android/ui_android_unittests_apk.isolate:7: '../../build/android/test_runner.py', On 2015/09/04 15:10:59, jbudorick wrote: > I know this isn't published yet, but you might be interested in using the > scripts we generate in <(PRODUCT_DIR)/bin/ instead of handrolling the > test_runner.py invocation. OK, let's commit this, then change that on the next iteration.
https://codereview.chromium.org/1324293002/diff/1/build/android/android.isolate File build/android/android.isolate (right): https://codereview.chromium.org/1324293002/diff/1/build/android/android.isola... build/android/android.isolate:3: 'files': [ How many of these can/should be isolates of their own, especially among the third-party libraries? https://codereview.chromium.org/1324293002/diff/1/build/android/android.isola... build/android/android.isolate:4: './', so this isolates itself? https://codereview.chromium.org/1324293002/diff/1/build/android/android.isola... build/android/android.isolate:7: '../../third_party/android_tools/sdk/platform-tools/', Why is this in here twice? https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... File ui/android/ui_android_unittests_apk.isolate (right): https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... ui/android/ui_android_unittests_apk.isolate:7: '../../build/android/test_runner.py', On 2015/09/09 at 18:09:19, stip wrote: > On 2015/09/04 15:10:59, jbudorick wrote: > > I know this isn't published yet, but you might be interested in using the > > scripts we generate in <(PRODUCT_DIR)/bin/ instead of handrolling the > > test_runner.py invocation. > > OK, let's commit this, then change that on the next iteration. that... doesn't make a whole lot of sense to me, tbh
https://codereview.chromium.org/1324293002/diff/1/build/android/android.isolate File build/android/android.isolate (right): https://codereview.chromium.org/1324293002/diff/1/build/android/android.isola... build/android/android.isolate:1: { Add copyright header https://codereview.chromium.org/1324293002/diff/1/build/android/android.isola... build/android/android.isolate:6: '../../build/util/lib/common/', sort https://codereview.chromium.org/1324293002/diff/1/build/android/android.isola... build/android/android.isolate:7: '../../third_party/android_tools/sdk/platform-tools/', Dedupe https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android.gyp File ui/android/ui_android.gyp (right): https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android.gyp#n... ui/android/ui_android.gyp:196: 'targets': [ +2 indent https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... File ui/android/ui_android_unittests_apk.isolate (right): https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... ui/android/ui_android_unittests_apk.isolate:1: { copyright https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... ui/android/ui_android_unittests_apk.isolate:7: '../../build/android/test_runner.py', On 2015/09/09 18:09:19, stip wrote: > On 2015/09/04 15:10:59, jbudorick wrote: > > I know this isn't published yet, but you might be interested in using the > > scripts we generate in <(PRODUCT_DIR)/bin/ instead of handrolling the > > test_runner.py invocation. > > OK, let's commit this, then change that on the next iteration. The goal is to get something committed so we can adjust the recipe then iterate from there, it'll be easier than getting it perfect immediately.
https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... File ui/android/ui_android_unittests_apk.isolate (right): https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... ui/android/ui_android_unittests_apk.isolate:7: '../../build/android/test_runner.py', On 2015/09/09 18:13:45, jbudorick wrote: > that... doesn't make a whole lot of sense to me, tbh With one file and one bot live on chromium.swarm, we will be in a state to ensure this is actually working.
https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... File ui/android/ui_android_unittests_apk.isolate (right): https://codereview.chromium.org/1324293002/diff/1/ui/android/ui_android_unitt... ui/android/ui_android_unittests_apk.isolate:7: '../../build/android/test_runner.py', On 2015/09/09 at 18:15:17, M-A Ruel wrote: > On 2015/09/09 18:13:45, jbudorick wrote: > > that... doesn't make a whole lot of sense to me, tbh > > With one file and one bot live on chromium.swarm, we will be in a state to ensure this is actually working. The alternative is '<(PRODUCT_DIR)/bin/run_ui_android_unittests' and adding the same to 'files' below, which doesn't seem all that onerous.
updated. ptal https://chromiumcodereview.appspot.com/1324293002/diff/1/build/android/androi... File build/android/android.isolate (right): https://chromiumcodereview.appspot.com/1324293002/diff/1/build/android/androi... build/android/android.isolate:1: { On 2015/09/09 18:14:23, M-A Ruel wrote: > Add copyright header Done. https://chromiumcodereview.appspot.com/1324293002/diff/1/build/android/androi... build/android/android.isolate:3: 'files': [ On 2015/09/09 18:13:45, jbudorick wrote: > How many of these can/should be isolates of their own, especially among the > third-party libraries? Not sure https://chromiumcodereview.appspot.com/1324293002/diff/1/build/android/androi... build/android/android.isolate:4: './', On 2015/09/09 18:13:45, jbudorick wrote: > so this isolates itself? not anymore https://chromiumcodereview.appspot.com/1324293002/diff/1/build/android/androi... build/android/android.isolate:6: '../../build/util/lib/common/', On 2015/09/09 18:14:23, M-A Ruel wrote: > sort Done. https://chromiumcodereview.appspot.com/1324293002/diff/1/build/android/androi... build/android/android.isolate:7: '../../third_party/android_tools/sdk/platform-tools/', On 2015/09/09 18:14:23, M-A Ruel wrote: > Dedupe Done. https://chromiumcodereview.appspot.com/1324293002/diff/1/ui/android/ui_androi... File ui/android/ui_android.gyp (right): https://chromiumcodereview.appspot.com/1324293002/diff/1/ui/android/ui_androi... ui/android/ui_android.gyp:196: 'targets': [ On 2015/09/09 18:14:23, M-A Ruel wrote: > +2 indent "Done." https://chromiumcodereview.appspot.com/1324293002/diff/1/ui/android/ui_androi... File ui/android/ui_android_unittests_apk.isolate (right): https://chromiumcodereview.appspot.com/1324293002/diff/1/ui/android/ui_androi... ui/android/ui_android_unittests_apk.isolate:1: { On 2015/09/09 18:14:23, M-A Ruel wrote: > copyright Done. https://chromiumcodereview.appspot.com/1324293002/diff/1/ui/android/ui_androi... ui/android/ui_android_unittests_apk.isolate:7: '../../build/android/test_runner.py', On 2015/09/09 18:18:28, jbudorick wrote: > On 2015/09/09 at 18:15:17, M-A Ruel wrote: > > On 2015/09/09 18:13:45, jbudorick wrote: > > > that... doesn't make a whole lot of sense to me, tbh > > > > With one file and one bot live on chromium.swarm, we will be in a state to > ensure this is actually working. > > The alternative is > > '<(PRODUCT_DIR)/bin/run_ui_android_unittests' > > and adding the same to 'files' below, which doesn't seem all that onerous. Done.
lgtm
https://codereview.chromium.org/1324293002/diff/40001/build/android/android.i... File build/android/android.isolate (right): https://codereview.chromium.org/1324293002/diff/40001/build/android/android.i... build/android/android.isolate:11: '<(PRODUCT_DIR)/bin/run_ui_android_unittests', this belongs in ui_android_unittests_apk.isolate
ppptttaaalll https://codereview.chromium.org/1324293002/diff/40001/build/android/android.i... File build/android/android.isolate (right): https://codereview.chromium.org/1324293002/diff/40001/build/android/android.i... build/android/android.isolate:11: '<(PRODUCT_DIR)/bin/run_ui_android_unittests', On 2015/09/11 21:29:40, jbudorick wrote: > this belongs in ui_android_unittests_apk.isolate good catch!
lgtm
The CQ bit was checked by stip@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org Link to the patchset: https://codereview.chromium.org/1324293002/#ps80001 (title: "Move run_ui_android_unittests to proper isolate file.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324293002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324293002/80001
stip@chromium.org changed reviewers: + jdduke@chromium.org, newt@chromium.org
+jdduke@ or newt@ for OWNERS
On 2015/09/11 21:55:38, stip wrote: > +jdduke@ or newt@ for OWNERS rubberstamp lgtm
hooray! thank you!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
The CQ bit was checked by stip@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324293002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324293002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5ae0f3cfa02de9fcb8f8ab33e6944b06fccf5cf0 Cr-Commit-Position: refs/heads/master@{#348650}
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5ae0f3cfa02de9fcb8f8ab33e6944b06fccf5cf0 Cr-Commit-Position: refs/heads/master@{#348650} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
