Closed Bug 916350 Opened 7 years ago Closed 7 years ago

Make it possible to run B2G reftests on B2G desktop builds

Categories

(Testing :: Reftest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: jgriffin, Assigned: ahal)

References

Details

Attachments

(3 files, 4 obsolete files)

It's been decided that we want to run reftests on B2G desktop builds in TBPL, as well as emulators.  We'll need to update the test harness to make this possible.
Blocks: 916122
Blocks: 916356
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Depends on: 947974
Comment on attachment 8344800 [details] [diff] [review]
Patch 1.0 (m-c) - add ability to run reftest on b2g desktop

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

::: layout/tools/reftest/b2g_desktop.py
@@ +47,5 @@
> +        if not reftestlist.startswith('file://'):
> +            reftestlist = 'file://%s' % reftestlist
> +
> +        self.profile = self.create_profile(options, reftestlist,
> +                                      profile_to_clone=options.profile)

nit: weird indenting

@@ +53,5 @@
> +        kp_kwargs = { 'processOutputLine': [self._on_output],
> +                      'onTimeout': [self._on_timeout],
> +                      'kill_on_timeout': False }
> +
> +        if not options.debugger and options.autorun:

there's no 'autorun' for reftests, is there?

@@ +65,5 @@
> +        log.info("%s | Running tests: start.", os.path.basename(__file__))
> +        cmd, args = self.build_command_line(options.app,
> +                            ignore_window_size=options.ignoreWindowSize)
> +        self.runner = FirefoxRunner(profile=self.profile,
> +                               binary=cmd,

nit: weird indenting

@@ +142,5 @@
> +        log.testFail(msg % (self.last_test, self.timeout))
> +
> +        # kill process and get stack
> +        try:
> +            os.killpg(self.runner.process_handler.pid, signal.SIGABRT)

This definitely won't work on Windows, which isn't relevant right now but may be in the future.  Can we use mozprocess' kill implementation instead?
Attachment #8344800 - Flags: review?(jgriffin) → review+
Attachment #8344797 - Flags: review?(jgriffin) → review+
https://github.com/mozilla/mozbase/commit/4b86b177751d9739cbc8811c1d8e4278d82067ec

I'll need to wait for mozbase to get synced to m-c before landing the m-c bits.
(In reply to Jonathan Griffin (:jgriffin) from comment #3)
> @@ +142,5 @@
> > +        log.testFail(msg % (self.last_test, self.timeout))
> > +
> > +        # kill process and get stack
> > +        try:
> > +            os.killpg(self.runner.process_handler.pid, signal.SIGABRT)
> 
> This definitely won't work on Windows, which isn't relevant right now but
> may be in the future.  Can we use mozprocess' kill implementation instead?

Actually I forgot to update this patch with a change I had been planning. Instead I landed https://github.com/mozilla/mozbase/commit/50bcb04fe88b053b16fa97833c252be439bed15e and replaced the try block with mozrunner.stop(sig=signal.SIGABRT).

This still won't work with windows, but I think the killAndGetStack functionality should live in mozrunner. We can add windows support for killAndGetStack to mozrunner later.
(to clarify my last comment)

mozprocess' kill implementation won't generate a stack on windows, but the kill itself _will_. We should add the stack capability in.
kill on Windows will not generate a stack. This is why we have this sort of thing:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#666
(mozprocess' kill won't either, to clarify, it will kill the process but no minidump will be generated.)
Yeah that's what I meant. We should use crashinject.exe somewhere in mozbase or somewhere that can be shared between harnesses.
Quick update, this patch is failing on Cedar: https://tbpl.mozilla.org/php/getParsedLog.php?id=32438814&tree=Cedar

It works locally even when using Cedar. This output precedes the timeout: 
System JS : ERROR chrome://reftest/content/reftest.jsm:393 - TypeError: CC['@mozilla.org/server/jshttp;1'] is undefined
System JS : ERROR chrome://reftest/content/reftest-content.js:110 - TypeError: initInfo is null
Earlier in the log I also see

System JS : ERROR chrome://satchel/content/formSubmitListener.js:15 - ReferenceError: XPCOMUtils is not defined

which seems equally strange.
That may or may not be causing the jshttpd type error, but the odd thing to me is that that line should never even be executed. On B2G desktop we pass in reftest.remote=False which means we shouldn't even run that line: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#394

I've verified that the prefs are set up correctly locally (and indeed it works) but maybe something is changing that pref when run in production?? I'll start doing some debug commits to cedar to try and figure out what's up.
Ignore that last comment. Also, I noticed the 32bit log has the same failure but not the "formSubmitListener" error in it: https://tbpl.mozilla.org/php/getParsedLog.php?id=32438772&tree=Cedar&full=1#error0

So I think comment 12 is incidental.
According to [1] and [2], not all Gecko applications necessarily implement autoregistration of XPCOM components. Firefox watches the profile/components directory for new components and then autoregisters them. I believe what is happening here is that b2g desktop does not implement autoregistration while the Makefile for the reftest extension assumes there is autoregistration, and therefore CC['@mozilla.org/server/jshttp;1'] is undefined.

Assuming I'm correct, then the solution is either:
A) make b2g_desktop autoregister components
B) make reftest use httpd.js as an inline script
C) register the component manually with regxpcom or nsIComponentRegistrar as described in [2]

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/README?force=1#22
[2] https://developer.mozilla.org/en-US/docs/Creating_XPCOM_Components/Component_Internals#Registration_Methods_in_XPCOM
Note this doesn't explain to me why this is working locally for me.. so it's just a theory at this point.
By "locally" do you mean "from an objdir of a local B2G desktop build" or "using a downloaded B2G desktop build + test package"?
Locally built, but yes good point, downloading the build would be a good thing to try next :p
I ask because it's certainly possible that a difference between packaged builds and objdirs is responsible.
Yeah, you're right, I can reproduce with a downloaded build. I believe the downloaded build is using this mozconfig:
http://mxr.mozilla.org/mozilla-central/source/b2g/config/mozconfigs/linux64_gecko/nightly

My mozconfig is:

. "$topsrcdir/b2g/config/mozconfigs/common"

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/objdirs/b2g_desktop
mk_add_options MOZ_MAKE_FLAGS="-j9 -s"

ac_add_options --enable-application=b2g
ac_add_options --disable-libjpeg-turbo
 
# This option is required if you want to be able to run Gaia's tests
ac_add_options --enable-tests

# turn on mozTelephony/mozSms interfaces
# Only turn this line on if you actually have a dev phone
# you want to forward to. If you get crashes at startup,
# make sure this line is commented.
#ac_add_options --enable-b2g-ril

ENABLE_MARIONETTE=1
Okay, then your analysis from comment 15 is probably spot-on. Running from an objdir, httpd.js is installed into dist/bin/components (along with httpd.manifest). When packaged, that's no longer there and we rely on packaging it into the reftest extension:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/Makefile.in#38
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/jar.mn

I'm fuzzy on how reftest works on B2G, but I suspect that we don't have real extension support there and therefore not everything works properly.
With this patch I was able to get the downloaded build working. I'll test it out on cedar before asking review though.
Comment on attachment 8356732 [details] [diff] [review]
Patch 1.1 (m-c) - manually register reftest extension's components if needed

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

This got them unstuck on Cedar.. still other problems, but at least they are running now.
Attachment #8356732 - Flags: review?(ted)
Comment on attachment 8356732 [details] [diff] [review]
Patch 1.1 (m-c) - manually register reftest extension's components if needed

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

::: layout/tools/reftest/reftest.js
@@ +394,5 @@
>      if (gRemote) {
>          gServer = null;
>      } else {
> +        // not all gecko applications autoregister xpcom components
> +        if (CC["@mozilla.org/server/jshttp;1"] === undefined) {

You could also write !("@mozilla.org/server/jshttp;1" in CC), but that's a little awkward. (Wish JS had Python's "not in" operator!)
Attachment #8356732 - Flags: review?(ted) → review+
There was a fair amount of bitrot, so rebased and pushed a new try run:
https://tbpl.mozilla.org/?tree=Try&rev=2bfd09f07ab8

Carrying r+ forward.
Attachment #8344800 - Attachment is obsolete: true
Attachment #8362570 - Flags: review+
None of these patches really make sense on their own, so I qfolded the three together:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c24dc80aa00
Hmm, try -p android -u reftest doesn't run reftests on Android? Didn't notice it didn't get run, sorry.
I think this is just because I moved mozprofile above the sys.path.insert. New try run with proper syntax:
https://tbpl.mozilla.org/?tree=Try&rev=6c7dba6be09d

I noticed bug 664857 was filed to create a 'reftest' alias in the try syntax that runs all reftest chunks on Android, though it was resolved without being fixed. I re-opened it because I think this is tripping up a lot of people.
This try run is green (there were a few new references to _automation I needed to update):
https://tbpl.mozilla.org/?tree=Try&rev=87eb0c601405

Carrying r+ forward
Attachment #8362570 - Attachment is obsolete: true
Attachment #8363246 - Flags: review+
That's what I get for not waiting for all try jobs to finish :(.. this is strange since the "import mozprofile" existed before my patch, and now that it is also on the same side of the sys.path.insert(), I can't see what could cause this.
It turns out automation.py has a hack that makes importing mozbase work, so the import just needed to be a little lower:
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#29

Here's an actually green try run to prove it:
https://tbpl.mozilla.org/?tree=Try&rev=aa6e3de1d6ac

I updated the fix, folded the register patch into the main one and carried r+ forward.
Attachment #8356732 - Attachment is obsolete: true
Attachment #8363246 - Attachment is obsolete: true
Attachment #8365079 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1626ce65aaf1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
There's a missing hunk that's on Cedar but not m-c which is causing the jobs to fail. I must have messed up rebasing my patch on top of the parallel reftest change. Bustage fix coming shortly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ryan has graciously offered to land this directly to get it spread across branches more quickly. Thanks Ryan!
https://hg.mozilla.org/mozilla-central/rev/07021aa58289
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.