Closed Bug 705192 Opened 8 years ago Closed 8 years ago

remotexpcshelltests.py cannot execute xpcshell via SUT agent

Categories

(Testing :: XPCShell Harness, defect)

x86
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla13

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(1 file, 7 obsolete files)

remotexpcshelltests.py uses devicemanager.runCmd to execute xpcshell, but runCmd is not implemented in devicemanagerSUT.
OS: Mac OS X → Android
Attached patch wip (obsolete) — Splinter Review
I have changed launchProcess to use the devicemanager's launchProcess, and updated the adb launchProcess significantly. It works fine with adb, but there are still a couple of problems with sut.
I have found several problems trying to launch xpcshell via devicemanagerSUT:
 - getDeviceRoot uses the SUT testroot command, which returns /mnt/sdcard; xpcshell cannot be executed from sdcard because we cannot chmod +x it in that location; I have updated testroot to return /data/local, if /data/local/tests exists -- just like we did in devicemanagerADB
 - devicemanagerSUT.launchProcess appends "> <deviceroot>/process.txt" to the command line when no output file is specified. This ends up being passed as arguments to Runtime.exec(), which fails. I expect redirection is a shell responsibility and the > is not interpretted by Runtime.exec(). It's probably best to just remove this redirection -- the agent's RedirOutputThread is going to redirect output anyway.
 - devicemanagerSUT.launchProcess makes no allowance for its cwd parameter.
And also...
 - remotexpcshelltests.py surrounds some command line arguments with single quotes: eg -e '_execute_test(); quit();'. The quotes are necessary when invoked via the adb shell, but cause xpcshell to fail silently when invoked via sut. I have moved the quoting code out of remotexpcshelltests.py and into devicemanagerADB.
And also...bug 695020 affects xpcshell significantly -- xpcshell typically completes before processExists() detects it.
Depends on: 695020
Attached patch wip - updated for bitrot (obsolete) — Splinter Review
Can execute xpcshell via ADB or SUT with this patch. With SUT, there is a little more work required to return / report results.
Attachment #577858 - Attachment is obsolete: true
Promising, but lots of testing still to do: How do these changes affect other test suites, etc...
Attachment #596729 - Attachment is obsolete: true
please check the changes in bug 728928 for devicemanager and process launching.
...actually bug 728298
It was relatively straight-forward to adapt the patch to use shell(), but I had to add some additional features to shell():
 - environment handling
 - cwd handling
 - character escapes
Attachment #598424 - Attachment is obsolete: true
Attachment #599255 - Attachment is obsolete: true
Depends on: 730153
Attached patch simpli (obsolete) — Splinter Review
:wlach -- note that I had 2 issues with the devicemanager shell work:
 - on ADB, the manual escaping of characters + list2cmdline results in double quoting of some xpcshell arguments, resulting in failures; I removed list2cmdline
 - _pop_last_line() fails when the file has a single line; I adjusted the character indexing
Attachment #599771 - Attachment is obsolete: true
Attachment #600225 - Attachment is obsolete: true
Attachment #600229 - Flags: review?(jmaher)
Attachment #600229 - Flags: feedback?(wlachance)
Comment on attachment 600229 [details] [diff] [review]
simplified patch based on the good work in bug 728298

Looks great, sorry about the bugs in devicemanager. My only suggestion would be to run things through try before pushing.
Attachment #600229 - Flags: feedback?(wlachance) → feedback+
Comment on attachment 600229 [details] [diff] [review]
simplified patch based on the good work in bug 728298

Review of attachment 600229 [details] [diff] [review]:
-----------------------------------------------------------------

the changes to remoteautomation.py will affect all other unittests, so we need this on try server.
Attachment #600229 - Flags: review?(jmaher) → review+
are we ready to check this in?  After this lands, what else is left?
Yes - ready for check-in. 

Bug 730152 is needed to tidy up hung xpcshell's...that's about all that I can think of.
Keywords: checkin-needed
Sorry...I meant bug 730153.
https://hg.mozilla.org/mozilla-central/rev/785345035a3b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.