811411 - Add ability to run C++ unit tests on mobile

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[(not currently active) Ted Mielczarek]

Currently our C++ unit tests just run during "make check". I wrote a little Python harness recently to manage running them:
http://mxr.mozilla.org/mozilla-central/source/testing/runcppunittests.py

It should be fairly straightforward to make a version of this that works on mobile. This would be nice because it'd let us expand this test coverage to mobile platforms.

Comment 1

[(not currently active) Ted Mielczarek]

Fixing this + bug 811409 would let us get these tests running on mobile in automation.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> Fixing this + bug 811409 would let us get these tests running on mobile in
> automation.

Do you mean fixing this + bug 811404?  Or do we actually need to fix all three?  Or am I just misunderstanding?

Comment 3

[(not currently active) Ted Mielczarek]

bug 811404 is a prerequisite of bug 811409. This bug is related, but not required by either of those. To get C++ unit tests running on mobile devices in automation we'll have to fix all three.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP script to remote CPP unit tests remotely, v1

Ted and I paired on this script today, in large part by grafting together pieces from remotexpcshelltests.py and runcppunittests.py and seasoning to taste.  So far, we've got it setting up an environment in which to the run tests, tested under adb, and the next step is to copy over the tests themselves in a way that they're runnable.

An example invocation: 

$objdir/_virtualenv/bin/python ./testing/remotecppunittests.py --xre-path=$objdir/dist/bin --dm_trans=adb $objdir/xpcom/tests/TestFile $objdir/media/mtransport/test/runnable_utils_unittest

While investigating how to get the tests into a spot where they can be executed, we found out that they can't run from the SD card, and thus the magic /data/local/tests directory MUST pre-exist on the Android device.  The next thing to figure out is why, when we push a test file to the right place by hand, and set LD_LIBRARY_PATH and MOZ_LINKER_CACHE on the device to things that we think make sense, we're still seeing this:

shell@android:/data/local/tests/cppunittests/tmp $ ./TestTimers                
link_image[1937]:  3028 could not load needed library 'libmozglue.so' for './TestTimers' (load_library[1092]: Library 'libmozglue.so' not found)CANNOT LINK EXECUTABLE

Even thought libmozglue.so is in the aforementioned paths.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

In attempting to verify that remotexpcshelltests.py itself is working correctly, I invoked it with 

$objdir/_virtualenv/bin/python ./testing/xpcshell/remotexpcshelltests.py --objdir=$objdir --dm_trans=adb ./testing/xpcshell/xpcshell.ini 

That took a very long time to copy over a boatload of files, and ultimately blew up thusly:

chmod /data/local/tests/xpcshell/b/defaults/pref
chmod /data/local/tests/xpcshell/b/defaults
Unable to chmod /data/local/tests/xpcshell/b/fennec-19.0a1.en-US.android-arm.apk: Operation not permitted
chmod /data/local/tests/xpcshell/b/fennec-19.0a1.en-US.android-arm.apk
Unable to chmod /data/local/tests/xpcshell/b/libfreebl3.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libfreebl3.so
Unable to chmod /data/local/tests/xpcshell/b/libmozalloc.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libmozalloc.so
Unable to chmod /data/local/tests/xpcshell/b/libmozglue.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libmozglue.so
Unable to chmod /data/local/tests/xpcshell/b/libmozsqlite3.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libmozsqlite3.so
Unable to chmod /data/local/tests/xpcshell/b/libnspr4.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libnspr4.so
Unable to chmod /data/local/tests/xpcshell/b/libnss3.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libnss3.so
Unable to chmod /data/local/tests/xpcshell/b/libnssckbi.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libnssckbi.so
Unable to chmod /data/local/tests/xpcshell/b/libnssutil3.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libnssutil3.so
Unable to chmod /data/local/tests/xpcshell/b/libomxplugin.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libomxplugin.so
Unable to chmod /data/local/tests/xpcshell/b/libplc4.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libplc4.so
Unable to chmod /data/local/tests/xpcshell/b/libplds4.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libplds4.so
Unable to chmod /data/local/tests/xpcshell/b/libplugin-container.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libplugin-container.so
Unable to chmod /data/local/tests/xpcshell/b/libsmime3.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libsmime3.so
Unable to chmod /data/local/tests/xpcshell/b/libsoftokn3.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libsoftokn3.so
Unable to chmod /data/local/tests/xpcshell/b/libsoundtouch.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libsoundtouch.so
Unable to chmod /data/local/tests/xpcshell/b/libssl3.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libssl3.so
Unable to chmod /data/local/tests/xpcshell/b/libxpcom.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libxpcom.so
Unable to chmod /data/local/tests/xpcshell/b/libxul.so: Operation not permitted
chmod /data/local/tests/xpcshell/b/libxul.so
Unable to chmod /data/local/tests/xpcshell/b/xpcshell: Operation not permitted
chmod /data/local/tests/xpcshell/b/xpcshell
Unable to chmod /data/local/tests/xpcshell/b/xpcw: Operation not permitted
chmod /data/local/tests/xpcshell/b/xpcw
chmod /data/local/tests/xpcshell/b
Error: couldn't find mozinfo.json at '/Users/dmose/r/inbound/src/testing/xpcshell/mozinfo.json'. Perhaps you need to use --build-info-json?


The json error message seems nicely helpful; it's not yet clear to me what's going on with the chmod stuff.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Dan Mosedale (:dmose) from comment #4)
> 
> shell@android:/data/local/tests/cppunittests/tmp $ ./TestTimers             
> 
> link_image[1937]:  3028 could not load needed library 'libmozglue.so' for
> './TestTimers' (load_library[1092]: Library 'libmozglue.so' not found)CANNOT
> LINK EXECUTABLE
> 
> Even thought libmozglue.so is in the aforementioned paths.

So the problem here is that the .so files were being installed without read permissions (they all had 001 perms).  Changing them to (eg 644) by hand made things work; need to add this to the script.  I've got tests executing by hand, and see how to move this script forward.  However, going to prioritize getting gdb working tonight so that we make best of use ekr's time in SF tomorrow.

Ted, if you felt like pushing on the script a bit in the next day or two, I wouldn't feel terrible about that.  :-)

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP script to run CPP unit tests remotely

Comment 8

[Geoff Brown [:gbrown]]

Attachment: wip - geoff's mods

My modifications to date. I added a "check-remote" make target based on TEST_PATH. 

eg.

export TEST_PATH="testFile testTimers"
make check-remote

Also, this adds a --localLib option to make it easier to specify which libs to push. The default is <objdir>/dist/fennec to encourage use of stripped libs.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Ooh, this looks like a fine start; thanks, Geoff!  I should have a chance to take this for a test drive later today or early tomorrow.

Comment 10

[Geoff Brown [:gbrown]]

Attachment: wip - geoff's mods

This rev adds a custom process environment for remote execution and executes the test via devicemanager.shell.

eg 

$ make check-remote
Android Debug Bridge version 1.0.31

will execute commands via run-as org.mozilla.fennec_mozdev
will use zip to push directories
993 KB/s (318061 bytes in 0.312s)
955 KB/s (361361 bytes in 0.369s)
remotecppunittests INFO | Running test TestFile
Running nsLocalFile tests...
TEST-UNEXPECTED-FAIL | main Creating temp directory, rv=80004005
Finished running nsLocalFile tests.
remotecppunittests TEST-UNEXPECTED-FAIL | TestFile | test failed with return code 1
remotecppunittests INFO | Running test TestTimers
Running TestTimers tests...
Finished running TestTimers tests.
grep: check-remote.log: No such file or directory
/bin/sh: @errors=: not found

Comment 11

[(not currently active) Ted Mielczarek]

I'm not sure that I'm super-excited about making the target "check-remote", since "make check" is a complicated mishmash of things, but I'm not going to bikeshed it. My plan is to stop running these under "make check" on the tinderboxes once we get them running from packaged builds.

Comment 12

[Geoff Brown [:gbrown]]

Attachment: wip - geoff's mods

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

OK, things are definitely moving forward.  Thanks for taking up the charge!  As we discussed in IRC, there are a few issues:

* the biggest one is that the .so files are being pushed with permissions 001, which causes the executable to not run.  This bug predated your changes, and may be somehow specific to my environment.

* sometimes the script also blows up trying to push libfreebl3.so, the first library.  Re-running the script generally fixes the problem.  This bug also predates your changes.  Here's a log:

make check-remote
Android Debug Bridge version 1.0.29

'cp' not found, but 'dd' was found as a replacement
0+0 records in
0+0 records out
0 bytes transferred in 0.001 secs (0 bytes/sec)
will execute commands via run-as org.mozilla.fennec_dmose
Pushing libfreebl3.so..
5582 KB/s (373468 bytes in 0.065s)
Traceback (most recent call last):
  File "/Users/dmose/r/inbound/src/config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "/Users/dmose/r/inbound/src/config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "/Users/dmose/r/inbound/src/testing/remotecppunittests.py", line 180, in <module>
    tester = RemoteCPPUnitTests(dm, options)
  File "/Users/dmose/r/inbound/src/testing/remotecppunittests.py", line 25, in __init__
    self.setupUtilities()
  File "/Users/dmose/r/inbound/src/testing/remotecppunittests.py", line 47, in setupUtilities
    self.pushLibs()
  File "/Users/dmose/r/inbound/src/testing/remotecppunittests.py", line 66, in pushLibs
    self.device.pushFile(os.path.join(localLib, file), remoteFile)
  File "/Users/dmose/r/inbound/src/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py", line 183, in pushFile
    self.shellCheckOutput(["dd", "if=" + remoteTmpFile, "of=" + destname])
  File "/Users/dmose/r/inbound/src/testing/mozbase/mozdevice/mozdevice/devicemanager.py", line 65, in shellCheckOutput
    raise DMError("Non-zero return code for command: %s (output: '%s', retval: '%s')" % (cmd, output, retval))
mozdevice.devicemanager.DMError: Non-zero return code for command: ['dd', 'if=/data/local/tests/tmp/libfreebl3.so', 'of=/data/local/tests/cppunittests/b/libfreebl3.so'] (output: '/data/local/tests/cppunittests/b/libfreebl3.so: cannot open for write: No such file or directory', retval: '1')
grep: check-remote.log: No such file or directory
/bin/sh: @errors=: command not found


* Finally, there's an odd error message when trying to cd to /tmp, I'm not sure if it's actually causing any sort of problem.  Here's an example log:

make check-remote
Android Debug Bridge version 1.0.29

'cp' not found, but 'dd' was found as a replacement
0+0 records in
0+0 records out
0 bytes transferred in 0.001 secs (0 bytes/sec)
will execute commands via run-as org.mozilla.fennec_dmose
Pushing libfreebl3.so..
2920 KB/s (373468 bytes in 0.124s)
Pushing libmozalloc.so..
3732 KB/s (13996 bytes in 0.003s)
Pushing libmozsqlite3.so..
4572 KB/s (899180 bytes in 0.192s)
Pushing libnspr4.so..
5267 KB/s (233220 bytes in 0.043s)
Pushing libnss3.so..
4529 KB/s (1147000 bytes in 0.247s)
Pushing libnssckbi.so..
4533 KB/s (460416 bytes in 0.099s)
Pushing libnssutil3.so..
4720 KB/s (151884 bytes in 0.031s)
Pushing libplc4.so..
2640 KB/s (12216 bytes in 0.004s)
Pushing libplds4.so..
2261 KB/s (8280 bytes in 0.003s)
Pushing libsmime3.so..
4994 KB/s (164804 bytes in 0.032s)
Pushing libsoftokn3.so..
4893 KB/s (246536 bytes in 0.049s)
Pushing libssl3.so..
4843 KB/s (305472 bytes in 0.061s)
Pushing libxpcom.so..
2449 KB/s (11544 bytes in 0.004s)
Pushing libxul.so..
This is a big file, it could take a while.
4033 KB/s (57926992 bytes in 14.023s)
285 KB/s (362896 bytes in 1.243s)
3752 KB/s (20084 bytes in 0.005s)
4500 KB/s (18904 bytes in 0.004s)
3429 KB/s (18896 bytes in 0.005s)
3516 KB/s (19468 bytes in 0.005s)
3229 KB/s (20212 bytes in 0.006s)
3117 KB/s (13492 bytes in 0.004s)
3169 KB/s (62811768 bytes in 19.351s)
remotecppunittests INFO | Running test mediaconduit_unittests
/system/bin/sh: cd: /data/local/tests/cppunittests/tmp - No such file or directory
/system/bin/sh: /data/local/tests/cppunittests/b/mediaconduit_unittests: cannot execute - Permission denied
remotecppunittests TEST-UNEXPECTED-FAIL | mediaconduit_unittests | test failed with return code 126
grep: check-remote.log: No such file or directory
/bin/sh: @errors=: command not found

Comment 14

[Geoff Brown [:gbrown]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> I'm not sure that I'm super-excited about making the target "check-remote",
> since "make check" is a complicated mishmash of things

That's a good point. I renamed the target to cppunittests-remote -- and I'm still open to other suggestions!

Comment 15

[Geoff Brown [:gbrown]]

(In reply to Dan Mosedale (:dmose) from comment #13)
> * the biggest one is that the .so files are being pushed with permissions
> 001, which causes the executable to not run.  This bug predated your
> changes, and may be somehow specific to my environment.

That should be fixed - now I explicitly chmod 777 the files after the push.

> * sometimes the script also blows up trying to push libfreebl3.so, the first
> library.  Re-running the script generally fixes the problem.  This bug also
> predates your changes.

I think that is fixed now.

> * Finally, there's an odd error message when trying to cd to /tmp, I'm not
> sure if it's actually causing any sort of problem.  Here's an example log:

I see that occasionally...I am still trying to find the cause.

Comment 16

[Geoff Brown [:gbrown]]

Attachment: remote cppunittests -- combines previous work with latest changes

This combines the previous wip patches with contributions from dmose, ted, and myself.

This adds a remotecppunittests.py script and a cppunittests-remote make target.

Sample make target usage:

cd <objdir-droid>
DM_TRANS=adb
TEST_PATH="TestFile TestTimers"
make cppunittests-remote

We cannot use SUT yet; I will handle that in a follow-up patch.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 691392 [details] [diff] [review]
remote cppunittests -- combines previous work with latest changes

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

this looks pretty good.

Comment 18

[Geoff Brown [:gbrown]]

I have started describing this work at https://wiki.mozilla.org/Mobile/Fennec/Android#cppunittests

Comment 19

[Geoff Brown [:gbrown]]

:ted - review ping?

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 691392 [details] [diff] [review]
remote cppunittests -- combines previous work with latest changes

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

Thanks for taking this over! I have a bunch of comments, lots of which are just style nits, but a few other things I think we ought to fix before landing this.

::: testing/remotecppunittests.py
@@ +17,5 @@
> +    def __init__(self, devmgr, options, progs):
> +        cppunittests.CPPUnitTests.__init__(self)
> +        self.options = options
> +        self.device = devmgr
> +        self.remoteTestRoot = self.device.getDeviceRoot() + "/cppunittests"

Lowercase names with underscores, please (I know we're inconsistent elsewhere, but let's try to be consistent in new code).

@@ +27,5 @@
> +
> +    def remoteJoin(self, path1, path2):
> +        joined = os.path.join(path1, path2)
> +        joined = joined.replace('\\', '/')
> +        return joined

You could just import posixpath and use posixpath.join here.

@@ +42,5 @@
> +        self.pushLibs()
> +        self.pushProgs(progs)
> +        self.device.chmodDir(self.remoteBinDir)
> +
> +    def pushLibs(self):

This feels like it ought to get factored out into mozbase. There are probably some other bits of this file that could stand to be there too.

@@ +49,5 @@
> +            if not os.path.exists(localLib):
> +                localLib = os.path.join(self.options.objdir, "fennec/lib")
> +                if not os.path.exists(localLib):
> +                    print >>sys.stderr, "Error: could not find libs in objdir"
> +                    sys.exit(1)

I don't like this hardcoded "dist/fennec" bit. Your makefile target specifies --localLib, so you should just require that to be set.

@@ +54,5 @@
> +        else:
> +            localLib = self.options.localLib;
> +
> +        for file in os.listdir(localLib):
> +            if file.endswith(".so"):

I feel like we're going to wind up shipping a Windows Phone version some day and we'll have to come back and fix this, but it's not critical to fix now.

@@ +57,5 @@
> +        for file in os.listdir(localLib):
> +            if file.endswith(".so"):
> +                print >> sys.stderr, "Pushing %s.." % file
> +                if 'libxul' in file:
> +                    print >> sys.stderr, "This is a big file, it could take a while."

This is so hacky. :)

@@ +72,5 @@
> +                        self.device.pushFile(os.path.join(root, file), remoteFile)
> +
> +    def pushProgs(self, progs):
> +        for localFile in progs:
> +            remoteFile = self.remoteJoin(self.remoteBinDir, os.path.basename(localFile))

This winds up being *slightly* different from the desktop test case, because there we run the test programs from where they were built, and not dist/bin, and here you're putting the tests in the same directory as the libs. It might be worth putting them in a separate directory just to keep things similar to desktop.

@@ +76,5 @@
> +            remoteFile = self.remoteJoin(self.remoteBinDir, os.path.basename(localFile))
> +            self.device.pushFile(localFile, remoteFile)
> +
> +    def buildEnvironment(self):
> +        env = self.buildCoreEnvironment( {} )

I would just make buildCoreEnvironment's env arg optional, defaulting to {}.

@@ +95,5 @@
> +        """
> +        basename = os.path.basename(prog)
> +        remoteBin = self.remoteJoin(self.remoteBinDir, basename)
> +        log.info("Running test %s", basename)
> +        timeout = 300

We should make this a constant at module scope in runcppunittests so we don't have it defined in two places.

@@ +97,5 @@
> +        remoteBin = self.remoteJoin(self.remoteBinDir, basename)
> +        log.info("Running test %s", basename)
> +        timeout = 300
> +        buf = StringIO.StringIO()
> +        returncode = self.device.shell([remoteBin], buf, env=env, cwd=self.remoteTmpDir, timeout=timeout)

Does device.shell distinguish between normal exit failure codes and failure due to timeout?

@@ +98,5 @@
> +        log.info("Running test %s", basename)
> +        timeout = 300
> +        buf = StringIO.StringIO()
> +        returncode = self.device.shell([remoteBin], buf, env=env, cwd=self.remoteTmpDir, timeout=timeout)
> +        output = str(buf.getvalue()[0:-1]).rstrip()

Can you explain why you have to do the munging of the output here?

@@ +99,5 @@
> +        timeout = 300
> +        buf = StringIO.StringIO()
> +        returncode = self.device.shell([remoteBin], buf, env=env, cwd=self.remoteTmpDir, timeout=timeout)
> +        output = str(buf.getvalue()[0:-1]).rstrip()
> +        buf.close()

Does this really matter?

@@ +101,5 @@
> +        returncode = self.device.shell([remoteBin], buf, env=env, cwd=self.remoteTmpDir, timeout=timeout)
> +        output = str(buf.getvalue()[0:-1]).rstrip()
> +        buf.close()
> +        print >> sys.stdout, output
> +        dumpDir = tempfile.mkdtemp()

Do you remove dumpDir anywhere? If not you're accumulating directories under /tmp... You can use TemporaryDirectory from runcppunittests instead.

@@ +141,5 @@
> +
> +        self.add_option("--localLib", action="store",
> +                        type = "string", dest = "localLib",
> +                        help = "location of libraries to push -- preferably stripped")
> +        defaults["localLib"] = None 

nit: trailing whitespace

@@ +146,5 @@
> +
> +        self.set_defaults(**defaults)
> +
> +if __name__ == '__main__':
> +    parser = RemoteCPPUnittestOptions()

As with the other file, we should probably put this all into a main() method.

@@ +168,5 @@
> +            print "Error: you must provide a device IP to connect to via the --deviceIP option"
> +            sys.exit(1)
> +
> +    options.xre_path = os.path.abspath(options.xre_path)
> +    progs = [os.path.abspath(os.path.join(options.xre_path, p)) for p in args]

The non-remote test harness runs tests directly from the directory they're built in, and not dist/bin. I'd like to do the same here, so that we can stop putting them in dist/bin.

::: testing/runcppunittests.py
@@ +55,5 @@
> +                log.testFail("%s | test failed with return code %d",
> +                             basename, proc.proc.returncode)
> +            return result
> +
> +    def buildCoreEnvironment(self, env):

The other methods in this class use underscores not camelCase. Please fix these to match. (Underscores is actually PEP-8 style, we've been historically bad about that: http://www.python.org/dev/peps/pep-0008/#method-names-and-instance-variables )

@@ +70,5 @@
> +        return env
> +
> +    def buildEnvironment(self):
> +        """
> +        Create and returns a dictionary of self.env to include all the appropriate env variables and values.

This comment is misleading, this method doesn't touch self.env.

@@ +79,1 @@
>              return False

This needs to do something more than return False, because nothing checks the return value of buildEnvironment. You could make it raise, and try/catch that up in main() or something.

@@ +143,1 @@
>      sys.exit(0 if result else 1)

I wonder if we shouldn't move this all into a main() method and just sys.exit(main()) in this block.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Unhappily using this patch (with the dependent patches also applied), on my unrooted device, the chmodDir blows up for lack of permissions:

Unable to chmod /mnt/sdcard/tests/cppunittests/b/libfreebl3.so: Operation not permitted
chmod /mnt/sdcard/tests/cppunittests/b/libfreebl3.so
Unable to chmod /mnt/sdcard/tests/cppunittests/b/libmozalloc.so: Operation not permitted
chmod /mnt/sdcard/tests/cppunittests/b/libmozalloc.so

etc.

Interestingly, the permissions on the device actually look ok (.so files and executables are 0775), but, surprisingly, attempting to execute the tests blows up anyway:

remotecppunittests INFO | Running test signaling_unittests
/system/bin/sh: /mnt/sdcard/tests/cppunittests/b/signaling_unittests: cannot execute - Permission denied
remotecppunittests TEST-UNEXPECTED-FAIL | signaling_unittests | test failed with return code 126
remotecppunittests INFO | Running test mediapipeline_unittest
/system/bin/sh: /mnt/sdcard/tests/cppunittests/b/mediapipeline_unittest: cannot execute - Permission denied
remotecppunittests TEST-UNEXPECTED-FAIL | mediapipeline_unittest | test failed with return code 126
remotecppunittests INFO | Running test mediaconduit_unittests
/system/bin/sh: /mnt/sdcard/tests/cppunittests/b/mediaconduit_unittests: cannot execute - Permission denied
remotecppunittests TEST-UNEXPECTED-FAIL | mediaconduit_unittests | test failed with return code 126

Comment 22

[(not currently active) Ted Mielczarek]

I don't know if you recall, but we hit this while developing the tests, and found that mozdevice sneakily will put things in /mnt/sdcard/tests if /data/local/tests doesn't exist, but the former doesn't actually work for binaries. You should be able to just mkdir the latter and have it work. If we didn't file that we should, because it's weird magical behavior.

Comment 23

[Geoff Brown [:gbrown]]

Bugs 810347 and 824274 aim to address the test root issue, introducing a test option for setting the test root and using an appropriate default for each test. A second, not-yet-landed patch on 810347 will remove the magical device manager behavior based on the existence of /data/local/tests.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Ted, thanks for the refresh, I'd totally forgotten that.  Geoff, glad to hear this stuff is all in motion for a clean resolution!

Comment 25

[Geoff Brown [:gbrown]]

Attachment: remote cppunittests -- updated for review comments

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #20)
> Comment on attachment 691392 [details] [diff] [review]

Thanks much for your detailed review! I believe this patch addresses all of your concerns, except as noted below.

> remote cppunittests -- combines previous work with latest changes
> ::: testing/remotecppunittests.py

> @@ +42,5 @@
> > +        self.pushLibs()
> > +        self.pushProgs(progs)
> > +        self.device.chmodDir(self.remoteBinDir)
> > +
> > +    def pushLibs(self):
> 
> This feels like it ought to get factored out into mozbase. There are
> probably some other bits of this file that could stand to be there too.

I did not change this. Remote xpcshell tests are the only other tests I can think of that might be able to re-use push_libs -- I'm not sure that's worth factoring out. Maybe address as a follow-up bug?

> @@ +54,5 @@
> > +        else:
> > +            localLib = self.options.localLib;
> > +
> > +        for file in os.listdir(localLib):
> > +            if file.endswith(".so"):
> 
> I feel like we're going to wind up shipping a Windows Phone version some day
> and we'll have to come back and fix this, but it's not critical to fix now.

I didn't change this. There are other files in that directory that we don't need to push. The real dependencies are captured in makefiles, I suppose -- not easy to extract.

> @@ +57,5 @@
> > +        for file in os.listdir(localLib):
> > +            if file.endswith(".so"):
> > +                print >> sys.stderr, "Pushing %s.." % file
> > +                if 'libxul' in file:
> > +                    print >> sys.stderr, "This is a big file, it could take a while."
> 
> This is so hacky. :)

Removed. I was trying to keep this consistent with the remote xpcshell code, but so much has changed (and improved!) now, there seems little reason to copy hacks.

> @@ +72,5 @@
> > +                        self.device.pushFile(os.path.join(root, file), remoteFile)
> > +
> > +    def pushProgs(self, progs):
> > +        for localFile in progs:
> > +            remoteFile = self.remoteJoin(self.remoteBinDir, os.path.basename(localFile))
> 
> This winds up being *slightly* different from the desktop test case, because
> there we run the test programs from where they were built, and not dist/bin,
> and here you're putting the tests in the same directory as the libs. It
> might be worth putting them in a separate directory just to keep things
> similar to desktop.

I did not change this. I sometimes use something like "make cppunittests-remote" to set up a test, but then follow-up manually: adb shell, cd to the test directory, and run the test directly while I debug. Having everything in one place is that much simpler for manual execution, cleanup, etc.

> @@ +97,5 @@
> > +        remoteBin = self.remoteJoin(self.remoteBinDir, basename)
> > +        log.info("Running test %s", basename)
> > +        timeout = 300
> > +        buf = StringIO.StringIO()
> > +        returncode = self.device.shell([remoteBin], buf, env=env, cwd=self.remoteTmpDir, timeout=timeout)
> 
> Does device.shell distinguish between normal exit failure codes and failure
> due to timeout?

device.shell will raise a DMError on timeout.

> @@ +98,5 @@
> > +        log.info("Running test %s", basename)
> > +        timeout = 300
> > +        buf = StringIO.StringIO()
> > +        returncode = self.device.shell([remoteBin], buf, env=env, cwd=self.remoteTmpDir, timeout=timeout)
> > +        output = str(buf.getvalue()[0:-1]).rstrip()
> 
> Can you explain why you have to do the munging of the output here?

It wasn't necessary - removed.
 
> @@ +99,5 @@
> > +        timeout = 300
> > +        buf = StringIO.StringIO()
> > +        returncode = self.device.shell([remoteBin], buf, env=env, cwd=self.remoteTmpDir, timeout=timeout)
> > +        output = str(buf.getvalue()[0:-1]).rstrip()
> > +        buf.close()
> 
> Does this really matter?

It wasn't necessary - removed.

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 697453 [details] [diff] [review]
remote cppunittests -- updated for review comments

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

Looks good, thanks for making those changes!

::: testing/remotecppunittests.py
@@ +78,5 @@
> +        basename = os.path.basename(prog)
> +        remote_bin = posixpath.join(self.remote_bin_dir, basename)
> +        log.info("Running test %s", basename)
> +        buf = StringIO.StringIO()
> +        returncode = self.device.shell([remote_bin], buf, env=env, cwd=self.remote_tmp_dir, 

nit: trailing space

::: testing/testsuite-targets.mk
@@ +332,5 @@
> +	  --xre-path=$(DEPTH)/dist/bin \
> +	  --localLib=$(DEPTH)/dist/fennec \
> +	  --dm_trans=$(DM_TRANS) \
> +	  --deviceIP=${TEST_DEVICE} \
> +	  $(TEST_PATH) $(EXTRA_TEST_ARGS) | tee ./$@.log

I think I mentioned this last time, but you can remove the tee into the log bit here, it's not being used.

Comment 27

[Geoff Brown [:gbrown]]

With final review comments addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d834b07541e0

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d834b07541e0