Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2214883002:
[Android] Add known devices flag for host_info. (Closed)
|
Created:
4 years, 4 months ago by rnephew (Reviews Here) Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Add known devices flag for host_info.
Rational: If a device is expected but missing, host info should count that
as an infra failure.
BUG=
Committed: https://crrev.com/cd0530d684a39fed943df0d7a55e57c8809d0b68
Cr-Commit-Position: refs/heads/master@{#410372}
Patch Set 1 #Patch Set 2 : Add warning known_devices_file is used only in host_info #
Total comments: 3
Patch Set 3 : fix executeable to executable #Patch Set 4 : [Android] Add known devices flag for host_info. #Messages
Total messages: 17 (4 generated)
rnephew@chromium.org changed reviewers: + dpranke@chromium.org, jbudorick@chromium.org, phajdan.jr@chromium.org, simonhatch@chromium.org
Seems odd that there's no way for host_info.py to have its own specific arguments while using common.run_script... Regardless, this lgtm, though I'm not an owner.
https://codereview.chromium.org/2214883002/diff/20001/testing/scripts/host_in... File testing/scripts/host_info.py (right): https://codereview.chromium.org/2214883002/diff/20001/testing/scripts/host_in... testing/scripts/host_info.py:61: sys.executeable, Is this right?
https://codereview.chromium.org/2214883002/diff/20001/testing/scripts/host_in... File testing/scripts/host_info.py (right): https://codereview.chromium.org/2214883002/diff/20001/testing/scripts/host_in... testing/scripts/host_info.py:61: sys.executeable, On 2016/08/04 16:47:02, shatch wrote: > Is this right? Would you believe me if I said yes? Fixed.
lgtm https://codereview.chromium.org/2214883002/diff/20001/testing/scripts/host_in... File testing/scripts/host_info.py (right): https://codereview.chromium.org/2214883002/diff/20001/testing/scripts/host_in... testing/scripts/host_info.py:61: sys.executeable, On 2016/08/04 16:50:48, rnephew (Reviews Here) wrote: > On 2016/08/04 16:47:02, shatch wrote: > > Is this right? > > Would you believe me if I said yes? > Fixed. haha
How do you plan to pass --known-devices-file ? Please note there's existing --args flag you could use here. Don't feel blocked on this, although you probably need OWNERS approval. I'm fine if US OWNERS are OK though.
On 2016/08/05 14:37:47, Paweł Hajdan Jr. wrote: > How do you plan to pass --known-devices-file ? > > Please note there's existing --args flag you could use here. > > Don't feel blocked on this, although you probably need OWNERS approval. I'm fine > if US OWNERS are OK though. I'll look into the --args flag, which would make the changes in common unneeded. --known-devices-file will be passed directly by the host_info step. https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_...
On 2016/08/05 14:51:04, rnephew (Reviews Here) wrote: > On 2016/08/05 14:37:47, Paweł Hajdan Jr. wrote: > > How do you plan to pass --known-devices-file ? > > > > Please note there's existing --args flag you could use here. > > > > Don't feel blocked on this, although you probably need OWNERS approval. I'm > fine > > if US OWNERS are OK though. > > I'll look into the --args flag, which would make the changes in common unneeded. > > > --known-devices-file will be passed directly by the host_info step. > > https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_... Ok, made it use --args. If I understand it correctly, in the recipe I just pass it the args like this: args = [] extra_args = ['--known-devices-file', PATH_TO_FILE] args.extend(['--args', self.m.json.input(extra_args)]) Look at link below for example from a recipe: https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_...
LGTM Please note src/testing/script files are intended to be used from ScriptTest. If you're planning to use it "directly", consider placing it somewhere else (e.g. src/infra/scripts), since src/testing/scripts implies certain kind of interface.
On 2016/08/08 09:05:28, Paweł Hajdan Jr. wrote: > LGTM > > Please note src/testing/script files are intended to be used from ScriptTest. > > If you're planning to use it "directly", consider placing it somewhere else > (e.g. src/infra/scripts), since src/testing/scripts implies certain kind of > interface. The eventually plan is to move it to somewhere else, but to first get it working as expected.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, simonhatch@chromium.org Link to the patchset: https://codereview.chromium.org/2214883002/#ps60001 (title: "[Android] Add known devices flag for host_info.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Add known devices flag for host_info. Rational: If a device is expected but missing, host info should count that as an infra failure. BUG= ========== to ========== [Android] Add known devices flag for host_info. Rational: If a device is expected but missing, host info should count that as an infra failure. BUG= Committed: https://crrev.com/cd0530d684a39fed943df0d7a55e57c8809d0b68 Cr-Commit-Position: refs/heads/master@{#410372} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cd0530d684a39fed943df0d7a55e57c8809d0b68 Cr-Commit-Position: refs/heads/master@{#410372} |
