514801 - xpcshell-tests: test_imapFilterActions.js fails intermittently on TB and permanently on SM

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1252110018.1252115814.24256.gz&fulltext=1
MacOSX 10.5 comm-central check on 2009/09/04 17:20:18
{
[...]
test_imapFilterActions.js | 1 == 0 - See following stack:
xpcshell/head.js :: do_throw :: line 197
xpcshell/head.js :: do_check_eq :: line 227
test_imapFilterActions.js :: testCounts :: line 757
test_imapFilterActions.js :: checkChangePriorityBody :: line 221
test_imapFilterActions.js :: testContinue :: line 428
test_imapFilterActions.js :: OnItemEvent :: line 540
}

Comment 1

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1252150492.1252157948.6463.gz
Win2k3 comm-central check on 2009/09/05 04:34:52

***

Regression timeframes for SeaMonkey (3 platforms):
http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=f39f311f9032&tochange=e62d7ebe4a8e
http://hg.mozilla.org/comm-central/pushloghtml?fromchange=8a32844b76fb&tochange=b11a45aa831d
It's obviously bug 127250 which added this test.

Example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1252108242.1252118201.19011.gz&fulltext=1
WINNT 5.2 comm-1.9.1 unit test on 2009/09/04 16:50:42

test_imapFilterActions.js | 0 == 1 - See following stack:
xpcshell\head.js :: do_throw :: line 159
xpcshell\head.js :: do_check_eq :: line 189
test_imapFilterActions.js :: testCounts :: line 760
test_imapFilterActions.js :: checkMarkRead :: line 108
test_imapFilterActions.js :: testContinue :: line 428
test_imapFilterActions.js :: OnItemEvent :: line 540
+
test_imapFilterActions.js | 2147500037 == 0 - See following stack:
xpcshell\head.js :: do_throw :: line 159
xpcshell\head.js :: do_check_eq :: line 189
test_imapFilterActions.js :: OnStopRunningUrl :: line 575
}

Comment 2

[Kent James (:rkent)]

Bug 127250 added extensive new tests of all imap filter actions, while the functional part of the bug only added a single new action. However, the calling sequence for all of the filter actions was changed, which was one reason that the more extensive tests were felt appropriate. But many of these things were quite difficult to test, and there is some additional backend code that was added merely to provide additional hooks needed for the unit tests.

I mention this to make the point that the failing tests do not necessarily indicate a problem with the functional changes done by the patch. It may be appropriate to disable the tests for now, and I will investigate further what is happening. The intermittent failures are almost certainly timing related. I'll need to investigate the SM change to see if it is a real issue.

Comment 3

[Kent James (:rkent)]

Attachment: Fixes SM test failure

This patch fixes the failing test in SM. But because it touches the very fragile new count management area, I want to think about it some more.

Comment 4

[Kent James (:rkent)]

Attachment: Disables test failing in SM, delays others

After thinking about my previous patch, I think it is too risky at this point in time. Other code assumes it is responsible for maintaining the new message count, and adding a new location where it is reset to 0 could have unforeseen consequences. Plus, I biefly tried to see any ill effects of this in SM, and could not. So instead of fixing the core, I want to simply disable the particular count test that is failing.

The Mac failures are quite different, and are timing related and random in location. This patch also includes an attempt to fix those by moving the actual tests to the delayed call, and increasing the length of that delay.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 399085 [details] [diff] [review]
Disables test failing in SM, delays others

>       if (gMoveCallbackCount != 2)
>         return;
>     }
>-    if (gChecks)
>-      gChecks();
>     gCurTestNum++;
>-    do_timeout(100, "doTest();");
>+    do_timeout(200, "doTest();");

It could also be possible that doing the checks in the testCountinue without having allowed OnStopRunningURL to exit could potentially mean we're blocking on things that could be running async (or haven't allowed them to finish). Hence giving us problems. So moving the checks is definitely the right thing to do IMHO.

r=Standard8

Comment 6

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/94b69751ee35

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> So instead of fixing the core,
> I want to simply disable the particular count test that is failing.

You may want to file a follow-up bug to fix that code/test.

V.Fixed, per SeaMonkey tinderboxes.