475322 - Browser chrome test harness shouldn't use a periodic poller

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This removes the poller and instead uses executeSoon to allow the existing script to finish up whatever it is doing.

This should save us about 1.5 seconds per test file which is a fairly worthwhile saving I think. Downside is that it adds some extra entries to the test's scope, not sure how much of a problem it is, certainly all existing browser chrome tests pass with the changes.

I noticed that the timeout doesn't include the synchronous portion of the test, not sure if we ever want to change that?

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This just fixes the timeout test to fail with the longer timeouts. gavin gave r+ over IRC

Comment 3

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/37d6dfd70bac

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Are you going to land this on branches as well? I think it's a good idea to keep the harness in sync between branches as much as possible.

Comment 5

[Dave Townsend [:mossop]]

(In reply to comment #4)
> Are you going to land this on branches as well? I think it's a good idea to
> keep the harness in sync between branches as much as possible.

Yes when I get enough free time to watch the tree (likely Wednesday), or someone else can land it.

Comment 6

[Dave Townsend [:mossop]]

Pushed to branch http://hg.mozilla.org/releases/mozilla-1.9.1/rev/647c4651340f

Comment 7

[:Gavin Sharp [email: [e-mail]]]

johnath and I noticed something odd on 1.9.1 - a test "timed out" but then reported results. Shouldn't we cancel the waitTimer outside of the executeSoon (i.e. immediately)?

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> johnath and I noticed something odd on 1.9.1 - a test "timed out" but then
> reported results. Shouldn't we cancel the waitTimer outside of the executeSoon
> (i.e. immediately)?

This is certainly not a new problem. The issue is that while we can detect a test has been running too long and start the next one, we have no way of stopping whatever the existing test is doing. If a network operation or such was just taking a really long time it will eventually report back and the test will continue. Cancelling the waitTimer outside the executeSoon isn't going to help in anyway that I can spot (except it might catch a case where the timeout is pending when the test calls finish, in which case you'd see a timeout and nothing after it).

What we would probably need to do (and it is only a way of hiding this) is to stop pushing test results after a timeout has occurred. I tend to think though that there is value in knowing that the test was actually still going albeit slowly.