Closed Bug 404077 Opened 17 years ago Closed 11 years ago

Make assertions cause test failures in Mochitest

Categories

(Testing :: Mochitest, defect)

Product:
Testing
Component:
Mochitest
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: Waldo, Assigned: dbaron)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(7 files, 7 obsolete files)

Windows Mochitest run with stack-and-abort behavior on asserts
(deleted), application/zip
Details
assertion checking (initially not making tests fail)
(deleted), patch
ted
: review+
sgautherie
: feedback-
Details | Diff | Splinter Review
download-logs.py
(deleted), text/plain
Details
Do GC in a small number of tests to reduce the spread of GC-related assertions.
(deleted), patch
jruderman
: review+
Details | Diff | Splinter Review
make-assertlists.sh
(deleted), text/plain
Details
unify-assertlists.sh
(deleted), text/plain
Details
make-asserttexts.
(deleted), text/plain
Details 
I can't find a bug already filed for this, just occasional mentions of it in bugmail, and we won't have a good idea of the scope of the problem or what needs to be fixed without having a bug.

We hit about 1100 assertions when I ran the tests on Windows earlier today, with about 30-odd distinct assertions by source location.  I'll file the necessary bugs and set the necessary dependencies when I have time, probably later today.
yeah, I just did a hunt through my bugmail and can't find anything specific on it, even though I know I've heard this requested before.

Coop's currently working on getting leak-enabled debug builds running mochitest and reftest. We can use those machines to fail on assertion as well, if that makes sense.
Blocks: 393112
Depends on: 397724
Attachment #289054 - Attachment is patch: false
Attachment #289054 - Attachment mime type: text/plain → application/zip
Depends on: 404017
Depends on: 353408
Depends on: 386939
Depends on: 397561
Depends on: 404115
Depends on: 393918
Depends on: 404117
Depends on: 404120
Depends on: 404121
Depends on: 321058
Depends on: 399554
Blocks: 279923
Mass move of the rest of the Mochitest bugs from Core:Testing to Testing:Mochitest. Filter on MochitestMassMove to ignore.
Component: Testing → Mochitest
Product: Core → Testing
QA Contact: testing → mochitest
dbaron has implemented the equivalent for reftest in bug 472557. We could use his new API to do this for Mochitest. We would need a way to annotate tests with the expected assertion count. Ideally we'd have a test manifest like reftest, but that's not quite simple. A simpler way might be to add a SimpleTest API that you can call like expectAssertions(5); (or something like that) to set the expected assertion count for your test, and having it default to zero, I guess.
Depends on: 472557
The tricky part with mochitest will be dealing with assertions that happen while unloading the test.  We might end up having to make the assertion checks work only when running the tests inside the harness and not happen while running a standalone test file.

A potential bigger problem is tests that trigger assertions later, such as during GC; perhaps we can just assume that we won't have many of either type, and we'll try to fix all of that category before enabling the assertion checking.

We've also yet to see how hard the reftest strategy will be to actually get working.  We might end up with a good bit of pain with variable numbers of assertions.  We're probably better off starting with some tests and getting some of the random/variable assertions fixed before moving on to all the tests.
Depends on: 459608
We should be able to revisit this now that we have debug tinderboxes. I know dbaron has been cataloging the top assertions that we hit.
Depends on: 571613
No longer depends on: 353408, 404120
Currently we have (on Linux debug):
  480 assertions in mochitest-plain-* (33 unique messages)
  142 assertions in mochitest-chrome (12 unique messages)
  485 assertions in mochitest-browser-chrome (19 unique messages)
 6293 assertions in mochitest-a11y (15 unique messages)
There are only 67 unique assertions that fire across all tests.
Depends on: 439258
Depends on: 614320
Depends on: 614326
Depends on: 614335
Depends on: 614348
Depends on: 614359
Depends on: 510489
Depends on: 614369
Depends on: 614390
Depends on: 614626
Depends on: 614627
Depends on: 614630
Depends on: 614635
Depends on: 614639
Depends on: 614993
Depends on: 614995
Depends on: 614474
Depends on: 374680
This seems to behave as expected (see patch description and in-code documentation).  I'll give it a push-to-try to make sure.
Can we do this without using enablePrivilege? We're trying to remove that, and replace it with the specialpowers extension bits:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/specialpowers/

I'd be happy to hack whatever you need to expose into SpecialPowers, I've been working on replacing our other enablePrivilege usage in the Mochitest harness.
It turns out there are various issues with a few tests, both with that patch, and with a different patch that avoided some of the issues (such as problems with some audio tests that call finish() multiple times).

I ended up filing bug 631289 on one of the problems that happened with the second variant of the patch; we'll see what happens there, but I may just give up on trying to correctly attribute assertions that happen during unloading of the page.  (After all, that still wouldn't handle assertions that happen during GC, etc.)
(In reply to comment #9)
> Can we do this without using enablePrivilege? We're trying to remove that, and
> replace it with the specialpowers extension bits:
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/specialpowers/
> 
> I'd be happy to hack whatever you need to expose into SpecialPowers, I've been
> working on replacing our other enablePrivilege usage in the Mochitest harness.

All it needs is the isDebugBuild and assertionCount attributes from nsIDebug2.
Depends on: 637097
Depends on: 378012
Depends on: 631289
Depends on: 671635
I've had a work-in-progress patch for this for ages, currently:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/fdf7f5b496e4/mochitest-assertion-checking
(see log at
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/log/tip/mochitest-assertion-checking )

I just haven't had the chance to debug the failures that just the patch (never mind the assertion checking) causes yet; if my memory is correct they're currently in mochitest-a11y (though they used to be elsewhere).
And if I wait a few more months, again, the old failures go away and I get a new set of failures:
https://tbpl.mozilla.org/?tree=Try&rev=969ff99bae00
You're not waiting until we don't suck, are you? Them's not failures, them's just the way we is.
I did rerun all the oranges to see if they were intermittent.  Looks like some of them were (despite not matching the filed bugs).
(And, for the record, I did the same the previous times and the failures were not intermittent, and were rather more novel.)
OK, now that this seems to be green based on the failures going away over the last 10 months, I suppose it's time to try getting it in (before new failures are introduced).

See description in commit message.

I also started a more thorough (than the one in comment 13) try run: https://tbpl.mozilla.org/?tree=Try&rev=6482436439c9

(I'm not sure if the code to handle tests that call finish() more than once is actually still needed; I think the ones I knew about were fixed a while back.)
Attachment #509304 - Attachment is obsolete: true
Attachment #584885 - Flags: review?(ted.mielczarek)
Comment on attachment 584885 [details] [diff] [review]
assertion checking (initially not making tests fail)

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

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +89,5 @@
> +  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +  var DEBUG_CONTRACTID = "@mozilla.org/xpcom/debug;1";
> +  var CC = Components.classes;
> +  var CI = Components.interfaces;
> +  var CR = Components.results;

Nit: CR is unused, is it?

@@ +372,5 @@
> +    // chance to unload it.  At some point in the future we should make
> +    // this an error.
> +    if (TestRunner._currentTest == TestRunner._lastTestFinished) {
> +        // 
> +        TestRunner.logger.log("TEST-INFO | " +

Nit: unable it and make it TEST-KNOWN-FAIL (or TEST-DETCEPXENU-FAIL?) ftb.

@@ +414,5 @@
> +        var iframe = $('testframe');
> +        // For some reason, if we use an onload handler rather than an
> +        // inline script, then tests that call finish() multiple times cause
> +        // the onload to fail to fire in some cases (e.g., test_audio1 and
> +        // test_audio2, in succession, which both call finish three times).

Nit: is that example still true? (Is there a bug filed?)

@@ +445,5 @@
> +            }
> +            if (numAsserts < min) {
> +                // WHEN ENABLING, change "log" to "error" and "DETCEPXENU"
> +                // to "UNEXPECTED".
> +                TestRunner.logger.log("TEST-DETCEPXENU-PASS | " + url + " | Assertion count " + numAsserts + " is less than expected range " + min + "-" + max + " assertions.");

Logic should be:
if (> max)
else if (< min)
else if (> 0)
Attachment #584885 - Flags: feedback-
(In reply to Serge Gautherie (:sgautherie) from comment #18)
> > +        // 
> > +        TestRunner.logger.log("TEST-INFO | " +
> 
> Nit: unable it and make it TEST-KNOWN-FAIL (or TEST-DETCEPXENU-FAIL?) ftb.

s/unable/enable/ ;->
Comment on attachment 584885 [details] [diff] [review]
assertion checking (initially not making tests fail)

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

Overall this looks fine, but I don't want to add more enablePrivilege here.

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +85,5 @@
>  
>  TestRunner._expectingProcessCrash = false;
>  
> +try {
> +  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

Please add an API on SpecialPowers instead. We've been trying very hard to get rid of enablePrivilege in the harness, I'd rather not accumulate more uses of it.
Attachment #584885 - Flags: review?(ted.mielczarek) → review-
Depends on: 734249
Depends on: 734553
No longer depends on: 614320
Assignee: nobody → dbaron

        Attachment #584885 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

        Attachment #610959 -
        Flags: review?(ted.mielczarek)

    
    

    
  
Comment on attachment 610959 [details] [diff] [review]
assertion checking (initially not making tests fail)

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

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +397,5 @@
> +    // Sometimes a test calls finish() multiple times before we have a
> +    // chance to unload it.  At some point in the future we should make
> +    // this an error.
> +    if (TestRunner._currentTest == TestRunner._lastTestFinished) {
> +        TestRunner.error("TEST-UNEXPECTED-FAIL | " +

Either you meant log(TEST-KNOWN-*) (or TEST-DETCEPXENU-*?),
or you should update the previous comment too.

@@ +447,5 @@
> +        var iframe = $('testframe');
> +        // For some reason, if we use an onload handler rather than an
> +        // inline script, then tests that call finish() multiple times cause
> +        // the onload to fail to fire in some cases (e.g., test_audio1 and
> +        // test_audio2, in succession, which both call finish three times).

Nit: is that example still true? (Is there a bug filed?)

@@ +469,5 @@
> +        var min = TestRunner._expectedMinAsserts;
> +        if (numAsserts > max) {
> +            // WHEN ENABLING, change "log" to "error" and "DETCEPXENU"
> +            // to "UNEXPECTED".
> +            TestRunner.log("TEST-DETCEPXENU-FAIL | " + url + " | Assertion count " + numAsserts + " is greater than expected range " + min + "-" + max + " assertions.");

Why not use TEST-KNOWN-* instead of TEST-DETCEPXENU-*?
You could add a unique string to the description if need be.
(everywhere)

@@ +471,5 @@
> +            // WHEN ENABLING, change "log" to "error" and "DETCEPXENU"
> +            // to "UNEXPECTED".
> +            TestRunner.log("TEST-DETCEPXENU-FAIL | " + url + " | Assertion count " + numAsserts + " is greater than expected range " + min + "-" + max + " assertions.");
> +        } else if (numAsserts > 0) {
> +            TestRunner.log("TEST-KNOWN-FAIL | " + url + " | Assertion count " + numAsserts + " within expected range " + min + "-" + max + " assertions.");

Logic should be:
if (> max)
else if (< min)
else if (> 0)

Or that log is potentially wrong.

@@ +473,5 @@
> +            TestRunner.log("TEST-DETCEPXENU-FAIL | " + url + " | Assertion count " + numAsserts + " is greater than expected range " + min + "-" + max + " assertions.");
> +        } else if (numAsserts > 0) {
> +            TestRunner.log("TEST-KNOWN-FAIL | " + url + " | Assertion count " + numAsserts + " within expected range " + min + "-" + max + " assertions.");
> +        }
> +        // Separate from the if-else chain so we always fail when min > max.

That erroneous case should be handled in expectAssertions(), not here.

        Attachment #610959 -
        Flags: feedback-

    
    

    
  
(In reply to Serge Gautherie (:sgautherie) from comment #22)
> Either you meant log(TEST-KNOWN-*) (or TEST-DETCEPXENU-*?),
> or you should update the previous comment too.

I've fixed the comment.

> Nit: is that example still true? (Is there a bug filed?)

I'll fix the comment.

> Why not use TEST-KNOWN-* instead of TEST-DETCEPXENU-*?
> You could add a unique string to the description if need be.
> (everywhere)

This technique works and I'd prefer to use it again.

> Logic should be:
> if (> max)
> else if (< min)
> else if (> 0)
> 
> Or that log is potentially wrong.

No, it shouldn't; see the comment:

+        // Separate from the if-else chain so we always fail when min > max.

> That erroneous case should be handled in expectAssertions(), not here.

I don't see why it matters.

    
    

    
  
(In reply to David Baron [:dbaron] from comment #23)

> > Logic should be:
> > if (> max)
> > else if (< min)
> > else if (> 0)
> > 
> > Or that log is potentially wrong.
> 
> No, it shouldn't; see the comment:

Then you should fix the previous description because you have checked "0 < numAsserts <= max", not "min <= numAsserts <= max".

> +        // Separate from the if-else chain so we always fail when min > max.
> 
> > That erroneous case should be handled in expectAssertions(), not here.
> 
> I don't see why it matters.

Because I always thought better to report/fix issues (like wrong data) asap rather than try to work around them later.
Because your current code and descriptions don't match.
Because, when "0 < numAsserts < min" or "max(!) < numAsserts < min", your current code logs two (contradictory) messages.

    
    

    
  
Comment on attachment 610959 [details] [diff] [review]
assertion checking (initially not making tests fail)

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

This looks fine to me, modulo what Serge has already pointed out.

        Attachment #610959 -
        Flags: review?(ted.mielczarek) → review+

    
    

    
  
I also needed some merging with https://hg.mozilla.org/mozilla-central/rev/bb0cc19274c5 to avoid having 2 definitions of isDebugBuild.

    
    

    
  
Turns out mochitest-chrome and mochitest-a11y no longer work (regressed sometime between late December and now):

https://tbpl.mozilla.org/?tree=Try&rev=c4efe84be0f2

JavaScript error: data:text/html,<script>(parent.TestRunner%20||%20parent.wrappedJSObject.TestRunner).testUnloaded()</script>, line 1: Permission denied to access property 'TestRunner'

Is anybody else interested in taking over trying to get this landed who could take over?

    
    

    
  
(A first step might be bisecting when it broke.)

Note that the patch addressing review comments is here:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/702d414a96e6/mochitest-assertion-checking

and then with merging across changes from around the end of March (see comment 26) is here:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4c5281a74a5a/mochitest-assertion-checking

    
    

    
  
Still failing during mochitest-chrome and mochitest-a11y:
https://tbpl.mozilla.org/?tree=Try&rev=752ab56b881e

    
  
Depends on: 671152

    
    

    
  
David: looks like using postMessage works better; I sent a version with those changes to try at <https://tbpl.mozilla.org/?tree=Try&rev=fc8c671169f8>.
Depends on: 817395

    
    

    
  
(Sorry, I didn't notice comment 30 until starting to write this comment, and the changes aren't accessible anymore.)

In any case, I finally got around to moving the document to a separate file (I didn't think of trying postMessage), which seems to work, based on these try runs (the second having a fix for what broke the mochitest-2 and mochitest-ipcplugins in the first):
https://tbpl.mozilla.org/?tree=Try&rev=f31e8bdcd84f
https://tbpl.mozilla.org/?tree=Try&rev=13a3cca109b1

That said, maybe I should think about using postMessage.

Ms2ger, do you still have that patch?

    
    

    
  
Oh, actually, try still has it; it's just the build results that are gone.

    
    

    
  
OK, new try run with Ms2ger's approach:
https://tbpl.mozilla.org/?tree=Try&rev=359de52f597b

    
    

    
  
It turns out both approaches cause a reliable leak on Mac debug mochitest-2.

    
    

    
  
The leak is in the child process for test_browserElement_oop_SetVisibleFrames.html

    
    

    
  
I can reproduce the leak on Linux if I enable the test.

The nsDocShell is held by two nsCOMPtrs from an HttpChannelChild/HttpBaseChannel.  The channel is an nsHTMLDocument's mChannel.

But if I take refcnt+comptr logs on the nsHTMLDocument (3 references leaked, 0 from nsCOMPtrs), the leak goes away.  (Timing-sensitive?)

    
    

    
  
(In reply to David Baron [:dbaron] from comment #36)
> But if I take refcnt+comptr logs on the nsHTMLDocument (3 references leaked,
> 0 from nsCOMPtrs), the leak goes away.  (Timing-sensitive?)

And of the 3, the cycle collector knows about one, which is from the nodeinfomanager.

    
    

    
  
Oh, and the document leaked is the one for file_browserElement_SetVisibleFrames_Inner.html?child1

    
  
Depends on: 842476

    
    

    
  
I filed bug 842476 on the leak.

    
  
Summary: Make assertions fatal in Mochitest → Make assertions cause test failures in Mochitest

    
    

    
  
So I tried some tweaks to make the leaving-a-page path more similar to the way it used to be.  In particular:

 (1) using the _makeIframe function in TestRunner rather than setting iframe.src directly

 (2) reverting the comment 31 (etc.) changes to use postMessage rather than an interstitial page, so that we load the next page over http/chrome rather than data:


Change (2) clearly fixes the problem in bug 842476 on Mac OS X 10.8 only; it remains on 10.7 and 10.6.

It's possible that one or the other reduces the rate of mochitest-3 intermittent failures on Mac OS X (where I think the patch without both changes increased the rate of some existing intermittent failures), but it's hard to tell for sure.

But I'm confused about the behavior of the mochitest-2 leak on Windows.

Try runs for comparison:
 https://tbpl.mozilla.org/?tree=Try&rev=83062779b20a with neither change (older base)
 https://tbpl.mozilla.org/?tree=Try&rev=9c0247430fe7 with changes (1) and (2)
 https://tbpl.mozilla.org/?tree=Try&rev=788600749c21 with only change (1)

    
    

    
  
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #40)
> It's possible that one or the other reduces the rate of mochitest-3
> intermittent failures on Mac OS X (where I think the patch without both
> changes increased the rate of some existing intermittent failures), but it's
> hard to tell for sure.

I think this was just random variation; in my earlier runs I just happened to hit some known intermittent failures.

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=31c9b4d81e6a is a try run with a workaround for bug 842476 (which makes the test not throw an exception, and thus not leak).

I'm worried there's still a problem around dom/plugins/test/test_hanging.html

    
    

    
  
OK, I made one additional tweak, to call the parent's TestRunner from the load event of the interstitial document rather than directly from a <script></script> in it.  That seems to be doing better so far:
https://tbpl.mozilla.org/?tree=Try&rev=31587f8109d7
when combined with the workaround for bug 842476.  (I think I tried it locally and it didn't fix bug 842476, so I think I'll still need that workaround)

    
    

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

I realize I'm taking slight liberties with minor post-review modification here to get this green:
 * comment 31, which I'd like to replace with Ms2ger's postMessage approach
 * comment 40 item (1), which I'd like to keep, and
 * comment 43, which I probably would also like to keep (was noted as issue originally)

Needs to stay open both for the first of those, for getting ted's confirmation that he's ok with those changes, and most importantly to actually move towards checking the assertions and reporting failures.
Whiteboard: [leave open]

    
    

    
  
I looked over your changes as-landed and didn't see anything objectionable. I'm fine with you using some wiggle room to get this landed.

    
    

    
  

      
      
      
      

        Attached file
          
        download-logs.py (deleted)
        — 
      

    


  
This script will let me download logs for analysis to build the table of current assertion counts.

Liberally cribbed from https://hg.mozilla.org/users/dbaron_mozilla.com/buildbot-json-tools/

    
    

    
  

      
      
      
      

        Attached file
          
        make-assertlists.sh (obsolete) (deleted)
        — 
      

    


  
cribbed from bug 472557

    
    

    
  

      
      
      
      

        Attached file
          
        unify-assertlists.sh (obsolete) (deleted)
        — 
      

    


  
cribbed from bug 472557

    
    

    
  

      
      
      
      

        Attached file
          
        unify-assertlists.sh (obsolete) (deleted)
        — 
      

    


  
... and updated to actually work on this set of data

        Attachment #717933 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        make-assertlists.sh (obsolete) (deleted)
        — 
      

    


  
...needs dos2unix, now that tinderbox isn't in the pipeline

        Attachment #717922 -
        Attachment is obsolete: true

    
    

    
  
First (and hopefully primary) round of test annotations landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc2f8fdfb55

    
  
Depends on: 600703

    
    

    
  


  
If this is needed in further mochitests, I'd like to take your review+
as covering those as well.

        Attachment #718775 -
        Flags: review?(jruderman)

    
    

    
  


  
If this is needed in further mochitests, I'd like to take your review+
as covering those as well.

        Attachment #718783 -
        Flags: review?(jruderman)

    
  

        Attachment #718775 -
        Attachment is obsolete: true

        Attachment #718775 -
        Flags: review?(jruderman)

    
    

    
  
Comment on attachment 718783 [details] [diff] [review]
Do GC in a small number of tests to reduce the spread of GC-related assertions.

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

Please file a bug on the assertion firing, and mention it in a comment (along with or instead of the assertion message).

Does this change allow you to change expectAssertions(0, 1) to expectAssertions(1) in these tests, and remove it from some other tests?  An unnecessarily lingering expectAssertions(0, 1) could hide other bugs.

        Attachment #718783 -
        Flags: review?(jruderman) → review+

    
    

    
  
Comment on attachment 718783 [details] [diff] [review]
Do GC in a small number of tests to reduce the spread of GC-related assertions.

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

It might be good to move the expectAssertions calls so they're next to the GC calls.  So it's clear that they should both be removed once the assertion is fixed.

    
    

    
  
I'm going to do another patch on top to adjust the expectAssertions calls so they won't have a range anymore.

    
  
Depends on: 845676

    
    

    
  

      
      
      
      

        Attached file
          
        make-assertlists.sh (deleted)
        — 
      

    


  

        Attachment #718034 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        unify-assertlists.sh (deleted)
        — 
      

    


  

        Attachment #718005 -
        Attachment is obsolete: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c7cedc21bdf2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

    
    

    
  
The main course hasn't hit m-c yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #76)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/535ef5219dac

I think I'd actually have chosen to file a bug there rather than annotate with a 0-inclusive range, but it's a close call.

    
    

    
  
Latest batch of failures:

content/chrome/content/base/test/chrome/test_csp_bug768029.html | Assertion count 1 is greater than expected range 0-0 assertions.

content/chrome/layout/generic/test/test_selection_preventDefault.html | Assertion count 2 is greater than expected range 0-1 assertions.
content/chrome/layout/generic/test/test_selection_preventDefault.html | Assertion count 2 is greater than expected range 0-1 assertions.
content/chrome/layout/generic/test/test_selection_preventDefault.html | Assertion count 3 is greater than expected range 0-2 assertions.
content/chrome/layout/generic/test/test_selection_preventDefault.html | Assertion count 3 is greater than expected range 0-2 assertions.

content/chrome/toolkit/content/tests/chrome/test_bug585946.xul | Assertion count 5 is greater than expected range 0-2 assertions.

content/chrome/toolkit/content/tests/chrome/test_dialogfocus.xul | Assertion count 5 is greater than expected range 0-4 assertions.

content/chrome/toolkit/content/tests/chrome/test_hiddenpaging.xul | Assertion count 2 is greater than expected range 0-0 assertions.

content/chrome/toolkit/content/tests/chrome/test_keys.xul | Assertion count 2 is greater than expected range 0-0 assertions.
content/chrome/toolkit/content/tests/chrome/test_keys.xul | Assertion count 2 is greater than expected range 0-0 assertions.
content/chrome/toolkit/content/tests/chrome/test_keys.xul | Assertion count 2 is greater than expected range 0-0 assertions.
content/chrome/toolkit/content/tests/chrome/test_keys.xul | Assertion count 3 is greater than expected range 0-0 assertions.

content/chrome/toolkit/content/tests/chrome/test_largemenu.xul | Assertion count 2 is greater than expected range 0-0 assertions.

dom/media/tests/mochitest/test_peerConnection_bug825703.html | Assertion count 11 is greater than expected range 0-0 assertions.

dom/media/tests/mochitest/test_peerConnection_bug827843.html | Assertion count 184 is greater than expected range 0-0 assertions.

dom/media/tests/mochitest/test_peerConnection_bug834153.html | Assertion count 214 is greater than expected range 0-0 assertions.

    
    

    
  
There were a few more than in the previous comment, annotated all of them in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d3771bac8c

dbaron, can I leave you to file the bugs?

    
    

    
  
It's starting to look like we could have done with a few more try run retriggers to weed out the intermittent assertions, since there are yet more on inbound now :-(

I've retriggered a number of the debug mochitest jobs on inbound to try and speed this process up.

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/65a2c1fd9d38


A bunch of these things are better off fixed in big batches (bug 846096, bug 846099, bug 845676, bug 683159, need bug on one more set that I'm forgetting) than individually annotated or filed.

    
    

    
  
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #84)
> A bunch of these things are better off fixed in big batches (bug 846096, bug
> 846099, bug 845676, bug 683159, need bug on one more set that I'm
> forgetting) than individually annotated or filed.

Of the big sets of things that spew intermittent assertions across multiple tests:

  bug 846099 and bug 845676 and bug 683159 are now dealt with

  bug 671976 and bug 846096 still need to be dealt with

    
    

    
  
also still needs to be dealt with:  bug 846137

    
    

    
  
also: bug 846150

    
    

    
  
Current status of the big sets of intermittent things:

dealt with: bug 846099, bug 845676, bug 683159, bug 671976

needs work: bug 846096 (needinfo? :smaug), bug 846137 (rare), bug 846150 (rare)

    
    

    
  

      
      
      
      

        Attached file
          
        make-asserttexts. (deleted)
        — 
      

    


  
slow, but does what I needed
Depends on: 846769
Depends on: 671976
Depends on: 847099
Depends on: 847106
Depends on: 846154
Depends on: 847115

    
    

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

It occurs to me that we should probably be noting the assertions we're annotating in the test.
Depends on: 847125
Depends on: 846137
Depends on: 846971
Depends on: 846599

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM] from comment #93)
> It occurs to me that we should probably be noting the assertions we're
> annotating in the test.

Yes.  In general we should be filing bugs on the assertions; it was just impossible to simultaneously do that for all the existing ones and get the patch landed.


I filed followup bugs:

bug 847275: make assertions cause test failures in mochitest-browser-chrome
bug 847276: switch from an interstitial http: page back to a data: URL
Depends on: 847636
Depends on: 848098
Depends on: 848958
Depends on: 849214
Depends on: 651615
Depends on: 849676

    
  
No longer depends on: 683159
Depends on: 850090
Depends on: 850174
Depends on: 850177
Depends on: 851962
Depends on: 854519
Depends on: 855985
Depends on: 856738
Depends on: 857401

    
  
Depends on: 864685
No longer depends on: 857401
No longer depends on: 864685
Depends on: 885107

    
    

    
  
Documentation for SimpleTest.expectAssertions would be nice.
Keywords: dev-doc-needed
Depends on: 906623
No longer blocks: 385228
No longer blocks: 471979

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: