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

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Ed Morley [:emorley]]

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)

Comment 1

[Ed Morley [:emorley]]

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);

Comment 2

[Ed Morley [:emorley]]

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?

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 4

[Ed Morley [:emorley]]

(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() ?

Comment 5

[Ed Morley [:emorley]]

Is there someone who would be up for owning this? :-)

Comment 6

[Martijn Wargers (dead)]

Attachment: 1032878.diff

I think something like this might catch the case where SimpleTest.ok, etc is called after SimpleTest.finish(), but totally not sure.

Comment 7

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=4ac848f27b15

Comment 8

[Martijn Wargers (dead)]

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).

Comment 9

[Martijn Wargers (dead)]

Attachment: 1032878_v2.diff

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 )

Comment 10

[Martijn Wargers (dead)]

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 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8467192 [details] [diff] [review]
1032878_v2.diff

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

can you write a unittest for this?

Comment 12

[Martijn Wargers (dead)]

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 13

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 14

[Martijn Wargers (dead)]

(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 15

[Martijn Wargers (dead)]

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.

Comment 16

[Ahmed Kachkach [: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.

Comment 17

[Martijn Wargers (dead)]

(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).

Comment 18

[Martijn Wargers (dead)]

Attachment: 1032878_v3.diff

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

Comment 19

[Martijn Wargers (dead)]

Attachment: 1032878_v3.diff

Sorry forgot to hg add the actual test.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d55a3c044e77

Comment 20

[Martijn Wargers (dead)]

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.

Comment 21

[Martijn Wargers (dead)]

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.

Comment 22

[Martijn Wargers (dead)]

Attachment: 1032878_v4.diff

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

Comment 23

[Martijn Wargers (dead)]

Attachment: Failures with this patch

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

Comment 24

[Martijn Wargers (dead)]

Attachment: 1032878_testsfix.diff

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 25

[Martijn Wargers (dead)]

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 - [e-mail]
test_call_start_from_end_handler.html - [e-mail]
test_autoplay_contentEditable.html - [e-mail]
test_offline_gzip.html - [e-mail]
test_domcursor.html - [e-mail]
test_browserElement_inproc_KeyEvents.html - [e-mail]
test_bug960946.html - [e-mail]
test_bug607529.html - [e-mail]
test_docshell_swap.xul - [e-mail]
test_bug883784.xul - [e-mail]
test_obj_group.html - [e-mail]
test_docload.html - [e-mail]

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

Comment 26

[Martijn Wargers (dead)]

Pushed to try, combined with  1032878_v4.diff: https://tbpl.mozilla.org/?tree=Try&rev=2df2792e031f

Comment 27

[Martijn Wargers (dead)]

Attachment: 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

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

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?

Comment 29

[Martijn Wargers (dead)]

(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.

Comment 30

[Martijn Wargers (dead)]

Joel, just let me know which changes you're not comfortable with reviewing (which files) and I'll defer those to others.

Comment 31

[Martijn Wargers (dead)]

(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.

Comment 32

[Ed Morley [:emorley]]

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?

Comment 33

[Martijn Wargers (dead)]

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.

Comment 34

[Martijn Wargers (dead)]

> ::: 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().

Comment 35

[Reuben Morais [:reuben]]

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.

Comment 36

[Martijn Wargers (dead)]

Attachment: 1032878_v3.diff

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.

Comment 37

[Martijn Wargers (dead)]

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.

Comment 38

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=0cbc24350166

Comment 40

[Martijn Wargers (dead)]

(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.

Comment 41

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 42

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 43

[Martijn Wargers (dead)]

Attachment: 1032878_v4.diff [checked in]

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.

Comment 44

[Martijn Wargers (dead)]

Attachment: 1032878_v4b.diff [checked in]

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.

Comment 45

[Martijn Wargers (dead)]

Attachment: 1032878_v4c.diff

Fix for test_mediatrack_events.html as part of bug 1032878

Comment 46

[Martijn Wargers (dead)]

Attachment: 1032878_v4d.diff

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.

Comment 47

[Shelly Lin [:shelly]]

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!

Comment 48

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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.

Comment 49

[alexander :surkov (:asurkov)]

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.

Comment 50

[Martijn Wargers (dead)]

(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 51

[alexander :surkov (:asurkov)]

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

Comment 52

[Martijn Wargers (dead)]

Attachment: 1032878_v4c.diff [checked in]

With onaddtrack, onchange nulled out as well now. Carrying over r+.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f54984be223e
https://hg.mozilla.org/integration/mozilla-inbound/rev/991f8d5da382
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cfcc44660a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/780c948d79f3

Comment 54

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for introducing a mochitest-1 leak: https://hg.mozilla.org/integration/mozilla-inbound/rev/8db38d415e20
https://tbpl.mozilla.org/php/getParsedLog.php?id=45692656&tree=Mozilla-Inbound

Comment 55

[Martijn Wargers (dead)]

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

Comment 56

[Martijn Wargers (dead)]

1032878_v4.diff: https://tbpl.mozilla.org/?tree=Try&rev=1d7a1f0837e5
1032878_v4b.diff https://tbpl.mozilla.org/?tree=Try&rev=9647c969f79a

Comment 57

[Ryan VanderMeulen [:RyanVM]]

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"

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8470594 [details] [diff] [review]
1032878_v4.diff [checked in]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f4a23d34287

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8470597 [details] [diff] [review]
1032878_v4b.diff [checked in]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb76ab1816ad

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8470988 [details] [diff] [review]
1032878_v4c.diff [checked in]

https://hg.mozilla.org/integration/mozilla-inbound/rev/813ad3e96004

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f4a23d34287
https://hg.mozilla.org/mozilla-central/rev/cb76ab1816ad
https://hg.mozilla.org/mozilla-central/rev/813ad3e96004

Comment 62

[Martijn Wargers (dead)]

I filed bug 1032878 on getting a fix for test_call_start_from_end_handler.html.

Comment 63

[Martijn Wargers (dead)]

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.

Comment 64

[Ahmed Kachkach [:akachkach]]

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)

Comment 65

[Martijn Wargers (dead)]

(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.

Comment 66

[Ahmed Kachkach [:akachkach]]

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.

Comment 67

[Martijn Wargers (dead)]

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 68

[Martijn Wargers (dead)]

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.

Comment 69

[Martijn Wargers (dead)]

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 70

[Joel Maher ( :jmaher ) (UTC -8)]

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?

Comment 71

[Martijn Wargers (dead)]

Note to self, apart from comment 70, need to also look at review comment 32.

Comment 72

[Martijn Wargers (dead)]

Attachment: 1032878_v5.diff (for check-in)

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().

Comment 73

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8477472 [details] [diff] [review]
1032878_v5.diff (for check-in)

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

thanks!

Comment 74

[Martijn Wargers (dead)]

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).

Comment 75

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b03cc32da6e

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b03cc32da6e