868646 - 1 notification gaia-unit-test fails on TBPL

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Jonathan Griffin (:jgriffin)]

Error:  

14:56:46     INFO -  gaia-unit-tests TEST-UNEXPECTED-FAIL |  "before each" hook | TypeError: self.toaster is null (http://system.gaiamobile.org:8080/js/update_manager.js?time=1367618203850:405)
14:56:46     INFO -  gaia-unit-tests INFO       | stack trace:
14:56:46     INFO -      Error: TypeError: self.toaster is null (http://system.gaiamobile.org:8080/js/update_manager.js?time=1367618203850:405)
14:56:46     INFO -          at onerror (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4959)

Log:   https://tbpl.mozilla.org/php/getParsedLog.php?id=22568249&tree=Cedar&full=1#error0

Comment 1

[Julien Wajsberg [:julienw]]

This happens sometimes and I thought Bug 864664 would have fixed this completely.

Today: https://tbpl.mozilla.org/php/getParsedLog.php?id=22820482&tree=Cedar&full=1
This is not the exact same failure because this is a timing issue.

Jonathan, can we know which gaia commit hash is used ?

Comment 2

[Jonathan Griffin (:jgriffin)]

Julien, this last run was from Friday and used this gaia commit:  http://hg.mozilla.org/integration/gaia-central/rev/bc834da1c2c4

which corresponds to:
https://github.com/mozilla-b2g/gaia/commit/4a51ebf718b1ffdedcbe6415d88fcbdb937e6b97

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: patch v1

* use sinon fake timers to control timeouts
* fixed missing teardown
* fixed a bad assert in a setup
---
 apps/system/test/unit/update_manager_test.js |  362 +++++++++++---------------
 1 file changed, 149 insertions(+), 213 deletions(-)

see also PR https://github.com/mozilla-b2g/gaia/pull/9925

Comment 4

[Etienne Segonzac (:etienne)]

Comment on attachment 752636 [details] [diff] [review]
patch v1

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

This is really cool and looks more robust!

But I wonder, now that we have a proper way to free ourselves from those orange-prone async tests, shouldn't we avoid having async mocks.
My idea would be to remove the setTimeout in |mock_apps_mgmt| and the 17 |this.sinon.clock.tick(1)| that look a bit weird.

What do you think?

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: patch v2

use fake timers only where necessary.

The only uncertainty I have now is that the most outer teardown now has no setTimeout. The tests passes on my slow computer and on my fast computer so it seems this leads to no problem. Will see what travis says.

see PR https://github.com/mozilla-b2g/gaia/pull/9950

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: patch v3

same PR https://github.com/mozilla-b2g/gaia/pull/9950

I added back the setTimeout in the main teardown function, and restore the real timer before that.

Works for me on both computers (slow and fast), let's see if this works in Travis

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 753257 [details] [diff] [review]
patch v3

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

\o/

::: apps/system/test/unit/update_manager_test.js
@@ +697,4 @@
>              UpdateManager.addToUpdatesQueue(updatableApps[1]);
> +
> +            this.sinon.clock.tick(tinyTimeout);
> +            this.sinon.clock.tick(tinyTimeout);

nit: what's the difference with a |this.sinon.clock.tick(tinyTimeout*2)| ?

Comment 8

[Julien Wajsberg [:julienw]]

master: 7c059ba62eda274492de0b2504baf94060d5a642

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Etienne Segonzac (:etienne) from comment #7)

> 
> nit: what's the difference with a |this.sinon.clock.tick(tinyTimeout*2)| ?

Changed it, and it worked, so I kept this :)

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: final patch

the patch that landed. carrying r=etienne