Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521513007:
Add support for a custom test_runner.py script. (Closed)
|
Created:
4 years ago by ehmaldonado_chromium Modified:
4 years ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for a custom test_runner.py script.
BUG=webrtc:6739
R=kjellander@chromium.org, jbudorick@chromium.org
Committed: https://crrev.com/5c22c2afacd399b830795330efff38f07f0c4bec
Cr-Commit-Position: refs/heads/master@{#434448}
Patch Set 1 #
Total comments: 3
Patch Set 2 : test_runner_script -> android_test_runner_script #Messages
Total messages: 25 (9 generated)
Description was changed from ========== Add support for a custom test_runner script. BUG=webrtc:6739 R=kjellander@chromium.org, jbudorick@chromium.org ========== to ========== Add support for a custom test_runner.p script. BUG=webrtc:6739 R=kjellander@chromium.org, jbudorick@chromium.org ==========
jbudorick: Would something like this be OK for you?
Description was changed from ========== Add support for a custom test_runner.p script. BUG=webrtc:6739 R=kjellander@chromium.org, jbudorick@chromium.org ========== to ========== Add support for a custom test_runner.py script. BUG=webrtc:6739 R=kjellander@chromium.org, jbudorick@chromium.org ==========
I don't have a problem with this, but I'm not sure how it'll solve the CHECKOUT_SOURCE_ROOT issue you've mentioned in the linked bug given that the webrtc wrapper is still being called in the failed runs. https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:594: if (defined(test_runner_script)) { I think this would have to be invoker.test_runner_script.
If we set that variable to our wrapper script [1] it will take care of setting the environment variable to the webrtc checkout path. [1] https://cs.chromium.org/chromium/src/third_party/webrtc/build/android/test_ru... https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:594: if (defined(test_runner_script)) { On 2016/11/23 17:09:31, jbudorick wrote: > I think this would have to be invoker.test_runner_script. I was planning to have this be a variable in //build_overrides/build.gni. It seems easier that way. Maybe I should document it there as well?
https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:594: if (defined(test_runner_script)) { On 2016/11/23 17:19:32, ehmaldonado_chromium wrote: > On 2016/11/23 17:09:31, jbudorick wrote: > > I think this would have to be invoker.test_runner_script. > I was planning to have this be a variable in //build_overrides/build.gni. It > seems easier that way. > Maybe I should document it there as well? If we can avoid forcing downstream projects like V8 to add an entry to their //build_overrides/build.gni I think it would be great. Things like that keeps blocking their autoroller and is annoying. Wasn't there a way to do it without needing that, or is that what John suggests?
On 2016/11/23 20:05:40, kjellander_chromium wrote: > https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... > build/config/android/internal_rules.gni:594: if (defined(test_runner_script)) { > On 2016/11/23 17:19:32, ehmaldonado_chromium wrote: > > On 2016/11/23 17:09:31, jbudorick wrote: > > > I think this would have to be invoker.test_runner_script. > > I was planning to have this be a variable in //build_overrides/build.gni. It > > seems easier that way. > > Maybe I should document it there as well? > > If we can avoid forcing downstream projects like V8 to add an entry to their > //build_overrides/build.gni I think it would be great. Things like that keeps > blocking their autoroller and is annoying. Wasn't there a way to do it without > needing that, or is that what John suggests? Nobody has to update //build_overrides/build.gni. We're checking if it is defined, and if it is then we add the flag. If it isn't then the flag is not added and nobody notices. :)
On 2016/11/23 20:55:07, ehmaldonado_chromium wrote: > On 2016/11/23 20:05:40, kjellander_chromium wrote: > > > https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... > > File build/config/android/internal_rules.gni (right): > > > > > https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... > > build/config/android/internal_rules.gni:594: if (defined(test_runner_script)) > { > > On 2016/11/23 17:19:32, ehmaldonado_chromium wrote: > > > On 2016/11/23 17:09:31, jbudorick wrote: > > > > I think this would have to be invoker.test_runner_script. > > > I was planning to have this be a variable in //build_overrides/build.gni. It > > > seems easier that way. > > > Maybe I should document it there as well? > > > > If we can avoid forcing downstream projects like V8 to add an entry to their > > //build_overrides/build.gni I think it would be great. Things like that keeps > > blocking their autoroller and is annoying. Wasn't there a way to do it without > > needing that, or is that what John suggests? > > Nobody has to update //build_overrides/build.gni. > We're checking if it is defined, and if it is then we add the flag. If it isn't > then the flag is not added > and nobody notices. :) Excellent, lgtm then, assuming you tried this out locally?
hmm, I might be confused about the layout of a webrtc checkout. The bot linked in the bug is using .../src/build/android/test_runner.py with various references to things in src/chromium/src/... Is that test runner the webrtc one, or the chromium one? I had thought it was the former given the existence of src/chromium, but perhaps I'm mistaken. On 2016/11/23 20:55:07, ehmaldonado_chromium wrote: > On 2016/11/23 20:05:40, kjellander_chromium wrote: > > > https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... > > File build/config/android/internal_rules.gni (right): > > > > > https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... > > build/config/android/internal_rules.gni:594: if (defined(test_runner_script)) > { > > On 2016/11/23 17:19:32, ehmaldonado_chromium wrote: > > > On 2016/11/23 17:09:31, jbudorick wrote: > > > > I think this would have to be invoker.test_runner_script. > > > I was planning to have this be a variable in //build_overrides/build.gni. It > > > seems easier that way. > > > Maybe I should document it there as well? > > > > If we can avoid forcing downstream projects like V8 to add an entry to their > > //build_overrides/build.gni I think it would be great. Things like that keeps > > blocking their autoroller and is annoying. Wasn't there a way to do it without > > needing that, or is that what John suggests? > > Nobody has to update //build_overrides/build.gni. > We're checking if it is defined, and if it is then we add the flag. If it isn't > then the flag is not added > and nobody notices. :) nit: if you're putting it in build_overrides, the name ought to be more specific -- something like "android_test_runner_script"
On 2016/11/23 22:15:33, jbudorick wrote: > hmm, I might be confused about the layout of a webrtc checkout. > > The bot linked in the bug is using .../src/build/android/test_runner.py with > various references to things in src/chromium/src/... > > Is that test runner the webrtc one, or the chromium one? I had thought it was > the former given the existence of src/chromium, but perhaps I'm mistaken. > > On 2016/11/23 20:55:07, ehmaldonado_chromium wrote: > > On 2016/11/23 20:05:40, kjellander_chromium wrote: > > > > > > https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... > > > File build/config/android/internal_rules.gni (right): > > > > > > > > > https://codereview.chromium.org/2521513007/diff/1/build/config/android/intern... > > > build/config/android/internal_rules.gni:594: if > (defined(test_runner_script)) > > { > > > On 2016/11/23 17:19:32, ehmaldonado_chromium wrote: > > > > On 2016/11/23 17:09:31, jbudorick wrote: > > > > > I think this would have to be invoker.test_runner_script. > > > > I was planning to have this be a variable in //build_overrides/build.gni. > It > > > > seems easier that way. > > > > Maybe I should document it there as well? > > > > > > If we can avoid forcing downstream projects like V8 to add an entry to their > > > //build_overrides/build.gni I think it would be great. Things like that > keeps > > > blocking their autoroller and is annoying. Wasn't there a way to do it > without > > > needing that, or is that what John suggests? > > > > Nobody has to update //build_overrides/build.gni. > > We're checking if it is defined, and if it is then we add the flag. If it > isn't > > then the flag is not added > > and nobody notices. :) > I should add: I'm not exactly sure how you intend to use this in webrtc given my confusion about the layout of a webrtc checkout, so I'm not sure if it'll work. That said, this should be fine from the chromium side, so lgtm w/ the nit below. > nit: if you're putting it in build_overrides, the name ought to be more specific > -- something like "android_test_runner_script"
The CQ bit was checked by ehmaldonado@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2521513007/#ps20001 (title: "test_runner_script -> android_test_runner_script")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The idea is to add something like android_test_runner_script = "../../webrtc/build/android/test_runner.py" to WebRTC's //build_overrides/build.gni. It will make our wrapper script be passed to the create_test_runner_script.py. Our wrapper script then will set the environment variable correctly.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/24 15:23:34, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) jbudorick: Are these real failures?
On 2016/11/24 16:27:07, ehmaldonado_chromium wrote: > On 2016/11/24 15:23:34, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > jbudorick: Are these real failures? I don't see how they could be. I suggest you retry. Example: ninja: Entering directory `/b/c/b/linux_chromeos/src/out/Release' ninja explain: stored deps info out of date for 'obj/third_party/angle/libANGLE/PathNULL.o' (1480001636 vs 1480001637) ninja explain: obj/third_party/angle/libANGLE/PathNULL.o is dirty ninja explain: obj/third_party/angle/libANGLE.a is dirty ninja explain: libGLESv2.so.TOC is dirty ninja explain: obj/third_party/angle/libANGLE.a is dirty ninja explain: libEGL.so.TOC is dirty ninja explain: libGLESv2.so.TOC is dirty ninja explain: libangle_util.so.TOC is dirty ninja explain: libEGL.so.TOC is dirty ninja explain: libGLESv2.so.TOC is dirty ninja explain: angle_end2end_tests is dirty ninja explain: obj/third_party/angle/libANGLE.a is dirty ninja explain: angle_unittests is dirty ninja explain: obj/both_gn_and_gyp.stamp is dirty ninja explain: obj/gn_all.stamp is dirty ninja explain: obj/All.stamp is dirty [1/10] CXX obj/third_party/angle/libANGLE/PathNULL.o [2/10] AR obj/third_party/angle/libANGLE.a [3/10] LINK ./angle_unittests [4/10] SOLINK ./libGLESv2.so [5/10] SOLINK ./libEGL.so [6/10] SOLINK ./libangle_util.so [7/10] LINK ./angle_end2end_tests [8/10] STAMP obj/both_gn_and_gyp.stamp [9/10] STAMP obj/gn_all.stamp [10/10] STAMP obj/All.stamp <Thread(Thread-1, started 140517594507008)> ProcessRead: proc.stdout finished. <Thread(Thread-1, started 140517594507008)> ProcessRead: cleaning up. <Thread(Thread-2, started daemon 140517586114304)> TimedFlush: Finished <Thread(Thread-1, started 140517594507008)> ProcessRead: finished. Failing build because ninja reported work to do. This means that after completing a compile, another was run and it resulted in still having work to do (that is, a no-op build wasn't a no-op). Consult the first "ninja explain:" line for a likely culprit.
The CQ bit was checked by ehmaldonado@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": 20001, "attempt_start_ts": 1480059628261210,
"parent_rev": "723fa14ae40d44a601f480b20fbc6374b69cc62b", "commit_rev":
"b7eef73a607ae6dc7186e393323723b783dc680e"}
Message was sent while issue was closed.
Description was changed from ========== Add support for a custom test_runner.py script. BUG=webrtc:6739 R=kjellander@chromium.org, jbudorick@chromium.org ========== to ========== Add support for a custom test_runner.py script. BUG=webrtc:6739 R=kjellander@chromium.org, jbudorick@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add support for a custom test_runner.py script. BUG=webrtc:6739 R=kjellander@chromium.org, jbudorick@chromium.org ========== to ========== Add support for a custom test_runner.py script. BUG=webrtc:6739 R=kjellander@chromium.org, jbudorick@chromium.org Committed: https://crrev.com/5c22c2afacd399b830795330efff38f07f0c4bec Cr-Commit-Position: refs/heads/master@{#434448} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5c22c2afacd399b830795330efff38f07f0c4bec Cr-Commit-Position: refs/heads/master@{#434448} |
