Closed Bug 1032878 Opened 5 years ago Closed 5 years ago

Windows 7 mochitest runs contain failures in test_load_candidates.html but are marked as passing

Categories

(Testing :: Mochitest, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: emorley, Assigned: martijn.martijn)

References

(Blocks 2 open bugs)

Details

(Keywords: sheriffing-P1)

Attachments

(4 files, 11 obsolete files)

15.84 KB, patch
surkov
: review+
Details | Diff | Splinter Review
1.41 KB, patch
rlin
: review+
shelly
: feedback+
Details | Diff | Splinter Review
1.35 KB, patch
martijn.martijn
: review+
Details | Diff | Splinter Review
5.63 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
Broken out from bug 1024535.

Somehow, the failures seen in bug 1024535 are not causing the Windows 7 mochitest-1 jobs to fail.

eg:

https://tbpl.mozilla.org/php/getParsedLog.php?id=42814621&tree=Mozilla-Central

19:15:52     INFO -  5840 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_load_candidates.html | Error events on source children should not bubble
...
19:27:50     INFO - # TBPL SUCCESS #

(note the return code 1 in the run is always present - it's bug 944364)
http://mxr.mozilla.org/mozilla-central/source/content/media/test/test_load_candidates.html?force=1#48
48   v.addEventListener("error", function(){ok(false,"Error events on source children should not bubble");}, false);
More from the log:
20:13:50     INFO -  5837 INFO TEST-PASS | /tests/content/media/test/test_load_candidates.html | [finished gizmo.mp4-9] Length of array should match number of running tests
20:13:50     INFO -  5838 INFO TEST-INFO | /tests/content/media/test/test_load_candidates.html | Finished at Mon Jun 30 2014 20:13:50 GMT-0700 (Pacific Standard Time) (1404184430.772s)
20:13:50     INFO -  5839 INFO TEST-INFO | /tests/content/media/test/test_load_candidates.html | Running time: 1.629s
20:13:50     INFO -  5840 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_load_candidates.html | Error events on source children should not bubble
20:13:50     INFO -  5841 INFO TEST-START | /tests/content/media/test/test_load_same_resource.html
20:13:52     INFO -  file=[xpconnect wrapped nsILocalFile]
20:13:52     INFO -  5842 INFO TEST-INFO | MEMORY STAT vsize after test: 1112973312
20:13:52     INFO -  5843 INFO TEST-INFO | MEMORY STAT vsizeMaxContiguous after test: 621936640
20:13:52     INFO -  5844 INFO TEST-INFO | MEMORY STAT residentFast after test: 219361280
20:13:52     INFO -  5845 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 65542894
20:13:52     INFO -  5846 INFO TEST-END | /tests/content/media/test/test_load_same_resource.html | finished in 2096ms

I'm guessing by the time the error event is fired, we've moved onto the next test, right?
this could be one of three things from briefly looking at this:
1) we have a case where tests are not completed, but we have called simpletest.finish()
2) the log truncation for a given test has a condition where failures can be ignored
3) a full moon
(In reply to Joel Maher (:jmaher) from comment #3)
> this could be one of three things from briefly looking at this:
> 1) we have a case where tests are not completed, but we have called
> simpletest.finish()

It sounds like the harness should just be made to fail the run, if an ok()/... condition fails - regardless of whether simpletest.finish() was already called - right? And for bonus points, fail the run if an event handler added during a test is triggered after that test called simpletest.finish() ?
Is there someone who would be up for owning this? :-)
Keywords: sheriffing-P1
Attached patch 1032878.diff (obsolete) — Splinter Review
I think something like this might catch the case where SimpleTest.ok, etc is called after SimpleTest.finish(), but totally not sure.
Ok, it seems to work, but I see this js error in some tests, like content/canvas/test/test_ImageData_ctor.html:
JavaScript error: http://mochi.test:8888/tests/SimpleTest/TestRunner.js, line 710: $(...).contentWindow.SimpleTest is undefined
Some of the tests don't use SimpleTest at all, so it needs a check for SimpleTest.

I thought about another way to solve this, by changing commands in SimpleTest to track when results are logged after SimpleTest.finish is called and report that to TestRunner. But I don't think that works, because these kinds of failures are happening when the document is unloading, and then the SimpleTest document can't access TestRunner (the parent frame).
Attached patch 1032878_v2.diff (obsolete) — Splinter Review
So I think this is correct, I haven't tested it yet.
Anyway, I have a couple of questions myself:
I didn't see any "Results logged after SimpleTest.finish()" message in https://tbpl.mozilla.org/php/getParsedLog.php?id=45116070&tree=Try&full=1
Which led me to believe the error wasn't caught. 
But the log shows:
00:39:52     INFO -  5538 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_load_candidates.html | Error events on source children should not bubble
00:39:52     INFO -  TEST-INFO | expected PASS
And:
00:51:37  WARNING -  6657 INFO Failed:  2
00:51:37  WARNING -  One or more unittests failed.

So it seems to be caught. I'm not sure why INFO Failed is 2, though. Perhaps, I'm using structuredLogger not correctly, I don't really know how it works.
Why I don't see the "Results logged after SimpleTest.finish()" message in the log, is the biggest mystery to me.

Also, it would have been nice to know, which messages are coming from the mochitest harness and which ones are coming from the structuredLogger.

Btw, I tested this locally by doing a window.onbeforeunload = function() {
SimpleTest.ok(true, "this should be marked as a failure");
}
..after SimpleTest.finish().
It would perhaps be good to add a test somewhere, to make sure that the test harness is catching these kinds of failures.
I don't know of any, but I saw this:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/browser/browser.ini#18
So it seems to me that we could add tests and add them in mochitest.ini, like this: runif = test_harness_reporting = 1
..or something like that.
( http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/docs/manifestparser.rst#231 )
Attachment #8465847 - Attachment is obsolete: true
Attachment #8467192 - Flags: review?(jmaher)
Assignee: nobody → martijn.martijn
Another uncaught TEST-UNEXPECTED-FAIL issue was filed in bug 1048423, but that one seems unrelated to tests being run after SimpleTest.finish().
Ahmed was planning on getting the structured parser to always fail, when a "TEST-UNEXPECTED-FAIL" was found.
In that case, the patch could be changed to barf out a "TEST-UNEXPECTED-FAIL" when tests are running after SimpleTest.finish().
Comment on attachment 8467192 [details] [diff] [review]
1032878_v2.diff

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

can you write a unittest for this?
Attachment #8467192 - Flags: review?(jmaher) → review-
Oh, btw, I think the reason why failures are not being reported by the mochitest harness when the document is unloading:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#347
parentRunner can't be accessed when the document is in the unloading state, so we're left with the dump option there.
Comment on attachment 8467192 [details] [diff] [review]
1032878_v2.diff

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

on second thought, a unittest will be too hacky in our current framework, we need a selftest system for the harness in general which we don't have.
Attachment #8467192 - Flags: review- → review+
(In reply to Joel Maher (:jmaher) from comment #13)
> on second thought, a unittest will be too hacky in our current framework, we
> need a selftest system for the harness in general which we don't have.

This was filed as bug 1048446.
Comment on attachment 8467192 [details] [diff] [review]
1032878_v2.diff

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

Asking addtnl review from Ahmed, he should know if the structuredLogger part would be correct or not.
He's also working on getting this error detected from the python side, so he can also judge whether this would still be necessary.
Attachment #8467192 - Flags: review?(akachkach)
Comment on attachment 8467192 [details] [diff] [review]
1032878_v2.diff

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

Looks good to me!
This wouldn't be picked up by the test harness though, and wouldn't cause the job to go orange (from what I understood only the summary is taken into account).

This is most probably unrelated to structured logs, but we should find a more robust way to make the jobs orange but this patch is definitely an upgrade.
Attachment #8467192 - Flags: review?(akachkach) → review+
(In reply to Ahmed Kachkach [:akachkach] from comment #16)
> Looks good to me!
> This wouldn't be picked up by the test harness though, and wouldn't cause
> the job to go orange (from what I understood only the summary is taken into
> account).

Chatted with him on irc: TestRunner.updateUI([{ result: false }]) call would cause a fail to appear in that summary, see: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/TestRunner.js#825
He agreed with me, that's the case. So this patch would cause the job to go orange (which is what we want here).
Attached patch 1032878_v3.diff (obsolete) — Splinter Review
Ok, this is with a mochitest, which is disabled by default, but could be used once bug 1048446 is fixed.
While making this test, I noticed that test_sanitySimpletest.html was also causing failures. This happened, because of a SimpleTest.expectUncaughtException() after SimpleTest.finish() and inside expectUncaughtException, there is a SimpleTest.ok call that was made. This patch ensures that it's only called when SimpleTest.finish is not called yet.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=891e752d2fb6
Attachment #8467192 - Attachment is obsolete: true
Attached patch 1032878_v3.diff (obsolete) — Splinter Review
Sorry forgot to hg add the actual test.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d55a3c044e77
Attachment #8467389 - Attachment is obsolete: true
Ok, I'm getting all kinds of oranges, but unfortunately, the summary is empty. The logs also don't tell me anything, so it's really impossible to find out what goes wrong. I guess this is bug 1048288?
I thought the TestRunner.structuredLogger.error call would show up in the log (the "Results logged after SimpleTest.finish" one), but for some reason, it isn't.
I can reproduce this with test_offline_gzip.html.
Locally, I get to see:
206 ERROR Results logged after SimpleTest.finish() for /tests/browser/base/content/test/general/test_offline_gzip.html
Apparently, these kinds of errors are not logged in tbpl? I think they should. I guess this is basically bug 1048559. That bug has a patch, I'll have to wait for that before I should do another try run to get meaningful errors.
Blocks: 1048775
Attached patch 1032878_v4.diff (obsolete) — Splinter Review
Ok, with the help of Ahmed, this patch should log TEST-UNEXPECTED-FAIL, which then gets picked up by the tbpl parser.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=092633a28948
Attachment #8467415 - Attachment is obsolete: true
Attached file Failures with this patch (obsolete) —
These are the failures with the last patch, so those tests need to be fixed, at least.
There might be more, Windows try hasn't finished yet and there might be intermittent failures lurking around.

content/media/test/test_mediarecorder_getencodeddata.html content/media/webspeech/recognition/test/test_call_start_from_end_handler.html dom/base/test/test_domcursor.html dom/browser-element/mochitest/test_browserElement_inproc_KeyEvents.html dom/inputmethod/mochitest/test_bug960946.html content/media/test/test_autoplay_contentEditable.html dom/browser-element/mochitest/test_browserElement_inproc_KeyEvents.html dom/inputmethod/mochitest/test_bug960946.html browser/base/content/test/general/test_offline_gzip.html dom/browser-element/mochitest/test_browserElement_inproc_KeyEvents.html layout/base/tests/test_bug607529.html content/base/test/csp/test_CSP_bug888172.html

Chrome:
content/chrome/dom/tests/mochitest/chrome/test_docshell_swap.xul content/chrome/dom/workers/test/test_bug883784.xul content/a11y/accessible/tests/mochitest/attributes/test_obj_group.html content/a11y/accessible/tests/mochitest/events/test_docload.html content/chrome/dom/tests/mochitest/chrome/test_docshell_swap.xul content/chrome/dom/workers/test/test_bug883784.xul content/a11y/accessible/tests/mochitest/attributes/test_obj_group.html content/a11y/accessible/tests/mochitest/events/test_docload.html
Attached patch 1032878_testsfix.diff (obsolete) — Splinter Review
This makes the failing mochitest-plain/-chrome tests pass combined with 1032878_v4.diff.
Some of the changes I made are pretty logical, but for some I can't really explain why it helps or if it's a good change.
Note that these subtests weren't failing. They were passing, but they ran after the SimpleTest.finish() call.
Comment on attachment 8468198 [details] [diff] [review]
1032878_testsfix.diff

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

These would be the proper reviewers for the changes in the tests, I made:
test_mediarecorder_getencodeddata.html - cpearce@mozilla.com
test_call_start_from_end_handler.html - cpearce@mozilla.com
test_autoplay_contentEditable.html - cpearce@mozilla.com
test_offline_gzip.html - jgriffin@mozilla.com
test_domcursor.html - reuben.bmo@gmail.com
test_browserElement_inproc_KeyEvents.html - mounir@lamouri.fr
test_bug960946.html - wdeng@mozilla.com
test_bug607529.html - roc@ocallahan.org
test_docshell_swap.xul - wmccloskey@mozilla.com
test_bug883784.xul - amarchesini@mozilla.com
test_obj_group.html - surkov.alexander@gmail.com
test_docload.html - surkov.alexander@gmail.com

Joel, if you feel comfortable with reviewing it (partly), please do!

The test changes I'm less comfortable with, are: test_autoplay_contentEditable.html, (test_mediarecorder_getencodeddata.html, test_call_start_from_end_handler.html (less so), test_domcursor.html, browserElement_KeyEvents.js
Attachment #8468198 - Flags: review?(jmaher)
Pushed to try, combined with  1032878_v4.diff: https://tbpl.mozilla.org/?tree=Try&rev=2df2792e031f
Attachment #8468198 - Flags: review?(jmaher)
Attached patch 1032878_testsfix_v2.diff (obsolete) — Splinter Review
Previous try was catching some extra failures and I fixed the error I made in test_docload.html.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=592e24cf0f27
Attachment #8468198 - Attachment is obsolete: true
Attachment #8468576 - Flags: review?(jmaher)
Comment on attachment 8468576 [details] [diff] [review]
1032878_testsfix_v2.diff

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

too many questions for a r+, maybe there is more context you could provide.

::: content/media/webspeech/recognition/test/test_call_start_from_end_handler.html
@@ +68,5 @@
> +    doneFunc: function() {
> +      //Pause the audio element, before unloading the document, otherwise
> +      //it will cause more results to fire after SimpleTest.finish
> +      audioTag.pause();
> +      SimpleTest.finish();

should this be executesoon(simpletest.finish) ?

::: dom/base/test/test_domcursor.html
@@ -89,5 @@
> -      req.continue();
> -      ok(false, "calling continue twice should fail");
> -    } catch (e) {
> -      ok(true, "calling continue twice should fail");
> -    }

this seems as though we are removing part of the test functionality

::: dom/browser-element/mochitest/browserElement_TopBarrier.js
@@ +63,5 @@
>  
>    mm.loadFrameScript(injectedScript, /* allowDelayedLoad = */ false);
>  
> +  // 8 is the number of is() calls
> +  waitForMessages(8);

how did you determine this?  is there a chance this could be changed in the future and fail again?

::: dom/inputmethod/mochitest/test_bug960946.html
@@ +94,5 @@
>      ok(true, 'sendKey success');
> +    gBackSpaceCounter++;
> +    if (gBackSpaceCounter == 3) {
> +      inputmethod_cleanup();
> +    }

how did you know to cleanup at 3?

::: dom/tests/mochitest/chrome/test_docshell_swap.xul
@@ +36,5 @@
>  
>      function gotPong(target_ok) {
>        pongCount++;
>        ok(target_ok, "message went to correct target");
> +      if (pongCount == 2) {

I don't understand why we need 2?

::: dom/workers/test/test_bug883784.jsm
@@ +13,5 @@
>          xhr.open('GET', event.data.url, false);
>          xhr.onreadystatechange = function() {
>            if (xhr.readyState == 4) {
>              ok(true, "Blob readable!");
> +            finish();

is there a chance that we could fail to get readyState = 4 and not finish?
Attachment #8468576 - Flags: review?(jmaher) → review-
(In reply to Joel Maher (:jmaher) from comment #28)
> Comment on attachment 8468576 [details] [diff] [review]
> too many questions for a r+, maybe there is more context you could provide.

Yeah, it is often necessary to look at the test files completely to see why the changes are necessary.

> :::
> content/media/webspeech/recognition/test/test_call_start_from_end_handler.
> html
> @@ +68,5 @@
> > +    doneFunc: function() {
> > +      //Pause the audio element, before unloading the document, otherwise
> > +      //it will cause more results to fire after SimpleTest.finish
> > +      audioTag.pause();
> > +      SimpleTest.finish();
> 
> should this be executesoon(simpletest.finish) ?

It didn't seem necessary, with this change, I wouldn't get failures. Or did you mean replace the audioTag.pause() with executeSoon?

> ::: dom/base/test/test_domcursor.html
> @@ -89,5 @@
> > -      req.continue();
> > -      ok(false, "calling continue twice should fail");
> > -    } catch (e) {
> > -      ok(true, "calling continue twice should fail");
> > -    }
> 
> this seems as though we are removing part of the test functionality

Yes, for some reason, these tests seem to insist on running when the document is unloading, which is not the time to run tests. 
I don't think that's what the test author wanted, anyway.
I'll defer to him on this one to see on how to fix this.
 
> ::: dom/browser-element/mochitest/browserElement_TopBarrier.js
> @@ +63,5 @@
> >  
> >    mm.loadFrameScript(injectedScript, /* allowDelayedLoad = */ false);
> >  
> > +  // 8 is the number of is() calls
> > +  waitForMessages(8);
> 
> how did you determine this?  is there a chance this could be changed in the
> future and fail again?

http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/browserElement_TopBarrier.js#31
It's basically the amount of calls in injectedScript.
Jlebar added 2 tests in the patch part 5 of bug 771273, but he forgot to increase the number from 6 to 8.
So yes, this could change in the future and fail again, if someone would add an is() call without increasing this number (but with the patch in this bug, it would cause a failure!). I could add a comment before injectedScript that if someone adds is() calls, it should also increase the number in waitForMessages.

> ::: dom/inputmethod/mochitest/test_bug960946.html
> @@ +94,5 @@
> >      ok(true, 'sendKey success');
> > +    gBackSpaceCounter++;
> > +    if (gBackSpaceCounter == 3) {
> > +      inputmethod_cleanup();
> > +    }
> 
> how did you know to cleanup at 3?

See http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/mochitest/test_bug960946.html?force=1#77
This is done in test_sendKey, where you can see that the backspace key is sythesized 3 times.

> 
> ::: dom/tests/mochitest/chrome/test_docshell_swap.xul
> @@ +36,5 @@
> >  
> >      function gotPong(target_ok) {
> >        pongCount++;
> >        ok(target_ok, "message went to correct target");
> > +      if (pongCount == 2) {
> 
> I don't understand why we need 2?

As I see it, there are 2 messageListeners for pong:
http://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/chrome/test_docshell_swap.xul#49
http://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/chrome/test_docshell_swap.xul#63
Perhaps it would be better to remove one of the messageListeners.

> ::: dom/workers/test/test_bug883784.jsm
> @@ +13,5 @@
> >          xhr.open('GET', event.data.url, false);
> >          xhr.onreadystatechange = function() {
> >            if (xhr.readyState == 4) {
> >              ok(true, "Blob readable!");
> > +            finish();
> 
> is there a chance that we could fail to get readyState = 4 and not finish?

That should not be happening, afaict.
I could add an onerror event handler, though, that would causes an unexpected fail, so this wouldn't cause a timeout.
Joel, just let me know which changes you're not comfortable with reviewing (which files) and I'll defer those to others.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #27)
> Created attachment 8468576 [details] [diff] [review]
> 1032878_testsfix_v2.diff
> 
> Previous try was catching some extra failures and I fixed the error I made
> in test_docload.html.
> Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=592e24cf0f27

test_autoplay_contentEditable.html is still failing intermittently on Windows (now Windows 7 opt) with this patch.

The rest looks ok, apart from test_load_candidates.html on Windows 7 opt, but that's what bug 1024535 is about.
Thank you for doing this! :-)

In the try run I see:
13:28:25     INFO -  2207 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_load_candidates.html | Error events on source children should not bubble
...
13:28:25     INFO -  2208 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_load_candidates.html | Error events on source children should not bubble - 1 result(s) logged after SimpleTest.finish()

Should (/could) this be:
13:28:25     INFO -  2207 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_load_candidates.html | Error events on source children should not bubble
...
13:28:25     INFO -  2208 INFO TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_load_candidates.html | 1 result(s) logged after SimpleTest.finish()!

...in order to make it clearer?
Yes, it can. I agree that for TEST-UNEXPECTED-FAIL subtests that are running after SimpleTest.finish, it's double.
But for TEST-PASS subtests that are running after SimpleTest.finish, you want to see that info, because TEST-PASS is normally not shown on tbpl, I think.
> ::: dom/base/test/test_domcursor.html
> @@ -89,5 @@
> > -      req.continue();
> > -      ok(false, "calling continue twice should fail");
> > -    } catch (e) {
> > -      ok(true, "calling continue twice should fail");
> > -    }
> 
> this seems as though we are removing part of the test functionality

Ruben, you wrote this test as part of bug 837917.
The part of the test that is highlighted above, is running when the document is unloading, after SimpleTest.finish(). That is not correct, because subtests should always run before SimpleTest.finish(). 
Can you tell me, if I can just remove the above part of the test or how to rewrite it in such a way, that these subtests aren't run after SimpleTest.finish().
Flags: needinfo?(reuben.bmo)
r=me on the change to test_domcursor.html. The onsuccess callback fires synchronously, and the only reason the test was working is because between the first and second call to req.continue() we fired an error in the cursor.
Flags: needinfo?(reuben.bmo)
Attached patch 1032878_v3.diff (obsolete) — Splinter Review
I tried to address most of your questions.
test_domcursor.html is already reviewed now by Reuben, you can ignore that part.

> > should this be executesoon(simpletest.finish) ?
> 
> It didn't seem necessary, with this change, I wouldn't get failures. Or did
> you mean replace the audioTag.pause() with executeSoon?

SimpleTest.executeSoon didn't seem to make any difference here, so I left this as it is.
Attachment #8468576 - Attachment is obsolete: true
Attachment #8469744 - Flags: review?(jmaher)
Comment on attachment 8469744 [details] [diff] [review]
1032878_v3.diff

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

Chris, could you review test_autoplay_contentEditable.html, test_mediarecorder_getencodeddata.html, test_mediatrack_events.html and test_call_start_from_end_handler.html by any chance?

In this bug, I'm trying to enforce that subtests are run before SimpleTest.finish().
Some of the mochitests aren't doing that, I'm trying to fix them in this patch.
Attachment #8469744 - Flags: review?(cpearce)
Duplicate of this bug: 677964
(In reply to Ahmed Kachkach [:akachkach] from comment #16)
> This is most probably unrelated to structured logs, but we should find a
> more robust way to make the jobs orange but this patch is definitely an
> upgrade.

This also came up in bug, comment 8. I filed bug 1050641 for it.
Depends on: 679894
Comment on attachment 8469744 [details] [diff] [review]
1032878_v3.diff

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

the other files not reviewed by others look great.  Thanks for adding some comments.
Attachment #8469744 - Flags: review?(jmaher) → review+
Comment on attachment 8469744 [details] [diff] [review]
1032878_v3.diff

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

r=cpearce for the test_autoplay_contentEditable.html change.

For test_mediarecorder_getencodeddata.html you should ask Randy Lin for review.

For test_mediarecorder_getencodeddata.html and test_mediatrack_events.html you should ask Shelly Lin for review.

For test_call_start_from_end_handler.html you should ask :smaug.

Ideally, these would all be separate patches.
Attachment #8469744 - Flags: review?(cpearce) → review+
Ok, this is without test_mediarecorder_getencodeddata.html, test_mediatrack_events.html and test_call_start_from_end_handler.html
I might as wel ask Surkov for the accessibility changes in this patch.
Attachment #8467931 - Attachment is obsolete: true
Attachment #8469744 - Attachment is obsolete: true
Attachment #8470594 - Flags: review?(surkov.alexander)
Fix for test_mediarecorder_getencodeddata.html as part of bug 1032878.

Fyi, in this bug, I'm trying to enforce that subtests are run before SimpleTest.finish(). I'm trying to fix the tests that don't do that.
Attachment #8470597 - Flags: review?(rlin)
Attachment #8470597 - Flags: feedback?(slin)
Attached patch 1032878_v4c.diff (obsolete) — Splinter Review
Fix for test_mediatrack_events.html as part of bug 1032878
Attachment #8470598 - Flags: review?(slin)
Attached patch 1032878_v4d.diff (obsolete) — Splinter Review
Fix for test_call_start_from_end_handler.html as part of bug 1032878

Fyi, in this bug, I'm trying to enforce that subtests are run before SimpleTest.finish(). I'm trying to fix the tests that don't do that.
Attachment #8470599 - Flags: review?(bugs)
Attachment #8467931 - Attachment is obsolete: false
Comment on attachment 8470598 [details] [diff] [review]
1032878_v4c.diff

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

Thank you:)

r=me with the following updates.

::: content/media/test/test_mediatrack_events.html
@@ +69,5 @@
>          element.onpause = null;
> +        //This helps to prevent these events from firing after SimpleTest.finish()
> +        //on B2G ICS Emulator, but not sure they have been run at all, then
> +        element.audioTracks.onremovetrack = null;
> +        element.videoTracks.onremovetrack = null;

Can you null out other event handler(onaddtrack, onchange...) as well? Thanks!
Attachment #8470598 - Flags: review?(slin) → review+
Attachment #8470597 - Flags: feedback?(slin) → feedback+
Comment on attachment 8470597 [details] [diff] [review]
1032878_v4b.diff [checked in]

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

Thanks. r=me for this onee.
Attachment #8470597 - Flags: review?(rlin) → review+
Attachment #8470599 - Flags: review?(bugs) → review+
Comment on attachment 8470594 [details] [diff] [review]
1032878_v4.diff [checked in]

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

::: accessible/tests/mochitest/events.js
@@ +388,5 @@
>        listenA11yEvents(false);
>  
>        var res = this.onFinish();
>        if (res != DO_NOT_FINISH_TEST)
> +        SimpleTest.executeSoon(SimpleTest.finish);

I don't think there's anything wrong here but I'm not sure I get what it's for.
(In reply to alexander :surkov from comment #49)
> Comment on attachment 8470594 [details] [diff] [review]
> I don't think there's anything wrong here but I'm not sure I get what it's
> for.

Without this change, subresults are logged after SimpleTest.finish(). The plan is to disallow that in the future (I chatted with you on irc, and you were ok with the change).
Comment on attachment 8470594 [details] [diff] [review]
1032878_v4.diff [checked in]

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

r=me per irc:
mwargers: surkov, regarding bug 1032878, comment 49. Without that change, there are mochitest subsresults logged after SimpleTest.finish(). The plan is to disallow that in the future
Attachment #8470594 - Flags: review?(surkov.alexander) → review+
With onaddtrack, onchange nulled out as well now. Carrying over r+.
Attachment #8470598 - Attachment is obsolete: true
Attachment #8470988 - Flags: review+
Attachment #8470594 - Attachment description: 1032878_v4.diff → 1032878_v4.diff (for check-in)
Attachment #8470597 - Attachment description: 1032878_v4b.diff → 1032878_v4b.diff (for check-in)
Attachment #8470599 - Attachment description: 1032878_v4d.diff → 1032878_v4d.diff (for check-in)
Sorry about that. These leaks were actually also shown in the try builds, but I thought it was bug 1045822.
Apparently, one of the test changes makes this leak much more frequent.
I'll try to find out which one of the changes causes this issue.
It has to be test_mediatrack_events, test_call_start_from_end_handler.html or test_mediarecorder_getencodeddata.html
Flags: needinfo?(martijn.martijn)
Given the leaked URLs showing in the logs, my money's on the patch for test_call_start_from_end_handler.html. It's specifically showing up in the list...

Open https://tbpl.mozilla.org/php/getParsedLog.php?id=45693376&full=1&branch=mozilla-inbound and search for "Leaked URLs"
Attachment #8470599 - Attachment description: 1032878_v4d.diff (for check-in) → 1032878_v4d.diff
Attachment #8470599 - Attachment is obsolete: true
Comment on attachment 8470594 [details] [diff] [review]
1032878_v4.diff [checked in]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f4a23d34287
Attachment #8470594 - Attachment description: 1032878_v4.diff (for check-in) → 1032878_v4.diff [checked in]
Comment on attachment 8470597 [details] [diff] [review]
1032878_v4b.diff [checked in]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb76ab1816ad
Attachment #8470597 - Attachment description: 1032878_v4b.diff (for check-in) → 1032878_v4b.diff [checked in]
Comment on attachment 8470988 [details] [diff] [review]
1032878_v4c.diff [checked in]

https://hg.mozilla.org/integration/mozilla-inbound/rev/813ad3e96004
Attachment #8470988 - Attachment description: 1032878_v4c.diff (for check-in) → 1032878_v4c.diff [checked in]
Depends on: 1052775
I filed bug 1032878 on getting a fix for test_call_start_from_end_handler.html.
Another try run: https://tbpl.mozilla.org/?tree=Try&rev=7cbc50ae5455
This should only fail with test_call_start_from_end_handler.html - bug 1052775.
Are those "No description given for subtest, please fix" shown when a test doesn't give asserts a name? (name === undefined in ok, is, etc.)

Because I ran into an issue with such tests and just added a default value when no name is given (bug 1052937)
(In reply to Ahmed Kachkach [:akachkach] from comment #64)
> Are those "No description given for subtest, please fix" shown when a test
> doesn't give asserts a name? (name === undefined in ok, is, etc.)

Yes, indeed.
I combined the tryserver run with the patch for bug 744387. I thought that there weren't that many tests that were misusing SimpleTest.is()/ok()/etc, but boy, was I wrong! Fixing all those tests, would be quite some work, see bug 744387.
I thought the same and was also surprised with the result of your try run :)! I guess at this point it's probably OK to put some default value (yours may be more explicit than just "undefined subtest name") that would hopefully encourage devs to fix their tests if they don't get helpful test results.
Ok, all the failing tests have been fixed now. Another try run to make sure there are no oranges left with 1032878_v4.diff: https://tbpl.mozilla.org/?tree=Try&rev=efbe0671b097

(In reply to Ahmed Kachkach [:akachkach] from comment #66)
> I thought the same and was also surprised with the result of your try run
> :)! I guess at this point it's probably OK to put some default value (yours
> may be more explicit than just "undefined subtest name") that would
> hopefully encourage devs to fix their tests if they don't get helpful test
> results.

Agreed, this is being tracked in bug 744387.
Comment on attachment 8467931 [details] [diff] [review]
1032878_v4.diff

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

Try servers seem to be going green this time.
Attachment #8467931 - Flags: review?(jmaher)
Comment on attachment 8467931 [details] [diff] [review]
1032878_v4.diff

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

To make more clear what this patch is doing, this makes sure that any SimpleTest.ok/is/etc. calls are done before SimpleTest.finish. Otherwise a test failure is logged.
Comment on attachment 8467931 [details] [diff] [review]
1032878_v4.diff

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

2 small things, overall this looks good.

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +710,5 @@
> +        $('testframe').contentWindow.addEventListener('unload', function() {
> +           var testwin = $('testframe').contentWindow;
> +           if (testwin.SimpleTest && testwin.SimpleTest._tests.length != testwin.SimpleTest.testsLength) {
> +             var wrongtestlength = testwin.SimpleTest._tests.length - testwin.SimpleTest.testsLength;
> +             var wrongtestnames = testwin.SimpleTest._tests[testwin.SimpleTest.testsLength].name

nit: missing a semicolon at the end here.

@@ +714,5 @@
> +             var wrongtestnames = testwin.SimpleTest._tests[testwin.SimpleTest.testsLength].name
> +             for (var i = 1; i < wrongtestlength; i++) {
> +               wrongtestnames += ', ' + testwin.SimpleTest._tests[testwin.SimpleTest.testsLength + i].name;
> +             }
> +             TestRunner.structuredLogger.testStatus(TestRunner.currentTestURL, wrongtestnames, 'FAIL', 'PASS', (wrongtestlength) + " result(s) logged after SimpleTest.finish(), showing first one");

my only concern here is that the list of names (comma separated) doesn't conform to other structured logs.  Would you want to have one message for each test that showed up unexpectedly?
Attachment #8467931 - Flags: review?(jmaher) → review+
Note to self, apart from comment 70, need to also look at review comment 32.
Ok, this does one message for each tests.
I tested this patch locally, you want me to also test this on try again?

I addressed comment 32 with comment 33 already, btw. I need the info in case the subtest would pass, but would be logged after SimpleTest.finish().
Attachment #8467931 - Attachment is obsolete: true
Attachment #8477472 - Flags: review?(jmaher)
Comment on attachment 8477472 [details] [diff] [review]
1032878_v5.diff (for check-in)

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

thanks!
Attachment #8477472 - Flags: review?(jmaher) → review+
Attachment #8468068 - Attachment is obsolete: true
I tested  1032878_v5.diff locally, it's functionally (only changing the report message) not different from  1032878_v4.diff and that gave a green try run: https://tbpl.mozilla.org/?tree=Try&rev=efbe0671b097 (comment 67).
Attachment #8477472 - Attachment description: 1032878_v5.diff → 1032878_v5.diff (for check-in)
Depends on: 1058072
Depends on: 1058087
Depends on: 1058109
No longer depends on: 1058109
https://hg.mozilla.org/mozilla-central/rev/2b03cc32da6e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1058451
Depends on: 1058797
You need to log in before you can comment on or make changes to this bug.