468400 - xpcshell-simple/test_places/unit/test_adaptive.js | *** TIMEOUT ***: The test timed out while polling database.

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Mats Palmgren (inactive)]

"Linux mozilla-central moz2-linux-slave08 dep unit test" Tinderbox orange:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228705679.1228708323.9266.gz#err0

TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/unit/test_adaptive.js | test failed, see log
../../../../_tests/xpcshell-simple/test_places/unit/test_adaptive.js.log:
>>>>>>>
*** test pending
*** test pending
Test 0 same count, diff rank, same term; no search
*** test finished
*** running event loop
Test 1 same count, diff rank, same term; no search
*** exiting
*** TEST-UNEXPECTED-FAIL | ../../../../_tests/xpcshell-simple/test_places/unit/test_adaptive.js | *** TIMEOUT ***: The test timed out while polling database.

====================

The only checkin for this build was
http://hg.mozilla.org/mozilla-central/rev/f6aa0df95deb
and it's very very unlikely to have caused this test failure,
so I believe it's something wrong with the test itself,
or with the Tinderbox host, or with the test framework.

Comment 2

[Marco Bonardo [:mak]]

that's a random due to some timer bug

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch

unluckily i cannot reproduce locally, but being aware of oldest timer issue with VM i can suspect the polling timeout could be not correctly set between tests, so we could end up calculating more than one test timeout.

For now i suggest moving to count polling loops instead of a timeout, if that is still failing we could reopen and try increasing the values, or eventually re-evaluate the approach.

Comment 4

[Marco Bonardo [:mak]]

Attachment: correct patch

sorry. that was the wrong patch

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 353047 [details] [diff] [review]
correct patch

I think we should just add an observer topic when it's completed.  It makes testing for correctness about a million times easier, and won't hurt us.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch

this implements the notification, i've tried to make the class a bit more generic to be reused once we will add new async statements, but maybe you prefer a specialized class for the autocomplete. let me know.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 353801 [details] [diff] [review]
patch

Let's just make it specific please.  It makes the code easier to read (and I'm not sure what else we'd have to use this for).

Comment 8

[Marco Bonardo [:mak]]

for example i was thinking to things we actually do on lazy_add, like setPageTitle, we could use this to notify after setting the title async, would be ready to use.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> for example i was thinking to things we actually do on lazy_add, like
> setPageTitle, we could use this to notify after setting the title async, would
> be ready to use.
I think until we figure out how exactly we'll want to do that, we should make this specific for our use case.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v2

reverted back to be autocomplete only.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 354141 [details] [diff] [review]
patch v2

>+NS_IMETHODIMP
>+AutoCompleteStatementCallbackNotifier::HandleError(mozIStorageError *aError)
>+{
>+  nsCAutoString warnMsg;
>+  warnMsg.Append("An error occured while executing an async statement: ");
>+  PRInt32 result;
>+  aError->GetResult(&result);
>+  warnMsg.Append(result);
>+  warnMsg.Append(" ");
>+  nsCAutoString message;
>+  aError->GetMessage(message);
>+  warnMsg.Append(message);
>+  NS_WARNING(warnMsg.get());
Check return values or cast to void please (I know we don't do that everywhere, but we do it in some places, and I think for clarity of code we should start doing it in new code).

r=sdwilsh

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/3e1f7b96a3f2

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 354141 [details] [diff] [review]
patch v2

requesting approval, this solves a potential random test failure, but involves also toolkit code so it's not tests-only. Risk is low.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

This testcase is failing on Fennec and after reading the code with mfinkle we were not sure why it is failing.  Specifically test #3 is failing where the first returned search result is uri2 instead of uri1.

Here is a reference to the test case that fails:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_adaptive.js#246

Could I get some assistance with understanding how and why this is failing?  There are a few other failures in the same test folder which I think are related.

Comment 15

[Marco Bonardo [:mak]]

in the same test folder, you mean some of the autocomplete tests?

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

specifically test_frecency.js and test_000_frecency.js have multiple tests in them that fail.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

reopening as these tests are failing on Fennec and appear to be related to the same issue/fix.

Comment 18

[Marco Bonardo [:mak]]

i think you should open a new bug specific for fennec issue instead, since this original issue is fixed on trunk.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

ok, closing this bug and I have opened bug 482964 to track the specific failures on Fennec

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354141 [details] [diff] [review]
patch v2

a191=beltzner

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/814c279d7b12