Closed Bug 1066735 Opened 10 years ago Closed 10 years ago

Remove android and b2g specific xpcshell root manifests

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set
normal

Tracking

(firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(2 files, 3 obsolete files)

E.g, search for 'does not exist':
https://tbpl.mozilla.org/php/getParsedLog.php?id=47976612&tree=Mozilla-Central&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=47973609&full=1&branch=mozilla-central

This isn't being flagged as an error because the xpcshell test harness uses strict=False when parsing the manifests [1]. We should fix these missing manifest errors and switch to strict=True.

[1] http://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#792
I'm not really sure how the xpcshell manifest generation works, but I'm guessing this issue has something to do with the fact that android and b2g have different root manifests, e.g:

http://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/xpcshell_android.ini

Ideally we would get rid of those two root manifests and just skip android/b2g in the default section of the manifests they aren't supposed to run with.
xpcshell manifests predate the "specify manifests in moz.build" time, so yeah, we should be auto-generating the master manifests nowadays. We should fix whatever's broken and skip whatever we need to get things working with that and then ditch those other manifests.
I'll tackle this as it's kind of pseudo-blocking the manifest reporting stuff I'm working on.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Attached patch remove_android_b2g_manifests (obsolete) — Splinter Review
This patch should skip all the manifests not listed in xpcshell_android.ini/xpcshell_b2g.ini. But it requires a mozharness change to get the automation to stop using them, so I'll test it out on ash.
Depends on: 1067534
Blocks: 996183
Unbitrotted patch.

Also as of bug 1067534, this no longer requires a mozharness change, here's a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d7f3dae68ac1
Attachment #8488832 - Attachment is obsolete: true
This should get fixed ASAP. It's causing us to silently lose coverage anytime a manifest is renamed/moved. E.g the indexed db manifest got renamed to xpcshell-shared.ini and we haven't been running those tests on b2g or android since.

This bug will aim to run the exact same set of tests that are currently running. I'll file followups to investigate turning the accidentally disabled tests back on later.
Summary: Several android/b2g xpcshell tests not running because their manifests aren't in the test package → Remove android and b2g specific xpcshell root manifests
Sigh, this is almost there, but the changes re-organized the chunks a bit which apparently caused a perma-fail in services/fxaccounts/tests/xpcshell/test_client.js:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e233c266863a

Also 4 b2g tests seem to have been disabled as a result of my patch.. I'll need to do some log parsing to figure out which ones.
Bug 1073639 covers the fxaccounts slowness, fwiw.
Blocks: 1073595
I found the b2g manifest that was accidentally skip-if'ed, so now both b2g and android are running the exact same set of tests as before. I think the test_client.js timeout is because I removed the 'requesttimeoutfactor = 2' that was set globally in the b2g manifest here:
http://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/xpcshell_b2g.ini#9

This patch adds it back in specifically for that test. Try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b41ec898a444

I'll do a bunch of retriggers to see if there are other tests that intermittently timeout and add the timeoutfactor there too. In the meantime, let's get this reviewed!
Attachment #8498238 - Attachment is obsolete: true
Attachment #8501077 - Flags: review?(cmanchester)
Blocks: 1079278
services/fxaccounts/tests/xpcshell/test_client.js is coming back with a so far consistent timeout.
I don't really know how device manager works, but it looks like a mozprocess timeout, not an xpcshell timeout.
Yeah, you're right. Requesttimeoutfactor is definitely working because without it I see:
test_client.js | test passed (time: 311407.929ms)

and with it I see:
test_client.js | test passed (time: 611198.152ms)

So something is hanging, it's not simply being slow. I have no idea what my patch could have changed to cause this. The only thing I can think of is if it depended on some specific ordering. I may need to disable it to carry on with this patch as I really don't think we should block this on a single test failure.
(In reply to Chris Manchester [:chmanchester] from comment #11)
> I don't really know how device manager works, but it looks like a mozprocess
> timeout, not an xpcshell timeout.

Oh, and ps that is an xpcshell timeout. On b2g the per test timeout is used in mozdevice (which in turn uses mozprocess):
http://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/remotexpcshelltests.py#135
(In reply to Andrew Halberstadt [:ahal] from comment #13)
> (In reply to Chris Manchester [:chmanchester] from comment #11)
> > I don't really know how device manager works, but it looks like a mozprocess
> > timeout, not an xpcshell timeout.
> 
> Oh, and ps that is an xpcshell timeout. On b2g the per test timeout is used
> in mozdevice (which in turn uses mozprocess):
> http://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/
> remotexpcshelltests.py#135

Gotcha, thanks. Maybe we should do a push with verbose output on and ni? the test authors if anything looks fishy in the timeouts?
Depends on: 1079405
Depends on: 1073639
No longer depends on: 1079405
This incarnation of the patch skips test_client.js on debug emulators. It also adds requesttimeoutfactor = 2 at the manifest level to the toolkit/components/osfile tests which RyanVM said were especially prone to intermittent timeouts.

Here's a new try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1db5e6a27d72
Attachment #8501077 - Attachment is obsolete: true
Attachment #8501077 - Flags: review?(cmanchester)
Attachment #8501237 - Flags: review?(cmanchester)
(In reply to Andrew Halberstadt [:ahal] from comment #15)
> This incarnation of the patch skips test_client.js on debug emulators. It
> also adds requesttimeoutfactor = 2 at the manifest level to the
> toolkit/components/osfile tests which RyanVM said were especially prone to
> intermittent timeouts.

CC Yoric, FYI. We should get a bug on file for figuring out what if anything can be done about the runtime of the OS.File tests on B2G debug. Especially since it seems to be intermittent.
(In reply to Chris Manchester [:chmanchester] from comment #14)
> Gotcha, thanks. Maybe we should do a push with verbose output on and ni? the
> test authors if anything looks fishy in the timeouts?

Yeah, there's already a bug on file and I added instructions for doing that there:
https://bugzilla.mozilla.org/show_bug.cgi?id=1073639#c7
Comment on attachment 8501237 [details] [diff] [review]
Remove android/b2g specific xpcshell manifests

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

Can we make the strict=True change in runxpcshelltests.py along with this patch? Either way this is a huge improvement to our manifest story for xpcshell.

I didn't take the step of confirming for myself we're preserving the set of running tests, but I could for a sanity check this afternoon if you prefer.

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +24,5 @@
>  [test_hmac.js]
>  
>  [test_sts_preloadlist_perwindowpb.js]
>  # Bug 978426: Test fails consistently only on B2G ARM
> +skip-if = buildapp == "b2g" && processor == "arm"

I don't see why this line shows up in the diff.

::: testing/xpcshell/example/unit/xpcshell.ini
@@ +4,5 @@
>  
>  [DEFAULT]
>  head =
>  tail =
> +skip-if = toolkit == 'gonk'

I know it was probably like this before, but I find skipping the harness checks wholesale fairly disturbing.

::: toolkit/components/osfile/tests/xpcshell/xpcshell.ini
@@ +1,5 @@
>  [DEFAULT]
>  head = head.js
>  tail =
> +# Intermittently times out on debug b2g emulators
> +requesttimeoutfactor = 2

It would require extending our manifest language, but timeoutfactor-if would be a good thing to do.
Attachment #8501237 - Flags: review?(cmanchester) → review+
(In reply to Chris Manchester [:chmanchester] from comment #18)
> >  [test_sts_preloadlist_perwindowpb.js]
> >  # Bug 978426: Test fails consistently only on B2G ARM
> > +skip-if = buildapp == "b2g" && processor == "arm"
> 
> I don't see why this line shows up in the diff.

Because it used to be "processor = arm" which is a syntax error. Until now this manifest was never parsed with a b2g platform, so the right side of the && was never evaluated.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> CC Yoric, FYI. We should get a bug on file for figuring out what if anything
> can be done about the runtime of the OS.File tests on B2G debug. Especially
> since it seems to be intermittent.

Filed bug 1079454.
Try run looks good. There was some minor bitrot, but nothing that should cause problems. Carry r+ forward.
Attachment #8501501 - Flags: review+
Blocks: 1079651
https://hg.mozilla.org/mozilla-central/rev/3d1a89fbcaa1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1170772
You need to log in before you can comment on or make changes to this bug.