Closed
Bug 728993
Opened 11 years ago
Closed 11 years ago
Need to check output to capture run-as failure in devicemanagerADB
Categories
(Testing :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla13
People
(Reporter: wlach, Assigned: wlach)
Details
Attachments
(1 file, 1 obsolete file)
2.32 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
Props to Christian Holler for noticing this and providing a patch: run-as returns 0 even when it won't work, so we'll need to check for its error message. Also, we should still specify the packageName even when run-as won't work.
Assignee | ||
Comment 1•11 years ago
|
||
Here's Christian's patch against mozilla-central, modified very slightly by me for style. It looks quite straightforward and seems to give the expected behaviour to me (that is, verifyRunAs will return the appropriate exception against a non-debuggable application).
Assignee: nobody → wlachance
Attachment #599033 -
Flags: review?(jmaher)
Comment 2•11 years ago
|
||
Comment on attachment 599033 [details] [diff] [review] Need to check output to capture run-as failure in devicemanagerADB Review of attachment 599033 [details] [diff] [review]: ----------------------------------------------------------------- I like what the patch is doing, just have a couple small things to address below. ::: build/mobile/devicemanagerADB.py @@ +688,5 @@ > + # when failing because of a non-existent or non-debuggable package :( > + runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines() > + if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \ > + ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]): > + raise DMError("run-as failed sanity check") Do we want this to raise an error? Also I think this is a lot of and/or conditions for one long line, we should split it up a bit so it is easier to debug and modify in the future.
Attachment #599033 -
Flags: review?(jmaher) → review+
Comment 3•11 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #2) > ::: build/mobile/devicemanagerADB.py > @@ +688,5 @@ > > + # when failing because of a non-existent or non-debuggable package :( > > + runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines() > > + if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \ > > + ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]): > > + raise DMError("run-as failed sanity check") > > Do we want this to raise an error? Raiding the error is required, otherwise the caller (in this case the init function) cannot detect that run-as is not working and will try to use run-as although it is broken.
Comment 4•11 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #3) > Raiding the error Should of course be "Raising" >.<
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #2) > Comment on attachment 599033 [details] [diff] [review] > Need to check output to capture run-as failure in devicemanagerADB > > Review of attachment 599033 [details] [diff] [review]: > ----------------------------------------------------------------- > > I like what the patch is doing, just have a couple small things to address > below. > > ::: build/mobile/devicemanagerADB.py > @@ +688,5 @@ > > + # when failing because of a non-existent or non-debuggable package :( > > + runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines() > > + if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \ > > + ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]): > > + raise DMError("run-as failed sanity check") > > Do we want this to raise an error? Also I think this is a lot of and/or > conditions for one long line, we should split it up a bit so it is easier to > debug and modify in the future. (In reply to Christian Holler (:decoder) from comment #3) > (In reply to Joel Maher (:jmaher) from comment #2) > > > ::: build/mobile/devicemanagerADB.py > > @@ +688,5 @@ > > > + # when failing because of a non-existent or non-debuggable package :( > > > + runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines() > > > + if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \ > > > + ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]): > > > + raise DMError("run-as failed sanity check") > > > > Do we want this to raise an error? > > Raiding the error is required, otherwise the caller (in this case the init > function) cannot detect that run-as is not working and will try to use > run-as although it is broken. Yep. Honestly raising an exception seems like an odd way of communicating that a feature isn't supported, but that's how the function is designed to work at present (see Init() for where it's called). Perhaps we should consider changing this later.
Assignee | ||
Comment 6•11 years ago
|
||
This simplifies checking for the run-as string from four conditions to three, and removes the unnecessary step of splitting the output of calling the process into lines.
Attachment #599033 -
Attachment is obsolete: true
Attachment #599166 -
Flags: review+
Comment 7•11 years ago
|
||
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/9964bcbdc4e2
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9964bcbdc4e2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•