Closed Bug 868646 Opened 7 years ago Closed 7 years ago

1 notification gaia-unit-test fails on TBPL

Categories

(Firefox OS Graveyard :: Gaia, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jgriffin, Assigned: julienw)

References

Details

(Whiteboard: u=fx-os-user c=scravag-sprint p=1)

Attachments

(1 file, 3 obsolete files)

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
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 ?
Assignee: nobody → felash
Blocks: 874441
Whiteboard: u=fx-os-user c=scravag-sprint-may-20-31 p=1
Whiteboard: u=fx-os-user c=scravag-sprint-may-20-31 p=1 → u=fx-os-user c=scravag-sprint p=1
Attached patch patch v1 (obsolete) — Splinter Review
* 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
Attachment #752636 - Flags: review?(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?
Attachment #752636 - Flags: review?(etienne)
Attached patch patch v2 (obsolete) — Splinter Review
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
Attachment #752636 - Attachment is obsolete: true
Attachment #753196 - Flags: review?(etienne)
Attached patch patch v3 (obsolete) — Splinter Review
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
Attachment #753196 - Attachment is obsolete: true
Attachment #753196 - Flags: review?(etienne)
Attachment #753257 - Flags: review?(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)| ?
Attachment #753257 - Flags: review?(etienne) → review+
master: 7c059ba62eda274492de0b2504baf94060d5a642
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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 :)
Attached patch final patchSplinter Review
the patch that landed. carrying r=etienne
Attachment #753257 - Attachment is obsolete: true
Attachment #753396 - Flags: review+
You need to log in before you can comment on or make changes to this bug.