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



Firefox OS
4 years ago
4 years ago


(Reporter: julienw, Assigned: julienw)


(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)



(1 attachment)



4 years ago
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)


The issue is actually in the shared mock for mozSettings.

Comment 1

4 years ago
Created attachment 8495463 [details] [review]
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.
Attachment #8495463 - Flags: review?(etienne)

Comment 2

4 years ago
BTW this can also produce intermittent failures with the current mocha.
Comment on attachment 8495463 [details] [review]
github PR

redirecting :)
Attachment #8495463 - Flags: review?(etienne) → review?(dflanagan)
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.
Attachment #8495463 - Flags: review?(dflanagan) → review+

Comment 5

4 years ago
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

4 years ago
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 :)
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.