Closed Bug 901387 Opened 6 years ago Closed 6 years ago

b2g.json cleanup and reorder

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

Details

Attachments

(1 file, 6 obsolete files)

Attached patch b2g.json.diff (wip) (obsolete) — Splinter Review
I was busy reordering the b2g.json to group the test files that have similar failures, together.
I'm also adding comments back as to why the tests are failing.
Since I'm grouping the related failures together, I don't have to that many text.

On the top of the list are the tests that are failing because XUL and XBL are disabled on B2G at the moment (bug 846090)

The atttached patch is a wip.
Attached patch b2g.json.diff (wip) (obsolete) — Splinter Review
Ok, I've dug through half of the exclude list now and filed bugs for every new issue that I was seeing, bug nrs mentioned in the exclude list.
Attachment #785559 - Attachment is obsolete: true
Assignee: nobody → martijn.martijn
Attached patch b2g.json.diff (obsolete) — Splinter Review
Another wip.
Attachment #786682 - Attachment is obsolete: true
Attached patch b2g.json.diff (obsolete) — Splinter Review
Updated wip.

There are some tests with "seems to pass" in it, those are being removed in bug 899654 (if everything goes to plan).

test_bug709083.html passes on b2g and I think it passes on Android too, so that one can be removed.
Attachment #787178 - Attachment is obsolete: true
Attached patch b2g.json.diff (obsolete) — Splinter Review
Ok, I'm combining this with bug 899654, where I was trying to remove some test files:
I'm removing these test files from the exclude list, which seem to pass on b2g:
     "content/html/content/test/forms/test_input_sanitization.html":"seems to pass",
     "dom/tests/mochitest/ajax/jquery/test_jQuery.html":"seems to pass",
     "content/base/test/test_CSP_inlinescript.html":"seems to pass",
     "content/base/test/test_CSP_inlinestyle.html":"seems to pass",
     "content/canvas/test/crossorigin/test_video_crossorigin.html":"seems to pass",
     "parser/htmlparser/tests/mochitest/test_bug709083.html": "seems to pass, bug 737020",

I'll do a try run for these.
Attachment #789670 - Attachment is obsolete: true
Blocks: 899654
Blocks: 737020
I retriggered chunk 1,2 en 4 10 times to check for intermittent orange for the newly removed test files from the exclude list.

parser/htmlparser/tests/mochitest/test_bug709083.html isn't actually tested, because that directory is not in the include list of b2g.
Comment on attachment 789811 [details] [diff] [review]
b2g.json.diff

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

I think this is ready for review (assuming the repeated chunk testing doesn't show up any failures).
This is only removing the test files mentioned in comment 4.

The rest of the list in the b2g.json file stays the same.
But I grouped similar failures together and added bug numbers and explanations to the failures, so it's easier to understand (this was a request from my team).
Attachment #789811 - Flags: review?(jgriffin)
All test chunks green!
Comment on attachment 789811 [details] [diff] [review]
b2g.json.diff

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

Thanks.

::: testing/mochitest/b2g.json
@@ +15,5 @@
> +    "content/base/test/test_classList.html":"1806 tests, 1 test failing because of xul",
> +    "content/base/test/test_title.html":"",
> +    "content/events/test/test_bug547996-2.xhtml":"16 tests total",
> +    "content/html/content/test/test_bug458037.xhtml":"",
> +  

nit: extra whitespace on this line

@@ +88,5 @@
>      "dom/imptests/webapps/DOMCore/tests/approved/test_Range-surroundContents.html":"",
>      "dom/imptests/webapps/DOMCore/tests/approved/test_Range-mutations.html":"Test timed out.",
>      "dom/encoding/test/test_stringencoding.html":"Test timed out on b2g board",
>  
> +  

nit: extra whitespace on this line

@@ +196,5 @@
> +    "content/html/content/test/test_bug619278.html":"bug 587671, need an invalidformsubmit observer",
> +    "content/html/content/test/test_bug622597.html":"bug 587671, need an invalidformsubmit observer",
> +
> +    "content/html/content/test/test_fullscreen-api.html":"time out, some kind of focus issue",
> +  

nit: extra whitespace on this line

@@ +326,5 @@
>      "dom/tests/mochitest/bugs/test_window_bar.html":"",
>  
> +    "dom/tests/mochitest/general/test_clipboard_events.html":"clipboard undefined",
> +    "content/base/test/test_copypaste.html":"clipboard undefined",
> +    "content/base/test/test_bug166235.html":"clipboard undefined",

Can we add these two content tests up with the other content tests, instead of inside the dom tests?
Attachment #789811 - Flags: review?(jgriffin) → review+
Attached patch b2g.json.diff (obsolete) — Splinter Review
Unfortunately, a large part of the patch didn't apply, so I had merge it by hand :/
Probably safer to do an extra try run.

(In reply to Jonathan Griffin (:jgriffin) from comment #9)
> > +    "dom/tests/mochitest/general/test_clipboard_events.html":"clipboard undefined",
> > +    "content/base/test/test_copypaste.html":"clipboard undefined",
> > +    "content/base/test/test_bug166235.html":"clipboard undefined",
> 
> Can we add these two content tests up with the other content tests, instead
> of inside the dom tests?

The point of the patch is to group related failures together, these test files are failing because clipboard is missing. I'd rather keep them together, if you don't mind.
Attachment #789811 - Attachment is obsolete: true
For some reason, previous try run completely busted, so triggered a new try run: https://tbpl.mozilla.org/?tree=Try&rev=be1be0ef0f2e
Comment on attachment 790443 [details] [diff] [review]
b2g.json.diff

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

Try run is ok.
Attachment #790443 - Flags: review?(jgriffin)
Comment on attachment 790443 [details] [diff] [review]
b2g.json.diff

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

Personally I think organizing alphabetically makes things easier to find than sorting by reason, but you're doing most of the work to maintain this file, so I definitely won't bikeshed.  ;)
Attachment #790443 - Flags: review?(jgriffin) → review+
My take: if you need a particular file/tree, ctrl-F or grep works great no matter how you arrange it.

What we're trying to position for is getting more help on fixing the tests, in which case getting an idea of what work will give the most gain makes a lot of sense. Bucketing by cause of failure helps with that (the other choice being independent docs, but those go out of sync quickly).
Attachment #790443 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/10e13449c6a2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
I filed bug 907995 as a sort-of follow-up.
You need to log in before you can comment on or make changes to this bug.