1073157 - [System] fix wallpaper_manager.js tests with a newer mocha

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Julien Wajsberg [:julienw]]

In Bug 874510 we want to upgrade mocha to a newer version. Some system tests are failing with the newer mocha:

02:54:20     INFO -  TEST-UNEXPECTED-FAIL | system/test/unit/wallpaper_manager_test.js | WallpaperManager start with validated blob - Error: expected false to be truthy (app://system.gaiamobile.org/common/test/helper.js?time=1411552459474:31)
02:54:20     INFO -      Error: Error: expected false to be truthy (app://system.gaiamobile.org/common/test/helper.js?time=1411552459474:31)
02:54:20     INFO -          at onerror (app://system.gaiamobile.org/common/vendor/mocha/mocha.js:5738:10)

https://tbpl.mozilla.org/php/getParsedLog.php?id=48759080&tree=Gaia-Try#error16

The issue is actually in the shared mock for mozSettings.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: github PR

The main issue here is that the mock's timeouts are triggered in a later test, because the new mocha is SO faster.

So in this patch I clearTimeout all timeouts in the teardown function.

I also took the liberty to do some small changes in wallpaper_manager_test.js, but I can revert if you want.

Comment 2

[Julien Wajsberg [:julienw]]

BTW this can also produce intermittent failures with the current mocha.

Comment 3

[Etienne Segonzac (:etienne)]

Comment on attachment 8495463 [details] [review]
github PR

redirecting :)

Comment 4

[David Flanagan [:djf]]

Comment on attachment 8495463 [details] [review]
github PR

I'm not entirely comfortable reviewing the shared settings mock. But I'm not sure who the owner is, and I guess Etienne thinks I'm an okay reviewer...

I have not tried to run the tests with these changes, but I assume you have.

The changes to the wallpaper manager tests look like clear improvements. (I left some queries on github, but assuming that I understood the changes correctly, they look good to me.) 

The changes to the settings mock seem correct to me. Documentation would be nice, but given that the entire file has no comments, I guess they're not really needed in this patch.

Comment 5

[Julien Wajsberg [:julienw]]

You're right, I'll add some comments before merging. This mock is exceptionally complicated (can work in both synchronous and asynchronous way) so it makes sense to at least document this.

I wish we'd transition from DOMRequest to Promises, mocks would be a lot simpler!

Comment 6

[Julien Wajsberg [:julienw]]

Added a comment explaining how the mock works, hope it's good for you :)
I also removed some "console.log" I left in a previous patch...

master: cad6f33bb044cdc3967b346a6a8872541bfb8e56

Thanks :)