773031 - Get xpcshell tests working on B2G emulators

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Run XPCShell tests on the B2G emulator.

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: xunit results

Comment 3

[Jonathan Griffin (:jgriffin)]

Comment on attachment 641239 [details] [diff] [review]
Run XPCShell tests on the B2G emulator.

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

This looks good.  Since we're duplicating so much code from runtestsremote.py (more than we duplicate for mochitest), I'm thinking we should just subclass XPCShellRemote instead of XPCShellTests, to save us from all the duplication.

::: testing/xpcshell/runtestsb2g.py
@@ +45,5 @@
> +            self.setup_utilities()
> +            self.setup_test_dir()
> +
> +
> +    def create_remote_dir(self, path, replace=False):

This function doesn't seem to be used anywhere.

@@ +61,5 @@
> +        self.clean()
> +
> +        # create folder structure
> +        remote_pref_dir = self.remote_join(self.remote_bin_dir, 'defaults/pref')
> +        for dir in ('/data/local/tests', self.remote_bin_dir,

We use '/data/local/tests' in a couple of places, we should probably set it as self.testDir or something so that we can change it easily later as needed.

@@ +89,5 @@
> +        for file in os.listdir(local_lib):
> +            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."

I like this!

@@ +361,5 @@
> +        kwargs['port'] = options.device_port
> +    dm = devicemanagerADB.DeviceManagerADB(**kwargs)
> +
> +    # make sure the /data/local/tests folder exists and set it as device_root
> +    dm.mkDirs('/data/local/tests')

You can now pass a deviceRoot parameter in DeviceManagerADB instantiation, which we should do here too.

@@ +385,5 @@
> +        else:
> +            ret_val = 0
> +    except:
> +        print "TEST-UNEXPECTED-FAIL | %s | Exception caught while running tests." % sys.exc_info()[1]
> +        traceback.print_exc()

or just add sys.exit(1) here, then you don't need ret_val any longer.

::: testing/xpcshell/runtestsb2g.sh
@@ +3,5 @@
> +# $B2GPATH == path to the B2G folder
> +# $EMUPATH == path to the emulator folder (containing run-emulator.sh)
> +# $ADBPATH == path to the adb binary
> +# $OBJDIR == path to the gecko objdir, like B2G/objdir-gecko
> +

We should make all these things except $B2GPATH optional, since we should be able to supply reasonable defaults or omit them altogether:

 - if you're running this from a full compiled build (instead of a downloaded build), then $B2GPATH == $EMUPATH, so no need to specify them separately
 - $OBJDIR: we can guess this if omitted, since it will always currently be $B2GPATH/objdir-gecko
 - $ADBPATH: we can omit this if not specified, and assume it is on the path

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Jonathan Griffin (:jgriffin) from comment #3)
> Comment on attachment 641239 [details] [diff] [review]
> Run XPCShell tests on the B2G emulator.
> 
> Review of attachment 641239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good.  Since we're duplicating so much code from
> runtestsremote.py (more than we duplicate for mochitest), I'm thinking we
> should just subclass XPCShellRemote instead of XPCShellTests, to save us
> from all the duplication.

Indeed, there is quite some duplication. I don't know if in the future we'll need to have more distinct things. If not, let me know and I will extend XPCShellRemote and modify accordingly.

> 
> ::: testing/xpcshell/runtestsb2g.py
> @@ +45,5 @@
> > +            self.setup_utilities()
> > +            self.setup_test_dir()
> > +
> > +
> > +    def create_remote_dir(self, path, replace=False):
> 
> This function doesn't seem to be used anywhere.

I don't use it anymore but I thought it would be helpful for future use. Will delete it.

> 
> @@ +61,5 @@
> > +        self.clean()
> > +
> > +        # create folder structure
> > +        remote_pref_dir = self.remote_join(self.remote_bin_dir, 'defaults/pref')
> > +        for dir in ('/data/local/tests', self.remote_bin_dir,
> 
> We use '/data/local/tests' in a couple of places, we should probably set it
> as self.testDir or something so that we can change it easily later as needed.

Right!

> 
> @@ +89,5 @@
> > +        for file in os.listdir(local_lib):
> > +            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."
> 
> I like this!
> 
> @@ +361,5 @@
> > +        kwargs['port'] = options.device_port
> > +    dm = devicemanagerADB.DeviceManagerADB(**kwargs)
> > +
> > +    # make sure the /data/local/tests folder exists and set it as device_root
> > +    dm.mkDirs('/data/local/tests')
> 
> You can now pass a deviceRoot parameter in DeviceManagerADB instantiation,
> which we should do here too.

I didn't have that patch applied, will fix.

> 
> @@ +385,5 @@
> > +        else:
> > +            ret_val = 0
> > +    except:
> > +        print "TEST-UNEXPECTED-FAIL | %s | Exception caught while running tests." % sys.exc_info()[1]
> > +        traceback.print_exc()
> 
> or just add sys.exit(1) here, then you don't need ret_val any longer.

Ok.

> 
> ::: testing/xpcshell/runtestsb2g.sh
> @@ +3,5 @@
> > +# $B2GPATH == path to the B2G folder
> > +# $EMUPATH == path to the emulator folder (containing run-emulator.sh)
> > +# $ADBPATH == path to the adb binary
> > +# $OBJDIR == path to the gecko objdir, like B2G/objdir-gecko
> > +
> 
> We should make all these things except $B2GPATH optional, since we should be
> able to supply reasonable defaults or omit them altogether:
> 
>  - if you're running this from a full compiled build (instead of a
> downloaded build), then $B2GPATH == $EMUPATH, so no need to specify them
> separately
>  - $OBJDIR: we can guess this if omitted, since it will always currently be
> $B2GPATH/objdir-gecko
>  - $ADBPATH: we can omit this if not specified, and assume it is on the path

So then I'll just pass "$@" to the python script and let it handle the arguments?

Comment 5

[Jonathan Griffin (:jgriffin)]

(In reply to Mihnea Dobrescu-Balaur from comment #4)
> (In reply to Jonathan Griffin (:jgriffin) from comment #3)
> > Comment on attachment 641239 [details] [diff] [review]
> > Run XPCShell tests on the B2G emulator.
> > 
> > Review of attachment 641239 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good.  Since we're duplicating so much code from
> > runtestsremote.py (more than we duplicate for mochitest), I'm thinking we
> > should just subclass XPCShellRemote instead of XPCShellTests, to save us
> > from all the duplication.
> 
> Indeed, there is quite some duplication. I don't know if in the future we'll
> need to have more distinct things. If not, let me know and I will extend
> XPCShellRemote and modify accordingly.

Yes, let's extend XPCShellRemote.

> > ::: testing/xpcshell/runtestsb2g.sh
> > @@ +3,5 @@
> > > +# $B2GPATH == path to the B2G folder
> > > +# $EMUPATH == path to the emulator folder (containing run-emulator.sh)
> > > +# $ADBPATH == path to the adb binary
> > > +# $OBJDIR == path to the gecko objdir, like B2G/objdir-gecko
> > > +
> > 
> > We should make all these things except $B2GPATH optional, since we should be
> > able to supply reasonable defaults or omit them altogether:
> > 
> >  - if you're running this from a full compiled build (instead of a
> > downloaded build), then $B2GPATH == $EMUPATH, so no need to specify them
> > separately
> >  - $OBJDIR: we can guess this if omitted, since it will always currently be
> > $B2GPATH/objdir-gecko
> >  - $ADBPATH: we can omit this if not specified, and assume it is on the path
> 
> So then I'll just pass "$@" to the python script and let it handle the
> arguments?

Sounds ok.  Let's just add some comments to this file to document what those are.

Comment 6

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: modified according to feedback

I got rid of the companion script. Documented the usage inside the py file. It is called "runtestsb2g", so it should be used to run the tests anyway.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 641630 [details] [diff] [review]
modified according to feedback

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

Looks good! r+ from me. I realize a lot of how things are, are due to the way remotexpcshell.py does it. So best to leave it consistent until we eventually use mozbase.

::: testing/xpcshell/runtestsb2g.py
@@ +84,5 @@
> +        self.device.chmodDir(self.remoteBinDir)
> +
> +
> +    def clean(self):
> +        self.device.removeDir(DEVICE_TEST_ROOT)

Not super important, but I don't think this is necessary if you are using the emulator as the changes will get discarded anyway. You can use marionette.is_emulator to check. I'd be fine with leaving it as is though.

@@ +123,5 @@
> +        return output_file
> +
> +
> +    # overridden
> +    # this returns 1 even when tests pass - this is why it's switched to 0

Is there a bug number for this? Would be good to include it here.

Comment 8

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> Comment on attachment 641630 [details] [diff] [review]
> modified according to feedback
> 
> Review of attachment 641630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! r+ from me. I realize a lot of how things are, are due to the
> way remotexpcshell.py does it. So best to leave it consistent until we
> eventually use mozbase.
> 
> ::: testing/xpcshell/runtestsb2g.py
> @@ +84,5 @@
> > +        self.device.chmodDir(self.remoteBinDir)
> > +
> > +
> > +    def clean(self):
> > +        self.device.removeDir(DEVICE_TEST_ROOT)
> 
> Not super important, but I don't think this is necessary if you are using
> the emulator as the changes will get discarded anyway. You can use
> marionette.is_emulator to check. I'd be fine with leaving it as is though.

It is necessary because [now] we have the userImg option on the emulator which keeps things as they were between runs.

> 
> @@ +123,5 @@
> > +        return output_file
> > +
> > +
> > +    # overridden
> > +    # this returns 1 even when tests pass - this is why it's switched to 0
> 
> Is there a bug number for this? Would be good to include it here.

What product should that bug be filed against?

Comment 9

[Jonathan Griffin (:jgriffin)]

Comment on attachment 641630 [details] [diff] [review]
modified according to feedback

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

Looks good, I'll land after you add the bug # for that xpcshell return code problem.

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

https://bugzilla.mozilla.org/show_bug.cgi?id=773703

Comment 11

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: fixed the weird file vs dir bug

It's the same file as before, just modified this:

< +        for dir in (DEVICE_TEST_ROOT, self.remoteScriptsDir, self.remoteBinDir,
< +                    self.remoteTmpDir, self.remoteJoin(self.remoteBinDir, 'defaults'),
< +                    remote_pref_dir, self.remoteComponentsDir):
---
> +        for dir in (DEVICE_TEST_ROOT, self.remoteBinDir,
> +                    self.remoteTmpDir, remote_pref_dir,
> +                    self.remoteScriptsDir, self.remoteComponentsDir):

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: add bug# for the return code thingie

Comment 13

[Jonathan Griffin (:jgriffin)]

Comment on attachment 642088 [details] [diff] [review]
add bug# for the return code thingie

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

Looks good!  By convention, we only use one blank line between methods of the same class, can you update accordingly?  I'll pass on to jmaher for feedback.

::: testing/xpcshell/runtestsb2g.py
@@ +9,5 @@
> +import traceback
> +import runxpcshelltests as xpcshell
> +from remotexpcshelltests import XPCShellRemote
> +from automationutils import *
> +import devicemanager, devicemanagerADB, devicemanagerSUT

We only need devicemanagerADB here.

@@ +227,5 @@
> +
> +def guess_objdir(options):
> +    return options.b2g_path + ('objdir-gecko' if options.b2g_path[-1] == '/'
> +                          else '/objdir-gecko')
> +

Wouldn't this better be just os.path.join(options.b2g_path, 'objdir-gecko')?

Comment 14

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

Comment on attachment 642088 [details] [diff] [review]
add bug# for the return code thingie

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

I would prefer full capitalization on the comments.

f- as a lot of the code is 95% the same as what is in the base class.

::: testing/xpcshell/runtestsb2g.py
@@ +20,5 @@
> +
> +
> +class B2GXPCShellRemote(XPCShellRemote):
> +
> +    # using Java var notation because of the overriding

Nit: capitalize the comment.

@@ +22,5 @@
> +class B2GXPCShellRemote(XPCShellRemote):
> +
> +    # using Java var notation because of the overriding
> +    def __init__(self, devmgr, options, args):
> +        xpcshell.XPCShellTests.__init__(self)

odd that you are subclassing from XPCShellRemote, but doing an init on the grandparent.

@@ +40,5 @@
> +        self.profileDir = self.remoteJoin(self.remoteTestRoot, 'p')
> +
> +        if options.setup:
> +            self.setup_utilities()
> +            self.setupTestDir()

this is the same as XPCShellRemote except for remoteAPK and debugger, we can fix up the base class to not worry about those options.

@@ +80,5 @@
> +                if 'libxul' in file:
> +                    print >> sys.stderr, "This is a big file, it could take a while."
> +                self.device.pushFile(os.path.join(local_lib, file), self.remoteBinDir)
> +
> +        self.device.chmodDir(self.remoteBinDir)

this is very similar to XPCShellRemote, we should fix/use the baseclass

@@ +96,5 @@
> +            '-s',
> +            '-e', 'const _HTTPD_JS_PATH = "%s";' % self.remoteJoin(self.remoteComponentsDir, 'httpd.js'),
> +            '-e', 'const _HEAD_JS_PATH = "%s";' % self.remoteJoin(self.remoteScriptsDir, 'head.js'),
> +            '-f', self.remoteScriptsDir + '/head.js'
> +        ]

This is the same as the XPCShellRemote class except for missing the greomni option and not having code for the debugger.

@@ +119,5 @@
> +        # of xpcshell.
> +        self.device.killProcess(cmd[0]);
> +        self.device.killProcess('xpcshell');
> +
> +        return output_file

this function (LaunchProcess) is a duplicate of XPCShellRemote except for nicer looking code (which should work on python 2.5) and not resetting env(), can this be fixed in the base class and used that way?

@@ +198,5 @@
> +
> +        self.add_option('--pid-file', action='store',
> +                        type='string', dest='pid_file',
> +                        help="Name of the pid file to generate")
> +        defaults['pid_file'] = ''

lets resuse the options class from remotexpcshelltests.py

Comment 15

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: modify according to feedback

Also removed some trailing whitespace, hope that's ok.

Comment 16

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

Comment on attachment 642738 [details] [diff] [review]
modify according to feedback

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

great, this is a lot less code!

::: testing/xpcshell/remotexpcshelltests.py
@@ +158,5 @@
> +            # for example, "/data/local/gdbserver" "localhost:12345"
> +            self.xpcsCmd = [
> +              self.remoteDebugger,
> +              self.remoteDebuggerArgs,
> +              self.xpcsCmd]

it seems that this is indented?  I assume we can leave this remoteDebugger block alone and ensure that b2g sets the options.remoteDebugger=None or whatever is appropriate?

Comment 17

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Joel Maher (:jmaher) from comment #16)
> Comment on attachment 642738 [details] [diff] [review]
> modify according to feedback
> 
> Review of attachment 642738 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> great, this is a lot less code!
> 
> ::: testing/xpcshell/remotexpcshelltests.py
> @@ +158,5 @@
> > +            # for example, "/data/local/gdbserver" "localhost:12345"
> > +            self.xpcsCmd = [
> > +              self.remoteDebugger,
> > +              self.remoteDebuggerArgs,
> > +              self.xpcsCmd]
> 
> it seems that this is indented?  I assume we can leave this remoteDebugger
> block alone and ensure that b2g sets the options.remoteDebugger=None or
> whatever is appropriate?

Yes that is indented. Ok, I'll make that mod.

Comment 18

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: modify according to feedback

Comment 19

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/574ec170d4ac

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/574ec170d4ac

Comment 21

[Jason Duell]

Comment on attachment 642764 [details] [diff] [review]
modify according to feedback

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

Great to have B2G xpcshell tests!

Do we have docs somewhere on how to invoke them?  

Also, does this run xpcshell tests directly on the child, or do we still need to write separate testfile_wrap.js files that invoke run_test_in_child(testfile.js) to get e10s coverage?

::: testing/xpcshell/runtestsb2g.py
@@ +36,5 @@
> +    # https://bugzilla.mozilla.org/show_bug.cgi?id=773703
> +    def getReturnCode(self, proc):
> +#        if self.shellReturnCode is not None:
> +#            return self.shellReturnCode
> +#        return -1

did we want to land this commented-out block?

Comment 22

[Jonathan Griffin (:jgriffin)]

(In reply to Jason Duell (:jduell) from comment #21)
> Comment on attachment 642764 [details] [diff] [review]
> modify according to feedback
> 
> Review of attachment 642764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great to have B2G xpcshell tests!
> 
> Do we have docs somewhere on how to invoke them?  

Not yet, except for the comments at the end of the file.  Stay tuned.

> 
> Also, does this run xpcshell tests directly on the child, or do we still
> need to write separate testfile_wrap.js files that invoke
> run_test_in_child(testfile.js) to get e10s coverage?
> 

We haven't changed the mechanics of xpcshell tests at all, so whatever the normal method of running e10s tests is should hopefully still work.

Comment 23

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Jason Duell (:jduell) from comment #21)
> 
> ::: testing/xpcshell/runtestsb2g.py
> @@ +36,5 @@
> > +    # https://bugzilla.mozilla.org/show_bug.cgi?id=773703
> > +    def getReturnCode(self, proc):
> > +#        if self.shellReturnCode is not None:
> > +#            return self.shellReturnCode
> > +#        return -1
> 
> did we want to land this commented-out block?

Because of bug #773703, it's the only way we can run the test with relevant results.