Closed Bug 971132 Opened 6 years ago Closed 6 years ago

B2G mochitests should use the new manifest format

Categories

(Testing :: Mochitest, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: ahal, Assigned: vaibhav1994)

References

Details

Attachments

(4 files, 10 obsolete files)

14.85 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
258.80 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
45.17 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
19.53 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
We should stop using b2g.json and use the mochitest.ini format instead.
work is well underway in bug 970925 to do this for android.  :vhabhav1994 is working on a script to automate this as much as possible- when that is done it should be mostly reusable for this bug.
Attached patch b2g.patch (obsolete) — Splinter Review
Attachment #8378501 - Flags: review?(jmaher)
Comment on attachment 8378501 [details] [diff] [review]
b2g.patch

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

I know this was done by script, but saying (buildapp == 'b2g' || toolkit == 'gonk') is redundant since the latter implies the former. Maybe being explicit isn't such a terrible idea though.

::: testing/mochitest/b2g.json
@@ +46,2 @@
>      "content/media/test/test_playback_rate_playpause.html": "",
> +    "content/media/test/test_can_play_type_ogg.html": "",

So are the tests remaining here ones that don't have manifests yet? If so we'll need to wait until they do before we go ahead and land this since I don't think it is possible to use both styles of manifest at the same time.
Assignee: ahalberstadt → vaibhavmagarwal
Comment on attachment 8378501 [details] [diff] [review]
b2g.patch

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

this is a solid patch.  Testing on try- please take ahal's suggestion in comment 4 to change
(buildapp == 'b2g' || toolkit == 'gonk')

to:
buildapp == 'b2g'

likewise additional comments in irc about manifest logic.  Only a r- as we can optimize the manifest logic and will need a new patch.
Attachment #8378501 - Flags: review?(jmaher) → review-
I found a problem when a test is disabled in both b2g-desktop.json and b2g-debug.json. For example, look at content/base/test/test_XHR_system.html. It was previously disabled in both b2g-debug.json and b2g-desktop.json. But in the new format it is translated to 'skip-if = (buildapp == 'b2g' && debug)' which will only skip it on debug platforms. I think in this case it needs to be 'skip-if = (buildapp == 'b2g' && (toolkit != 'gonk' || debug))'
As requested, here's the mapping of manifest to skip-if syntax.

Test in manifests    | Skip syntax
----------------------------------
b2g, debug, desktop  | buildapp == 'b2g'
b2g, debug           | toolkit == 'gonk'
b2g, desktop         | (buildapp == 'b2g' && !debug)
debug, desktop       | (buildapp == 'b2g' && (toolkit != 'gonk' || debug))
b2g                  | (toolkit == 'gonk' && !debug)
debug                | (toolkit == 'gonk' && debug)
desktop              | (buildapp == 'b2g' && toolkit != 'gonk')
Attached patch b2g.patch (obsolete) — Splinter Review
Updated changes.
Attachment #8378501 - Attachment is obsolete: true
Attachment #8378534 - Flags: review?(jmaher)
Attached patch b2gupdated.patch (obsolete) — Splinter Review
Some minor changes done
Attachment #8378534 - Attachment is obsolete: true
Attachment #8378534 - Flags: review?(jmaher)
Attachment #8378557 - Flags: review?(jmaher)
Comment on attachment 8378557 [details] [diff] [review]
b2gupdated.patch

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

I didn't look super closely, but from what I can tell it looks correct, thank you so much! I'd still like to see a try run with this patch to compare the number of tests being run. For the record syntax is 'try -b do -p emulator,linux64_gecko -u mochitests -t none'.
Attachment #8378557 - Flags: feedback+
Attached patch b2g.patch (obsolete) — Splinter Review
Made some changes due to which the patch was not running properly on try server.
Attachment #8378557 - Attachment is obsolete: true
Attachment #8378557 - Flags: review?(jmaher)
Attachment #8379742 - Flags: review?(jmaher)
with this latest patch and try server (https://tbpl.mozilla.org/?tree=Try&rev=973477db972e) I see this for emulator opt:
/tests/dom/browser-element/mochitest/test_browserElement_NoPref.html
/tests/dom/browser-element/mochitest/test_browserElement_NoWhitelist.html
/tests/content/canvas/test/test_canvas.html

these are not in current b2g*.json files, nor already annotated in mochitest.ini files.  Are these new tests?  Is this something related to the patch?  Should we clean these up in the b2g.json file?
Flags: needinfo?(ahalberstadt)
I'm not really sure, I was looking into it as well. It looks like they didn't fail on the previous try push, so unless we got extremely unlucky and they landed between then, it looks like this patch is somehow causing the failures.

One thing I noticed is that chunks 1-4 have a different number of tests running as compared to the previous try push, but chunks 5-9 have the same number. Is that expected?

Another question I had is, where in that try run is the harness reading the mochitest.ini files? As far as I can tell, it'll only do this if we pass in --manifest, which we aren't. Joel, could you help me figure out how this is working?
Flags: needinfo?(ahalberstadt) → needinfo?(jmaher)
the previous try push was missing some items in the exclude list.  Looking at this vs inbound over half of the items are exactly the same for emulator opt and the others are really close.  

I tried to figure out where in regular mochitests we define mochitest.ini and it was not obvious.  Nor was it for android or b2g.  When I am working and not just checking email, I will figure it out.  I do know that we have removed hundreds of marked failing files from b2g.json (and all *.json files) and the runs are still green.

We just need to figure out why these 3 tests are failing on emulator opt.

For test_canvas.html (chunk 2) the logs on inbound show the same number of checks, but in the try push we have additional checks which fail at the end!?!

For the NoAttr and NoManifest test cases, we run those on inbound as well!  They produce (as do the other test cases in the same directory) no test-pass or other test-* messages, just start/end.  But for some reason with this patch we are seeing failures.

So this patch is turning on/off the appropriate tests.  Could it be turning on a test that wasn't on before?  Are we somehow doing something different with the test conditions?
Flags: needinfo?(jmaher)
The test_canvas.html failure looks like the one in bug 966591 comment 93. Did someone pus hto try from inbound and expect meaningful results?
Ms2ger, thanks for finding the canvas issue, now to figure out the remaining issues here:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35097416&tree=Try

these two tests:
http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/test_browserElement_NoPref.html
http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/test_browserElement_NoWhitelist.html

both set pref/permission and then expect us not to fire an event.  for some reason we are firing the event.  Could we have another issue with using inbound?  Is it possible that we have turned on a new test which is passing, but not cleaning up properly?
oh, I think I found the root cause of this:
https://bugzilla.mozilla.org/show_bug.cgi?id=970290
updated to latest inbound and pushed, the test_content.html failures are gone, still have a couple to sort out:
https://tbpl.mozilla.org/?tree=Try&rev=56cbb5439f4f
for reference chunk 5 is failing and we have the exact same number of pass/todo for inbound + the try push referenced in comment 19.

Ahal, any further thoughts?
Flags: needinfo?(ahalberstadt)
Comment on attachment 8379742 [details] [diff] [review]
b2g.patch

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

as discussed in the bug comments on IRC- we are really close to seeing green here.  I found 2 TODO statements in this patch which show up when there is already a skip-if statement containing b2g or gonk, I have hand edited those and pushed to try server again.

r- for now.
Attachment #8379742 - Flags: review?(jmaher) → review-
It looks like not only is it the same number of tests, but the same set as well (I double checked the first and last tests were the same both on the try run and inbound).

I don't really understand why this would be.. but it definitely seems to be cause by this patch somehow.
Flags: needinfo?(ahalberstadt)
:kanru, I see you had modified these two tests (test_browserElement_[NoAttr|NoWhitelist].html) just last week to wait for testready instead of load.  I have been struggling with these specific tests, and I did a push to try with a inbound clone from this morning yielding:
https://tbpl.mozilla.org/?tree=Try&rev=214e00e2d112 (chunk '5')

My try push is taking a lot of tests from b2g.json and turning them off inside of mochitest.ini files.  This effectively removes them from the tests.zip package at build time.

Any thoughts on this?
Flags: needinfo?(kchen)
Attached patch directory.patch (obsolete) — Splinter Review
Cleaning up directories containing mochitest.ini in b2g.json, b2g-debug.json, b2g-desktop.json
Attachment #8380737 - Flags: review?(jmaher)
(In reply to Joel Maher (:jmaher) from comment #23)
> :kanru, I see you had modified these two tests
> (test_browserElement_[NoAttr|NoWhitelist].html) just last week to wait for
> testready instead of load.  I have been struggling with these specific
> tests, and I did a push to try with a inbound clone from this morning
> yielding:
> https://tbpl.mozilla.org/?tree=Try&rev=214e00e2d112 (chunk '5')
> 
> My try push is taking a lot of tests from b2g.json and turning them off
> inside of mochitest.ini files.  This effectively removes them from the
> tests.zip package at build time.
> 
> Any thoughts on this?

Could you land your patch as-is then make a new patch to disable these two tests in bug 958761? I just want to make it clear the tests are disabled intentionally. I'll investigate why they are still failing.
Flags: needinfo?(kchen)
\o/ for the conversion btw.
(In reply to Kan-Ru Chen [:kanru] from comment #25)
> Could you land your patch as-is then make a new patch to disable these two
> tests in bug 958761? I just want to make it clear the tests are disabled
> intentionally. I'll investigate why they are still failing.

I mean bug 970290
Depends on: 976520
Attached patch b2g.patch (obsolete) — Splinter Review
Fixed some TODO's in this patch.
Attachment #8379742 - Attachment is obsolete: true
Attachment #8381402 - Flags: review?(jmaher)
Attached patch b2gupdated.patch (obsolete) — Splinter Review
Pulled in fresh from inbound and uploaded patch
Attachment #8381402 - Attachment is obsolete: true
Attachment #8381402 - Flags: review?(jmaher)
Attachment #8382364 - Flags: review?(jmaher)
Comment on attachment 8380737 [details] [diff] [review]
directory.patch

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

this is great, I got this to pass on try!  I had to handle some bitrot
Attachment #8380737 - Flags: review?(jmaher) → review+
Comment on attachment 8382364 [details] [diff] [review]
b2gupdated.patch

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

this is great, I got this to pass on try!  I had to handle some bitrot
Attachment #8382364 - Flags: review?(jmaher) → review+
Attachment #8380737 - Attachment is obsolete: true
Attachment #8382364 - Attachment is obsolete: true
Attachment #8386945 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/72ffff0fdad2
https://hg.mozilla.org/mozilla-central/rev/016a86a9aec2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Great to see progress here on moving these exclusions to where everybody will see them.

Is there a bug on file for getting rid of the rest of the things that are excluded via the testing/mochitest/b2g*.json files rather than using skip-if?  In particular:

 (A) all the b2g*.json files have a runtests list that excludes mochitests outside of a list of 7 directories.  This means both that:

    (1) it's not obvious in the directories not run that those tests aren't being run on b2g.  (Unless I'm misreading things, these are currently browser (probably ok), editor, embedding (maybe ok), extensions/cookie, gfx, image, intl, js, modules/libjar, netwerk, parser/htmlparser, security/manager, testing/mochitest, toolkit (maybe parts are ok), uriloader, webapprt (probably ok), and widget.)

    (2) any mochitests added outside of those directories won't be run on b2g, which means we'll continue accumulating *more* tests that don't work on B2G, when everybody writing new tests should be making their tests work on B2G unless they have a good reason not to

 (B) there's still a substantial (although, thanks to this bug, much smaller) list in the excludetests lists of those json files, which should be entirely eliminated and moved into manifests.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #36)
>     (1) it's not obvious in the directories not run that those tests aren't
> being run on b2g.  (Unless I'm misreading things, these are currently
> browser (probably ok), editor, embedding (maybe ok), extensions/cookie, gfx,
> image, intl, js, modules/libjar, netwerk, parser/htmlparser,
> security/manager, testing/mochitest, toolkit (maybe parts are ok),
> uriloader, webapprt (probably ok), and widget.)

At the moment the filtering happens at build time, so tests which are excluded simply don't get copied to the tests.zip. There is no "root" manifest yet. That being said, there are plans to change this. Though even when this happens it will still be hard to tell if a test is running on a given platform or not without looking at the root manifest. I think what we need is a tool that evaluates the manifest conditions and spits out a list of all active tests given your current build context. 

> 
>     (2) any mochitests added outside of those directories won't be run on
> b2g, which means we'll continue accumulating *more* tests that don't work on
> B2G, when everybody writing new tests should be making their tests work on
> B2G unless they have a good reason not to

Yes, this is a good point. Once we have a root manifest we can switch from whitelisting to blacklisting more easily. I should note that we'll still want to whitelist certain platforms. For example, we don't want to run all mochitests on emulator-ics, emulator-jb and emulator-kk. Instead we probably want a subset running on each.

>  (B) there's still a substantial (although, thanks to this bug, much
> smaller) list in the excludetests lists of those json files, which should be
> entirely eliminated and moved into manifests.

Yep, those are blocked on getting mochitest.ini in their various directories. I think ted was close to finishing off the last of them. Let's re-open this bug to track finishing them up.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch b2gupdates.patch (obsolete) — Splinter Review
Patch for removing all the remaining entries from b2g.json, b2g-debug.json, b2g-desktop.json
Attachment #8389423 - Flags: review?(jmaher)
Attached patch b2gupdates.patch (obsolete) — Splinter Review
Made a minor change in [default] section of dom/browser-element/mochitest/mochitest-oop.ini and dom/browser-element/mochitest/priority/mochitest.ini
Attachment #8389423 - Attachment is obsolete: true
Attachment #8389423 - Flags: review?(jmaher)
Attachment #8389617 - Flags: review?(jmaher)
Comment on attachment 8389617 [details] [diff] [review]
b2gupdates.patch

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

thank you for this patch, it is great on try server!
Attachment #8389617 - Flags: review?(jmaher) → review+
Attachment #8389617 - Attachment is obsolete: true
Attachment #8389801 - Flags: review+
once this lands we will still have the runtests directories to resolve- that should be adding skip-if to the default section of the remaining mochitest-plain directories that we don't run.
Attached patch removing runtests from b2g*.json (obsolete) — Splinter Review
Removing the entries in runtests from b2g*.json files
Attachment #8389913 - Flags: review?(jmaher)
Made a minor fix for including a skip-if for Harness_sanity mochitest.ini file.
Attachment #8389913 - Attachment is obsolete: true
Attachment #8389913 - Flags: review?(jmaher)
Attachment #8389993 - Flags: review?(jmaher)
https://hg.mozilla.org/mozilla-central/rev/35e1bf061284
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8389993 [details] [diff] [review]
removing runtests from b2g*.json

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

verified on try server!
Attachment #8389993 - Flags: review?(jmaher) → review+
Blocks: 983262
I filed bug 983262 to remove the .json files after this lands.
https://hg.mozilla.org/mozilla-central/rev/1c02b7858d13
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Will there be any capability to run from a manifest locally? We talked it about it previously, but it's actually quite useful for triaging on new platforms.
I am not sure what that means?  Do you mean if you have a specific device that doesn't pass all the tests, can you create a custom manifest for a local run and run the tests using that manifest?  if that is the question, yes- you could have done it before and you could do it now.
Yeah, sorry, that wasn't especially clear. What I meant was that working with a centralized manifest for doing the initial triage is somewhat easier in the centralized json format rather than hopping around the directories to hit the distributed .inis. 

When I spoke with Andrew about this recently, it was implied we might keep the json functionality for local use, even though the current json files would be removed. I was wondering if that would still be the case.
the goal is to remove the .json files, but you bring up a valid point.  one way to do this is to have a custom master mochitest.ini file and slowly include directories- it is not as efficient as the current .json format, but when looking at the bigger picture, it ends up being the same amount of work.
Sure, as long as there's a route where you can look and get an overview. Thanks!
(and obviously, once you're done, distribute into proper format--just mean while the triage is underway)
You need to log in before you can comment on or make changes to this bug.