Running no tests is probably a bug in the test. The patch is one line, but probably a bunch of tests will have to be fixed.

This will almost surely cause test failures, to be fixed in v1. I'm using Bugzilla here so that I can autoland.

Autoland Patchset: Patches: 605890 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=3f60772d504d Try run started, revision 3f60772d504d. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=3f60772d504d

Try run for 3f60772d504d is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=3f60772d504d Results (out of 116 total builds): success: 95 warnings: 21 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-3f60772d504d

Callek helped me track down the cryptic failure (thanks!) and it turns out it's dom/tests/mochitest/chrome/window_focus_docnav.xul, which was added by bug 653230. It seems like this file is running no actual tests. Neil, do you know why this might be? I looked at the file but don't understand how it's supposed to work, since it's a chrome test.

That file is opened by http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/chrome/test_focus_docnav.xul#15

Test logs show that the test is outputing results. For example, see https://tbpl.mozilla.org/php/getParsedLog.php?id=10096386&tree=Mozilla-Inbound&full=1

Okay, I realized the check here was really in the wrong place. I've tried moving it to a different place, which should hopefully be more reliable and/or give better error messages. At a minimum, it should now output the right test URL (although I'm not totally sure why).

Autoland Patchset: Patches: 606324 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=acf6d5a537a9 Try run started, revision acf6d5a537a9. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=acf6d5a537a9

Try run for acf6d5a537a9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=acf6d5a537a9 Results (out of 115 total builds): success: 48 warnings: 67 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-acf6d5a537a9

Okay, so that was a false alarm: the failures were due to entirely different tests. Sorry for the noise. The list of failures from the new tryserver run looks more plausible, and is definitely more comprehensible. I'll look at fixing those.

Breakdown of failures: /tests/content/base/test/test_reentrant_flush.html: Bug 709256 added this test. It runs on the load handler, but has no waitForExplicitFinish(). What seems to happen is that on load, SimpleTest.js runs a load handler that calls finish(), which causes a failure with my patch because no tests have been run yet. Then the actual tests are still run (?). A workaround is to add waitForExplicitFinish() and finish(). (This is a false positive.) /tests/dom/plugins/test/test_pluginstream_seek_close.html: Bug 641738 changed it so it contained '<script type="application/javascript>' with no closing quote for the type attribute. This resulted in the script being eaten by the attribute, so no tests were run. Removing the ' type="application/javascript' fixes the test, which now passes. (This kind of error would be more reliably detected if parse errors caused tests to fail unless they opted out.) /tests/dom/tests/mochitest/bugs/test_bug622361.html: This test was just testing for leaks, so it didn't test anything beyond that. Adding an ok(true) at the end fixes it. (So this is a false positive.) /tests/dom/tests/mochitest/dom-level*: TODO: I looked at the first few failures, and they're all the same. They look old and very broken. DOMTestCase.js overwrites SimpleTest._logResult, and the overwritten version throws an exception at line 653 (parentRunner is null). The failing tests all hang for me on localhost; I'm not sure why they don't time out on the try server. I don't know why they work without this patch being applied. /tests/dom/tests/mochitest/whatwg/test_postMessage_hash.html: Bug 544097 accidentally removed a trailing ">" from the iframe that was being tested, which caused no tests to be run. Re-adding the ">" fixes it, so that the tests are run and pass. (This kind of error would be more reliably detected if parse errors caused tests to fail unless they opted out.) chrome://mochitests/content/chrome/content/base/test/chrome/test_bug357450.xul: Bug 659315 moved the file from content/base/test/test_bug357450.xul to content/base/test/chrome/test_bug357450.xul. The file had a <script src="test_bug357450.js">, and the move broke it. Changing to src="../test_bug357450.js" fixes it and actually runs the tests, which all pass. (This kind of error would be more reliably detected if resource load errors caused tests to fail unless they opted out.) chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_focus_docnav.xul: Test added by bug 653230 (it wasn't a false alarm after all). The problem might be related to the fact that it calls window.open(). TODO: I'm not sure what the problem is here. Why does it print "TEST-UNEXPECTED-FAIL | unknown test url | [SimpleTest.report()] No checks actually run."? chrome://mochitests/content/chrome/editor/libeditor/html/tests/test_bug616590.xul: This deliberately runs no tests. Bug 616590 comment 17 says it's fine to add ok(true) before the tests finish, so I did. (This is another false positive.) chrome://mochitests/content/chrome/layout/generic/test/test_bug632379.xul: This originally had SimpleTest.waitForExplicitFinish(), but bug 675201 removed it. The tests were being run with SimpleTest.waitForFocus(), but the page would apparently load before it got focus, so no tests were being run. Re-adding waitForExplicitFinish() fixes the problem. (This kind of error could be more reliably detected if a test that called waitForFocus() but finished before getting focus failed.) chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_tooltip_noautohide.xul: Same as previous: this originally had waitForExplicitFinish(), but bug 513707 removed it. Re-adding it causes it to pass. (This kind of error could be more reliably detected if a test that called waitForFocus() but finished before getting focus failed.) chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html: TODO: The test doesn't seem to work for me at all when I run it on localhost (it hangs). The doTest() function seems to never fire. I can't get the file to do anything at all: adding ok(false) early on doesn't do anything. This was added by bug 441737. I'm attaching a new patch that fixes the failures I was able to fix.

This new patch should have fewer failures. See comment 11 for an analysis. Any idea on how to fix the remaining ones?

Autoland Patchset: Patches: 606612 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=5e836ef6f8a3 Try run started, revision 5e836ef6f8a3. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=5e836ef6f8a3

> It runs on the load handler, but has no waitForExplicitFinish(). Hrm. Once upon a time mochitest guaranteed that things passed to addLoadEvent would all run _before_ the test harness called "finish". Did that change? We have lots of tests depending on that behavior, iirc; some of them may happen to do a check or two before the load event and so wouldn't get caught by this patch, but would also no longer be testing what it's supposed to test or something?

Comment on attachment 606612 [details] [diff] [review] Patch v0.75, some tests fixed but some tests will still fail Review of attachment 606612 [details] [diff] [review]: ----------------------------------------------------------------- nice cleanup in the tests.

(In reply to Boris Zbarsky (:bz) from comment #14) > Hrm. Once upon a time mochitest guaranteed that things passed to > addLoadEvent would all run _before_ the test harness called "finish". Did > that change? We have lots of tests depending on that behavior, iirc; some > of them may happen to do a check or two before the load event and so > wouldn't get caught by this patch, but would also no longer be testing what > it's supposed to test or something? Well, the behavior is weird . . . first a failure was reported for no tests run, then the actual tests still ran. See the log: https://tbpl.mozilla.org/php/getParsedLog.php?id=10102333&tree=Try#error0 So I honestly don't understand what happened with that test. How are there passes logged after an unexpected fail? It could be that I was fixing the wrong thing and my fix was actually broken. If the test passes on this run, then I guess that will suggest that my fix did something. I moved the check to SimpleTest.finish(), so there should really be no way it gets called before finish().

(In reply to Aryeh Gregor from comment #11) > /tests/dom/tests/mochitest/dom-level*: TODO: I looked at the first few > failures, and they're all the same. They look old and very broken. > DOMTestCase.js overwrites SimpleTest._logResult, and the overwritten version > throws an exception at line 653 (parentRunner is null). The failing tests > all hang for me on localhost; I'm not sure why they don't time out on the > try server. I don't know why they work without this patch being applied. I think this is bug 657485. These currently just throw an exception, which effectively disables the test. I suggest that for the purposes of this patch, these tests can just be disabled, with a followup bug filed to fix them. They're not doing anything now anyway, so it wouldn't be a regression.

Hmm. So looking at SimpleTest.js, it certainly calls addLoadEvent with a function that will call finish(). Since things added via addLoadEvent are executed in order, that would mean that any test using addLoadEvent but not waitForExplicitFinish() is somewhat broken. Unless I'm missing something, of course.

Ok, so here's the list of files in my tree that have calls to addLoadEvent but not calls to waitForExplicitFinish, if I did my greps right: accessible/tests/mochitest/browser.js accessible/tests/mochitest/common.js build/pgo/js-input/string-unpack-code.html content/base/test/test_reentrant_flush.html content/media/test/test_preload_actions.html dom/tests/mochitest/ajax/mochikit/MochiKit/DOM.js dom/tests/mochitest/ajax/mochikit/tests/MochiKit-DOM.html dom/tests/mochitest/chrome/489127.html js/src/jit-test/tests/sunspider/check-string-unpack-code.js js/src/metrics/jint/sunspider/string-unpack-code.js layout/base/tests/bug613807-1.html testing/extensions/community/chrome/content/MochiKit/MochiKit.js testing/mochitest/MochiKit/DOM.js testing/mochitest/MochiKit/packed.js testing/mochitest/tests/MochiKit-1.4.2/MochiKit/DOM.js testing/mochitest/tests/MochiKit-1.4.2/tests/test_MochiKit-DOM.html A lot of these are utilities. A few are files that depend on something else they include to call waitForExplicitFinish. test_reentrant_flush seems to be the only one that's actually broken, which is good. ;)

Comment on attachment 606612 [details] [diff] [review] Patch v0.75, some tests fixed but some tests will still fail Review of attachment 606612 [details] [diff] [review]: ----------------------------------------------------------------- SimpleTest.js should be in its own patch. Tests should probably be in per-component patches (maybe in blocking bugs), unless you find someone to do a cross-component review. It's easier to track (before/after). ::: content/base/test/chrome/test_bug357450.xul @@ +12,5 @@ > > <title>Test for Bug 357450</title> > <script type="application/javascript" > src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> > + <script type="text/javascript" src="../test_bug357450.js"></script> Fwiw, could add a comment that this file is shared with non-chrome tests. http://mxr.mozilla.org/mozilla-central/search?string=test_bug357450.js&case=1&find=%2Fcontent%2Fbase%2Ftest%2F ::: dom/plugins/test/mochitest/test_pluginstream_seek_close.html @@ +4,5 @@ > <script type="text/javascript" > src="/tests/SimpleTest/SimpleTest.js"></script> > <link rel="stylesheet" type="text/css" > href="/tests/SimpleTest/test.css" /> > +<script> Shouldn't you rather add the missing '"'? ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +670,5 @@ > if (SimpleTest._expectingUncaughtException) { > SimpleTest.ok(false, "expectUncaughtException was called but no uncaught exception was detected!"); > } > + if (SimpleTest._tests.length == 0) { > + SimpleTest.ok(false, "[SimpleTest.report()] No checks actually run."); s/report/finish/

(In reply to Aryeh Gregor from comment #7) > Okay, I realized the check here was really in the wrong place. I've tried Iirc, that was not the wrong place: it was the place used when running a test alone. And I gave up working on this issue because it was way too hard to get people to fix/review their tests. > moving it to a different place, which should hopefully be more reliable > and/or give better error messages. At a minimum, it should now output the > right test URL (although I'm not totally sure why). You're just moving it so it runs at each test end, no magic. (And thanks for resuming the work on this issue!) (In reply to Aryeh Gregor from comment #16) > Well, the behavior is weird . . . first a failure was reported for no tests > run, then the actual tests still ran. See the log: Starting the next test doesn't stops the currently executing one if it has not actually ended: what I/we call "leaking to the next test(s)".

(In reply to Boris Zbarsky (:bz) from comment #14) > Hrm. Once upon a time mochitest guaranteed that things passed to > addLoadEvent would all run _before_ the test harness called "finish". Did > that change? I'm not sure about that: current code is there since it was added in bug 357523. (I don't know what the situation was before that.) (In reply to Boris Zbarsky (:bz) from comment #18) > Hmm. So looking at SimpleTest.js, it certainly calls addLoadEvent with a > function that will call finish(). Since things added via addLoadEvent are > executed in order, that would mean that any test using addLoadEvent but not > waitForExplicitFinish() is somewhat broken. I confirm waitForExplicitFinish() is needed when a test wants (to be sure to) to survive (past) its load event. http://mxr.mozilla.org/comm-central/source/mozilla/testing/mochitest/tests/SimpleTest/SimpleTest.js#138 139 if (typeof(addLoadEvent) == 'undefined') { 140 function addLoadEvent(func) { ... http://mxr.mozilla.org/comm-central/source/mozilla/testing/mochitest/tests/SimpleTest/SimpleTest.js#739 740 if (isPrimaryTestWindow) { 741 addLoadEvent(function() { 742 if (SimpleTest._stopOnLoad) { 743 SimpleTest.finish();

Comment on attachment 606612 [details] [diff] [review] Patch v0.75, some tests fixed but some tests will still fail Review of attachment 606612 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +670,5 @@ > if (SimpleTest._expectingUncaughtException) { > SimpleTest.ok(false, "expectUncaughtException was called but no uncaught exception was detected!"); > } > + if (SimpleTest._tests.length == 0) { > + SimpleTest.ok(false, "[SimpleTest.report()] No checks actually run."); While here, can you add an hint like "(Did you miss to add 1+ SimpleTest check and/or need to call waitForExplicitFinish()?)"

(In reply to Serge Gautherie (:sgautherie) from comment #20) > SimpleTest.js should be in its own patch. > Tests should probably be in per-component patches (maybe in blocking bugs), > unless you find someone to do a cross-component review. > It's easier to track (before/after). Will do. > Fwiw, could add a comment that this file is shared with non-chrome tests. Done in next patch version. > Shouldn't you rather add the missing '"'? The type="" attribute on <script> isn't necessary. It's always defaulted to JavaScript in all browsers, and is optional in HTML5. > s/report/finish/ Done in next patch version. (In reply to Serge Gautherie (:sgautherie) from comment #23) > While here, can you add an hint like "(Did you miss to add 1+ SimpleTest > check and/or need to call waitForExplicitFinish()?)" Done in next patch version.

* test_bug357450.xul has a relative path broken by a move in bug 659315. * test_reentrant_flush.html as discussed. Ideally should be fixed in bug 736561, but this workaround should be good enough for now. * test_webgl_crossorigin_textures.html previously ran no tests if WebGL didn't work, which had the effect of todo(false). I added todo(false) instead explicitly, with an informative message. * test_webgl_conformance_test_suite.html used dump() in one code path to mention that it was disabled on old OS X. Again, this had the effect of todo(false), so I did that instead. A different code path a little further down does this too. * test_bug622361.html was only meant to test for a leak, so I just added an ok(true). * focus_frameset.html was included by test_focus_docnav.xul (I finally tracked that down). The test framework doesn't run finish() for iframes or window.open(), but it does run for <browser>. I worked around it with waitForExplicitFinish(), although it's slightly abusive -- we don't ever explicitly finish either. This is really a SimpleTest bug, but I don't understand chrome well enough to say how to fix it. * test_postMessage_hash.html lost a > in bug 544097 and was broken since then. * test_bug632379.xul called waitForFocus() without waitForExplicitFinish() (see bug 736529). Also, I'd like feedback on how to address the dom/tests/mochitest/dom-level* failures at <https://tbpl.mozilla.org/php/getParsedLog.php?id=10127605&tree=Try>. AFAICT, these tests are broken and throw uncaught exceptions before doing anything. Can we just disable them? Or would you prefer an extra todo(false) at the start of each one?

Pretty simple. See bug 616590 comment 17.

* cocoa_focus.html deliberately doesn't run any tests in some cases. In this case I add a todo(false) with an informative message. Let me know if it should be ok(true) instead. * test_pluginstream_seek_close.html was broken by bug 641738, which removed a quotation mark and ate the script.

Bug 513707 removed the waitForExplicitFinish() from this test, which made it not test anything anymore.

This just moves the check to finish(), and makes it ok() instead of todo(). This can't get committed until I've fixed the remaining failures, but I've gotten almost all of them now.

Comment on attachment 606695 [details] [diff] [review] Patch part 1, v1 -- Fix DOM and layout mochitests that no run no tests [Checked in: comment 69] Fix he indent in focus_frameset.html, please. r=me with that. I really appreciated the review notes! For the DOM tests, todo(false) seems fine, plus followup bugs if we think those tests should be doing something....

(In reply to Boris Zbarsky (:bz) from comment #30) > Fix he indent in focus_frameset.html, please. The previous line is indented only because it's in a braceless if, so the indentation here is correct. > For the DOM tests, todo(false) seems fine, plus followup bugs if we think > those tests should be doing something.... I was getting confused. I'll take a closer look on Sunday or Monday and write a separate patch to fix them, if it looks practical, or at least make them fail with a sensible todo. They look very old and crufty, though, and test to ancient standards. Not sure if it's worth keeping them.

Try run for 5e836ef6f8a3 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=5e836ef6f8a3 Results (out of 114 total builds): exception: 3 success: 68 warnings: 43 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-5e836ef6f8a3

They're old and crufty because they're imports of what was at the time the official DOM test suite... Keeping them is worth it; they've caught bugs before.

Comment on attachment 606698 [details] [diff] [review] Patch part 3, v1 -- Fix plugin mochitests that run no tests Review of attachment 606698 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/test/mochitest/cocoa_focus.html @@ +34,5 @@ > NSLeftMouseUp = 2; > > // Don't run any tests if we're not testing the Cocoa event model. > if (plugin1.getEventModel() != 1) { > + todo(false, "Not testing Cocoa event model"); Fwiw, merge the previous comment and this todo() into todo(false, "Skipping this test when not testing the Cocoa event model"); ::: dom/plugins/test/mochitest/test_pluginstream_seek_close.html @@ +4,5 @@ > <script type="text/javascript" > src="/tests/SimpleTest/SimpleTest.js"></script> > <link rel="stylesheet" type="text/css" > href="/tests/SimpleTest/test.css" /> > +<script> (In reply to Aryeh Gregor from comment #24) > The type="" attribute on <script> isn't necessary. It's always defaulted to > JavaScript in all browsers, and is optional in HTML5. Agreed, yet it (is already there and) doesn't hurt. We usually have it. (Especially in head, just like the SimpleTest.js line.) Nonetheless, if you do want to remove it, so be it.

Comment on attachment 606695 [details] [diff] [review] Patch part 1, v1 -- Fix DOM and layout mochitests that no run no tests [Checked in: comment 69] Review of attachment 606695 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/test/chrome/test_bug357450.xul @@ +12,5 @@ > > <title>Test for Bug 357450</title> > <script type="application/javascript" > src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script> > + <!-- This file is shared with non-chrome tests --> Nit: <!-- test_bug357450.js is shared with related non-chrome tests. --> ::: content/base/test/test_reentrant_flush.html @@ +21,5 @@ > <pre id="test"> > <script type="application/javascript"> > > /** Test for Bug 709256 **/ > +SimpleTest.waitForExplicitFinish(); Nit: Add a blank line before and after. ::: content/canvas/test/crossorigin/test_webgl_crossorigin_textures.html @@ +78,5 @@ > var canvas = document.getElementById("canvas"); > try { > gl = canvas.getContext("experimental-webgl"); > } catch (e) { > + todo(false, "Canvas WebGL not supported"); Nit: While here, I guess we should rewrite this try+catch to be similar to http://mxr.mozilla.org/mozilla-central/source/content/canvas/test/webgl/test_webgl_conformance_test_suite.html?force=1 At least, use the same message: ""Can't create a WebGL context"" ::: content/canvas/test/webgl/test_webgl_conformance_test_suite.html @@ +87,5 @@ > // Mac OS 10.5 would be Darwin version 9. the |version| string we've got here > // is the Darwin version. > is106orHigher = (kDarwinVersion >= 10.0); > if (!is106orHigher) { > + todo(false, "WebGL mochitest disabled on Mac OSX versions older than 10.6."); WHAT? OH! I have now filed bug 736696. Leave this broken file alone ;-< ::: dom/tests/mochitest/chrome/focus_frameset.html @@ +6,5 @@ > if (opener) > SimpleTest.waitForFocus(function () opener.framesetWindowLoaded(window)); > +// Don't try to call finish() in this frame, since it will get upset that we > +// ran no tests. > +SimpleTest.waitForExplicitFinish(); That's plain wrong, sorry. This file is used by 2 tests: http://mxr.mozilla.org/mozilla-central/search?string=focus_frameset.html&case=1&find=%2Fdom%2Ftests%2Fmochitest%2F.*\.xul window_focus.xul is fine. window_focus_docnav.xul is broken: 4 <window onload="start()" ... 94 function start() 95 { 96 window.opener.wrappedJSObject.SimpleTest.waitForExplicitFinish(); It's called too late.

(In reply to Aryeh Gregor from comment #11) > /tests/dom/tests/mochitest/dom-level*: TODO: I looked at the first few > failures, and they're all the same. They look old and very broken. > DOMTestCase.js overwrites SimpleTest._logResult, and the overwritten version > throws an exception at line 653 (parentRunner is null). The failing tests > all hang for me on localhost; I'm not sure why they don't time out on the > try server. I don't know why they work without this patch being applied. Running these tests has/had various issues. Move this issue to a blocking bug. > chrome://mochitests/content/a11y/accessible/test_nsIAccessibleDocument.html: > TODO: The test doesn't seem to work for me at all when I run it on localhost > (it hangs). The doTest() function seems to never fire. I can't get the > file to do anything at all: adding ok(false) early on doesn't do anything. > This was added by bug 441737. This a11y test is broken: getRootDirectory() is defined in chrome-harness.js, but including the latter just silently breaks somehow. Move this issue to a blocking bug.

Comment on attachment 606698 [details] [diff] [review] Patch part 3, v1 -- Fix plugin mochitests that run no tests Review of attachment 606698 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/test/mochitest/test_pluginstream_seek_close.html @@ +4,5 @@ > <script type="text/javascript" > src="/tests/SimpleTest/SimpleTest.js"></script> > <link rel="stylesheet" type="text/css" > href="/tests/SimpleTest/test.css" /> > +<script> It's unnecessary verbiage and that does hurt a little bit.

(In reply to Boris Zbarsky (:bz) from comment #33) > They're old and crufty because they're imports of what was at the time the > official DOM test suite... > > Keeping them is worth it; they've caught bugs before. Okay. (In reply to Serge Gautherie (:sgautherie) from comment #35) > Nit: > <!-- test_bug357450.js is shared with related non-chrome tests. --> >... > Nit: > Add a blank line before and after. Okay. > Nit: > > While here, I guess we should rewrite this try+catch to be similar to > http://mxr.mozilla.org/mozilla-central/source/content/canvas/test/webgl/ > test_webgl_conformance_test_suite.html?force=1 > > At least, use the same message: > ""Can't create a WebGL context"" I'm just trying to make sure there are no new failures here, not fix tests that are otherwise totally broken. > WHAT? OH! I have now filed bug 736696. > Leave this broken file alone ;-< I'll leave in this fix so that it doesn't cause new failures on landing the last patch in this series. If you want to fix the file beyond that, be my guest. > That's plain wrong, sorry. > > This file is used by 2 tests: > http://mxr.mozilla.org/mozilla-central/search?string=focus_frameset. > html&case=1&find=%2Fdom%2Ftests%2Fmochitest%2F.*\.xul > > window_focus.xul is fine. > > window_focus_docnav.xul is broken: > 4 <window onload="start()" > ... > 94 function start() > 95 { > 96 window.opener.wrappedJSObject.SimpleTest.waitForExplicitFinish(); > > It's called too late. I believe you're mistaken. To the contrary, that line in window_focus_docnav.xul is unnecessary. It's calling waitForExplicitFinish() on the SimpleTest object of its opener, but its opener is test_focus_docnav.xul, which already called waitForExplicitFinish(). The line you point to here could just be removed; it does nothing. The problem is that focus_frameset.html is including SimpleTest.js, and the SimpleTest code running in focus_frameset.html schedules a call to finish() onload. This call to finish() will be on the SimpleTest object in focus_frameset.html, which never had any tests run on it. This leads to a weird output line that's not numbered, like 0 INFO SimpleTest START 1 INFO TEST-START | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_focus_docnav.xul TEST-UNEXPECTED-FAIL | unknown test url | [SimpleTest.finish()] No checks actually run. 2 INFO TEST-INFO | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_focus_docnav.xul | must wait for focus with "unknown test url", because the test in question never started or finished and isn't hooked up to a test runner. This is all because the isPrimaryTestWindow definition in SimpleTest.js doesn't seem to work correctly when the current page is included by <browser>. In particular, trying the fix you suggest leaves the TEST-UNEXPECTED-FAIL line, while my fix removes the line. (In reply to Serge Gautherie (:sgautherie) from comment #36) > Running these tests has/had various issues. > Move this issue to a blocking bug. Filed bug 735884. > This a11y test is broken: getRootDirectory() is defined in > chrome-harness.js, but including the latter just silently breaks somehow. > Move this issue to a blocking bug. Filed bug 736886.

This is exactly the same as the last version of the patch, except that I added todo()s for the DOM tests that are doing nothing, and I incorporated aesthetic feedback from Serge in comment 35 (changes to comments, whitespace, and failure messages).

As it stands, this test runs no actual checks. Some kind of error in chrome-harness.js makes it abort. The patch comments out the relevant parts, which means it's at least running some tests rather than none. For further work, I've filed bug 736886.

Autoland Patchset: Patches: 606696, 606698, 606703, 606704, 607005, 607010 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=328e8198d6fe Try run started, revision 328e8198d6fe. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=328e8198d6fe

Comment on attachment 607010 [details] [diff] [review] Patch part 4.5, v1 -- Fix a11y mochitest that runs no tests Review of attachment 607010 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/tests/mochitest/test_nsIAccessibleDocument.html @@ +14,5 @@ > src="role.js"></script> > <script type="application/javascript" > src="states.js"></script> > > + <!-- Commented out due to bug 736886 --> That patch should be attached to that bug. <!-- chrome-harness.js silently breaks this test. (Bug 736886) --> @@ +43,5 @@ > is(attributes.getStringProperty("tag"), "BODY", > "Wrong attribute on document!"); > > // nsIAccessibleDocument > + // getRootDirectory() doesn't work, bug 736886 // getRootDirectory() depends on chrome-harness.js include. (Bug 736886) @@ +46,5 @@ > // nsIAccessibleDocument > + // getRootDirectory() doesn't work, bug 736886 > + //var rootDir = getRootDirectory(window.location.href); > + //is(docAcc.URL, rootDir.path + "test_nsIAccessibleDocument.html", > + // "Wrong URL for document!"); Use '/* ... */': easier to use and preserves Hg history.

Comment on attachment 607005 [details] [diff] [review] Patch part 1, v2 -- Fix DOM and layout mochitests that no run no tests I'm a bit confused by the DOM test changes. The function passed to addLoadEvent will run after finish(), right? How does it help?

Try run for 328e8198d6fe is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=328e8198d6fe Results (out of 116 total builds): success: 99 warnings: 17 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-328e8198d6fe

Cute that adding the waitForExplicitFinish in test_bug632379.xul makes bug 668716 permanent instead of intermittent - I assume it's been broken since it was moved to mochitest-chrome, but most of the time it avoids running and thus avoids failing? All of the "green" logs I've looked at show exactly that, though I suppose it's possible that it also sometimes runs and passes.

Aryeh, I suggest "you" land part 2 and 4 (for which you have r+ and f+), so they get out of the way (and also make it easier to track in case they fail or reveal failures in other tests: the less changes per patch the better wrt this bug). Wrt part 1, the dom-level tests should be attached to bug 483992 instead. And I would suggest to split your previous patch into one that is agreed on (to land soon) and one with the tests we haven't agreed on yet (I'll look again at them soon).

(In reply to Serge Gautherie (:sgautherie) from comment #42) > That patch should be attached to that bug. I filed that bug as a followup to fix the bug properly. The patch attached here just does a quick workaround. > <!-- chrome-harness.js silently breaks this test. (Bug 736886) --> It's not silent with this bug's patches applied. :) But I'll change the comment to something similar in the next version. > // getRootDirectory() depends on chrome-harness.js include. (Bug 736886) Okay. > Use '/* ... */': easier to use and preserves Hg history. Okay. (In reply to Boris Zbarsky (:bz) from comment #43) > I'm a bit confused by the DOM test changes. The function passed to > addLoadEvent will run after finish(), right? How does it help? Well, it does: """ 1779 INFO TEST-START | /tests/dom/tests/mochitest/dom-level1-core/test_elementreplaceattributewithself.html 1780 INFO TEST-KNOWN-FAIL | No tests run (bug 735884) | didn't expect 0, but got it 1781 INFO TEST-END | /tests/dom/tests/mochitest/dom-level1-core/test_elementreplaceattributewithself.html | finished in 90ms """ <https://tbpl.mozilla.org/php/getParsedLog.php?id=10168212&tree=Try&full=1> DOMTestCase.js monkeys around with SimpleTest. I'm guessing that it calls SimpleTest.waitForExplicitFinish() or something equivalent. I was going to add waitForExplicitFinish() and finish() myself, but it turned out not to be necessary, so I didn't. Ideally I'd like to run the check all the way at the end, when finish() is really called, so that if tests actually get run before then we'll get an unexpected pass. But this way of fixing the failures works for now. If you have a better way, please suggest it and I'll do it. (In reply to Phil Ringnalda (:philor) from comment #45) > Cute that adding the waitForExplicitFinish in test_bug632379.xul makes bug > 668716 permanent instead of intermittent - I assume it's been broken since > it was moved to mochitest-chrome, but most of the time it avoids running and > thus avoids failing? All of the "green" logs I've looked at show exactly > that, though I suppose it's possible that it also sometimes runs and passes. Should I disable it entirely instead of fixing it, then? (In reply to Serge Gautherie (:sgautherie) from comment #46) > Aryeh, > > I suggest "you" land part 2 and 4 (for which you have r+ and f+), so they > get out of the way (and also make it easier to track in case they fail or > reveal failures in other tests: the less changes per patch the better wrt > this bug). > > Wrt part 1, the dom-level tests should be attached to bug 483992 instead. > And I would suggest to split your previous patch into one that is agreed on > (to land soon) and one with the tests we haven't agreed on yet (I'll look > again at them soon). I'd prefer to get part 5 landed ASAP, because every day it sits here is a day someone might push a bad test that will break as soon as this lands. That means all the other parts need to be landed ASAP to avoid test failures. Anything that shuts up the failing tests and doesn't make them test anything less than they already are is fair game for this bug, IMO. Other bugs can be used to fix them properly. Improvements to test infrastructure shouldn't be blocked by having to properly fix every broken test that the improvement exposes. Otherwise people improving test infrastructure get unrelated work thrown at them. And the more they improve it, the more bad tests they find, and the more unrelated work they have to do!

Same patch as before, with feedback from Serge in comment 42 (wording and comment style). Note: I have new versions of parts 3 and 4 in the works to fix further test failures. I'll post them for review once I get a try run with no new test failures, just to avoid re-requesting review excessively. (Maybe I should have gotten a successful try run before requesting review to start with in this case.)

Try run for 7b0945525b4b is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=7b0945525b4b Results (out of 1 total builds): failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-7b0945525b4b

Try run for 9a7eb4daed77 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9a7eb4daed77 Results (out of 1 total builds): exception: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-9a7eb4daed77

Try run for 50924f0413ab is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=50924f0413ab Results (out of 218 total builds): exception: 1 success: 173 warnings: 29 failure: 14 other: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-50924f0413ab

(In reply to Serge Gautherie (:sgautherie) from comment #46) > I suggest "you" land part 2 and 4 Please do, unless they trigger/reveal other failures. (In reply to Aryeh Gregor from comment #48) > Note: I have new versions of parts 3 and 4 in the works to fix further test > failures. Part 3: agreed. Part 4: please don't, unless there is an issue with it. Add a separate patch for other issues.

Comment on attachment 606698 [details] [diff] [review] Patch part 3, v1 -- Fix plugin mochitests that run no tests r- wrt { 8942 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/test_cocoa_focus.html | an unexpected uncaught JS exception reported through window.onerror - todo is not defined at http://mochi.test:8888/tests/dom/plugins/test/cocoa_focus.html:38 }

Comment on attachment 607177 [details] [diff] [review] Patch part 4.5, v2 -- Fix a11y mochitest that runs no tests Review of attachment 607177 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/tests/mochitest/test_nsIAccessibleDocument.html @@ +50,4 @@ > var rootDir = getRootDirectory(window.location.href); > is(docAcc.URL, rootDir.path + "test_nsIAccessibleDocument.html", > "Wrong URL for document!"); > + */ please add todo

Comment on attachment 607177 [details] [diff] [review] Patch part 4.5, v2 -- Fix a11y mochitest that runs no tests Review of attachment 607177 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Aryeh Gregor from comment #47) > I filed that bug as a followup to fix the bug properly. The patch attached In that case, that bug should depend on, not block this bug. > here just does a quick workaround. In this case, as only one check of this test is disabled, and though I still think the patch should be better in that bug, I'll admit it's acceptable to do it here :-| > It's not silent with this bug's patches applied. Silent means that the test "itself" doesn't report any kind of "error": it just "dies". The future harness check count checking won't change that. ::: accessible/tests/mochitest/test_nsIAccessibleDocument.html @@ +14,5 @@ > src="role.js"></script> > <script type="application/javascript" > src="states.js"></script> > > + <!-- chrome-harness.js breaks this test (bug 736886) --> Please, add a '.' and use a 'B'. I agree "silently" is optional, if you don't want to add it. @@ +44,5 @@ > "Wrong attribute on document!"); > > // nsIAccessibleDocument > + // getRootDirectory() depends on broken chrome-harness.js include (bug > + // 736886) Same nit as the other comment. It's acceptable to have this on one line only... @@ +45,5 @@ > > // nsIAccessibleDocument > + // getRootDirectory() depends on broken chrome-harness.js include (bug > + // 736886) > + /* Move it before the added comment.

Try run for 124f225b25a1 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=124f225b25a1 Results (out of 232 total builds): exception: 2 success: 186 warnings: 28 failure: 16 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-124f225b25a1

(In reply to Boris Zbarsky (:bz) from comment #19) > Ok, so here's the list of files in my tree that have calls to addLoadEvent > but not calls to waitForExplicitFinish, if I did my greps right: Should do the same survey for "onload=..." :-<

Comment on attachment 607005 [details] [diff] [review] Patch part 1, v2 -- Fix DOM and layout mochitests that no run no tests Review of attachment 607005 [details] [diff] [review]: ----------------------------------------------------------------- I still think you should leave these DOM tests alone (or at the very least separate), but at least take the time to add right workarounds, not misleading ones. ::: dom/tests/mochitest/dom-level1-core/test_elementreplaceattributewithself.html @@ +9,5 @@ > <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > <script type="text/javascript" src="DOMTestCase.js"></script> > <script type="text/javascript" src="exclusions.js"></script> > <script type="text/javascript"> > +addLoadEvent(function() { This is misplaced: 68 SimpleTest.finish(); ... 77 SimpleTest.waitForExplicitFinish(); 78 addLoadEvent(setUpPage); @@ +10,5 @@ > <script type="text/javascript" src="DOMTestCase.js"></script> > <script type="text/javascript" src="exclusions.js"></script> > <script type="text/javascript"> > +addLoadEvent(function() { > + todo_isnot(SimpleTest._tests.length, 0, "No tests run (bug 735884)"); This is "wrong": this test runs just fine! You should use |ok(true,"complete"...|.

Comment on attachment 607463 [details] [diff] [review] (AAv1) test_bug518777.html misses waitForExplicitFinish() + finish() [Checked in: Comment 59] https://hg.mozilla.org/mozilla-central/rev/65578021a495 [Approval Request Comment] Regression caused by (bug #): Bug 518777. User impact if declined: None, but this would help to prevent random-oranges. Testing completed (on m-c, etc.): This comment. Risk to taking this patch (and alternatives if risky): None, test-only. String changes made by this patch: None. Actually, I would even like blanket-approvals for the (upcoming) other patches in this bug.

(In reply to Serge Gautherie (:sgautherie) from comment #52) > (In reply to Serge Gautherie (:sgautherie) from comment #46) > > I suggest "you" land part 2 and 4 > > Please do, unless they trigger/reveal other failures. I will as soon as I get my L3 commit access (bug 737033). > Part 3: agreed. > Part 4: please don't, unless there is an issue with it. Add a separate patch > for other issues. Okay, that's reasonable. (In reply to Serge Gautherie (:sgautherie) from comment #53) > Comment on attachment 606698 [details] [diff] [review] > Patch part 3, v1 -- Fix plugin mochitests that run no tests > > r- wrt > { > 8942 ERROR TEST-UNEXPECTED-FAIL | > /tests/dom/plugins/test/test_cocoa_focus.html | an unexpected uncaught JS > exception reported through window.onerror - todo is not defined at > http://mochi.test:8888/tests/dom/plugins/test/cocoa_focus.html:38 > } New version fixes that, about to be posted. (In reply to alexander :surkov from comment #54) > Comment on attachment 607177 [details] [diff] [review] > Patch part 4.5, v2 -- Fix a11y mochitest that runs no tests > > Review of attachment 607177 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/tests/mochitest/test_nsIAccessibleDocument.html > @@ +50,4 @@ > > var rootDir = getRootDirectory(window.location.href); > > is(docAcc.URL, rootDir.path + "test_nsIAccessibleDocument.html", > > "Wrong URL for document!"); > > + */ > > please add todo Will post a new version in a minute. (In reply to Serge Gautherie (:sgautherie) from comment #55) > (In reply to Aryeh Gregor from comment #47) > > > I filed that bug as a followup to fix the bug properly. The patch attached > > In that case, that bug should depend on, not block this bug. That's the opposite of what I was told in #developers. [120318 16:21:06] <AryehGregor> Okay, I always get confused: a followup bug should block the bug it follows up on, or depend on? [120318 16:21:18] <Ms2ger> Block the old bug [120318 16:21:23] <AryehGregor> I thought so. [120318 16:21:30] <Ms2ger> There's no technical reason, just convention [120318 16:22:05] <philor> "This being unfixed blocks the original bug from being completely correctly fixed" is how I remember which way to go [120318 16:22:38] <Ms2ger> philor, ... that actually makes some sense [120318 16:22:56] <philor> every now and then I do, though I try not to make a habit of it [120318 16:23:48] <Ms2ger> Always so terribly nice about yourself > Please, add a '.' and use a 'B'. > I agree "silently" is optional, if you don't want to add it. > . . . > Same nit as the other comment. > It's acceptable to have this on one line only... > . . . > Move it before the added comment. Personally I prefer the way I wrote it. I asked Alexander for review, so since he gave review without requesting these changes, I'll leave it the way it is. (In reply to Serge Gautherie (:sgautherie) from comment #58) > This is misplaced: > 68 SimpleTest.finish(); > ... > 77 SimpleTest.waitForExplicitFinish(); > 78 addLoadEvent(setUpPage); How is it misplaced? > > +addLoadEvent(function() { > > + todo_isnot(SimpleTest._tests.length, 0, "No tests run (bug 735884)"); > > This is "wrong": this test runs just fine! > You should use |ok(true,"complete"...|. How does it "run fine"? As far as I can tell, it has a bunch of ok()'s and similar that are supposed to run, like assertSame("replacedAttr",streetAttr,replacedAttr); but they don't run because something in DOMTestCase.js throws an exception. That seems like it should be a todo, not ok.

The previous patch version was wrong. First of all, todo() wasn't defined in that scope, it should have been opener.todo(). Second of all, it missed a file -- the same fix for cocoa_focus.html needed to be made in cocoa_window_focus.html. These are the only substantive changes relative to the last patch version. (I also incorporated an aesthetic change suggested by Serge in comment 34.) This patch should be the last version -- there are no more failures in the last try run <https://tbpl.mozilla.org/?tree=Try&rev=124f225b25a1>, except for the test_bug518777.html one that was fixed in the last patch attached to this bug.

With todo() as requested in comment 54.

Sorry for having to ask for review for more tests instead of just submitting one patch to start with. This should be the last of the toolkit tests that this bug breaks, since the last try run <https://tbpl.mozilla.org/?tree=Try&rev=124f225b25a1> was almost completely green. These two tests both test Windows 7-specific functionality, so they should pass automatically on other platforms (not todo). Alternatively, should I try to modify the makefile so they don't run at all on Windows < 7? Currently they run on all Windows but exit right away on pre-7.

(In reply to Aryeh Gregor from comment #60) > That's the opposite of what I was told in #developers. I don't think so. > [120318 16:22:05] <philor> "This being unfixed blocks the original bug from > being completely correctly fixed" is how I remember which way to go I agree with that. But these follow-ups don't block what you will have already worked around (= adding a check) here: the bugs they block are the ones that added the "broken" code in the first place. I think this bug should have been a meta and those bugs block this one, but that's not at all how you chose to do it. > How is it misplaced? 1) Put the addLoadEvent() together. 2) Using addLoadEvent() is not even needed. 3) The workaround (message) is not even right. 4) See bug 483992 for saner fixes. > How does it "run fine"? As far as I can tell, it has a bunch of ok()'s and > similar that are supposed to run, like > > assertSame("replacedAttr",streetAttr,replacedAttr); All checks succeed with the build I test with locally... > but they don't run because something in DOMTestCase.js throws an exception. That exception happens when running standalone only, so unrelated to tinderboxes. And the correct solution for that other issue is to fix that code.

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e35e1b1247 https://hg.mozilla.org/integration/mozilla-inbound/rev/69b504f1edaa https://hg.mozilla.org/integration/mozilla-inbound/rev/565359dfe020 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c19eb7dac4a

https://hg.mozilla.org/mozilla-central/rev/12e35e1b1247 https://hg.mozilla.org/mozilla-central/rev/69b504f1edaa https://hg.mozilla.org/mozilla-central/rev/565359dfe020 https://hg.mozilla.org/mozilla-central/rev/1c19eb7dac4a

Sorry, should have noted in the whiteboard to not close this yet.

(In reply to Serge Gautherie (:sgautherie) from comment #64) > . . . > That exception happens when running standalone only, so unrelated to > tinderboxes. > And the correct solution for that other issue is to fix that code. I'm fine with that, as long as the failures are fixed. I was just adding todo() because I wasn't sure why they were failing and didn't think diagnosing should block this bug's patch from landing. Thanks for fixing it properly -- I'll ditch the latest version of my patch part 1 and go back to the one bz already r+'d.

Part 1: http://hg.mozilla.org/integration/mozilla-inbound/rev/c56fbf96661b Part 6: http://hg.mozilla.org/integration/mozilla-inbound/rev/25a2a013eea1

https://hg.mozilla.org/mozilla-central/rev/c56fbf96661b https://hg.mozilla.org/mozilla-central/rev/25a2a013eea1

Autoland Patchset: Patches: 606704 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=9a29b6e74b12 Try run started, revision 9a29b6e74b12. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=9a29b6e74b12

Comment on attachment 606704 [details] [diff] [review] Patch part 5, v1 -- Mochitests that run no tests should fail Review of attachment 606704 [details] [diff] [review]: ----------------------------------------------------------------- this looks good!

Try run for 9a29b6e74b12 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9a29b6e74b12 Results (out of 177 total builds): success: 166 warnings: 10 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-9a29b6e74b12

Note: when merging with the patch from bug 736529 (which also adds a check to SimpleTest.finish()), I put the waitForFocus() check before the check from this bug. I guess it makes no difference, but if both checks fail, the waitForFocus() check is more specific and it's probably best that it be reported first. https://hg.mozilla.org/integration/mozilla-inbound/rev/93fc7bf5e5be

Comment on attachment 607463 [details] [diff] [review] (AAv1) test_bug518777.html misses waitForExplicitFinish() + finish() [Checked in: Comment 59] [Triage Comment] Let's move forward with this on Aurora 13 first and then consider uplifting to Beta 12 after a couple of days without any regressions. Please re-nominate at that time.

(the above includes blanket approval for the other test fixes for Aurora 13)

Comment on attachment 607463 [details] [diff] [review] (AAv1) test_bug518777.html misses waitForExplicitFinish() + finish() [Checked in: Comment 59] (In reply to Jens Hatlak (:InvisibleSmiley) from bug 737454 comment #19) > http://hg.mozilla.org/releases/mozilla-aurora/rev/6c2316525c7c Merged with follow-up patch there.