Closed Bug 631329 Opened 11 years ago Closed 11 years ago

Intermittent "TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug629838.html | Test timed out."


(Core :: Layout, defect)

Not set





(Reporter: cjones, Assigned: kael)



(Keywords: intermittent-failure)


(1 file, 2 obsolete files)
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitests-4/5 on 2011/02/03 10:35:16 
s: talos-r3-leopard-039
22098 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug629838.html | Test timed out.

Things go haywire around

991 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | Background color iteration 24, afterpaint count: 19, mozpaint count: 23
992 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | OnAfterPaint
993 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | Background color iteration 25, afterpaint count: 20, mozpaint count: 25
994 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | OnAfterPaint
995 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | Background color iteration 26, afterpaint count: 21, mozpaint count: 26
996 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | OnAfterPaint
997 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | Background color iteration 27, afterpaint count: 22, mozpaint count: 27
998 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | OnAfterPaint

The problem looks to be in this code

function startTest() {
  setTimeout(function () {
    afterPaintCount = 0;
    initialPaintCount = window.mozPaintCount;
    window.addEventListener("MozAfterPaint", onAfterPaint, true);
  }, 1000);

when this runs, 0 <= window.mozPaintCount <= 4.  But, the test then waits 1000ms before doing anything else.  It's making the bad assumption that no painting happens after DOMContentLoaded.

So, when we get to the "loop condition" here

  if ((afterPaintCount >= window.mozPaintCount - initialPaintCount) && 
      (afterPaintCount > 20)) {

the assumption that the test can't miss MozAfterPaint events is wrong, and the first clause in the if condition is bogus.

I wonder if the best fix is to move the initialization code out of the delayed task in startTest().
Assignee: nobody → kevin.gadd
Disregard comment 0, I confused myself.

I'm not sure what's going on here.  The test looks OK, AFAICT.
In the meantime, if we change

  if ((afterPaintCount >= window.mozPaintCount - initialPaintCount) && 

into an "assertion", the test will fail more quickly at least.
Summary: Intermittent "TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug629838.html | Test timed out." (test is wrong?) → Intermittent "TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug629838.html | Test timed out."
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central opt test mochitests-4/5
Blocks: 438871
Whiteboard: [orange]
Run the test until mozPaintCount has increased by 50, then check to ensure that we got at least 37 MozAfterPaint events. This should pass more reliably while still ensuring that MozAfterPaint is being fired by empty transactions.

Attachment #509692 - Flags: review?(roc)
Attachment #509692 - Flags: review?(jones.chris.g)
Just picked an arbitrary number that's high enough to fail the test if the MozAfterPaint/empty transaction fix is missing, but low enough to pass for the example from the original comment.
Comment on attachment 509692 [details] [diff] [review]
Make the test for bug 629838 less sensitive to variations in paint event count


Factor out the constants, add a comment about why they were chosen and how to change them if the test starts failing intermittently again, then r=me.
Attachment #509692 - Flags: review?(jones.chris.g) → review+
Moved the tuning values into constants and expanded the comments
Attachment #509692 - Attachment is obsolete: true
Attachment #509699 - Flags: review?(jones.chris.g)
Attachment #509692 - Flags: review?(roc)
Comment on attachment 509699 [details] [diff] [review]
Make the test for bug 629838 less sensitive to variations in paint event count (2)

Looks good.

Minor point of order: usually if reviewers r+ a patch but leave comments to be addressed, then they don't need to see the fixed-up patch.  For me, that means "this patch is fine to check in as-is but would be better if you fixed X,Y,Z, which I trust you to do".  Reviewers vary though :).
Attachment #509699 - Flags: review?(jones.chris.g) → review+
Keywords: checkin-needed
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
I backed this patch as part of this pushlog <> because of the oranges it caused <>.

I'm not sure which one of the bugs was at fault, so I just backed them all out.  The assignee needs to investigate and make sure that his patch has not been the culprit, and then re-add checkin-needed.
Resolution: FIXED → ---
Also, Kevin, please start the commit message for all of your patches with the bug number.  The only bug number referenced in your commit message was bug 629838, so there is no way for anybody to reach this bug from your commit.
Blocks: 629838
Fixed commit comment and rebased.
Attachment #509699 - Attachment is obsolete: true
Keywords: checkin-needed
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.