1181687 - Use real Promises in the MockLazyLoader

Status: RESOLVED FIXED

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

Description

[Jim Porter (:squib)]

Currently, the MockLazyLoader[1] returns a fake Promise (an object with .then and .catch), but this fails for code that wants to chain a Promise. Unfortunately, using a real Promise in the MockLazyLoader breaks many System app unit tests because it expects the Promise to be executed synchronously. As a result, the Music app had to add its own MockLazyLoader[2] that returns real Promises. I see two alternatives to fixing this:

1) Fix the System app unit tests to stop expecting a synchronous (fake-) Promise.
2) Move the fake-Promise MockLazyLoader to apps/system/test/unit and move the real-Promise MockLazyLoader from music to shared/test/unit/mocks.

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/test/unit/mocks/mock_lazy_loader.js
[2] https://github.com/jimporter/gaia/blob/246c92f657f0f6e5a56dd252ce455bb2e8f90fdd/apps/music/test/unit/mock_lazy_loader.js

Comment 1

[Sam Foster [:sfoster] (he/him)]

I'm not sure what the "right" thing to do is, but [2] seems like the best choice: it lets us (system app) fix those tests as necessary without breaking other tests in the process. We can always mock window.Promise if synchronocity is really required. Personally, my head is still ringing from my last encounter with unit testing lazy-loading methods and promise chains - its no fun.

Comment 2

[Michiel de Jong [:michielbdejong]]

We ran into the same issue (and same work-around) with the Sync app. Do you want me to have a go at solution 2? Sounds like an easy fix and nice thing to clean up.

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] michielbdejong:1181687-MockLazyLoader-Promises > mozilla-b2g:master

Comment 4

[Michiel de Jong [:michielbdejong]]

I changed the tests from systems app that seemed to work with proper promises. There are now 23 tests left to fix in the systems app that still use apps/system/test/unit/mock_lazy_loader.js (which resolves its promise synchronously).

I also ran across 13 tests that include the actual shared/js/lazy_loader.js, not sure what that meant, so I left those as they are.

Comment 5

[Sam Foster [:sfoster] (he/him)]

I'm not sure what is needed from me here? I left a nit on the PR about the comment in MockLazyLoader, but if the tests pass with those changes (and they seem to), this LGTM.

Comment 6

[Michiel de Jong [:michielbdejong]]

Yes, I only needinfo'd you to make sure this is also what you had in mind, because it touches the system app unit tests. Nit fixed, thanks!

Comment 7

[Jim Porter (:squib)]

Comment on attachment 8690774 [details] [review]
[gaia] michielbdejong:1181687-MockLazyLoader-Promises > mozilla-b2g:master

Looks good to me, but you might want a system app person to look at this too. I'm not a reviewer for that component.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8690774 [details] [review]
[gaia] michielbdejong:1181687-MockLazyLoader-Promises > mozilla-b2g:master

Thank you Michiel. Make sure that treeherder is happy with this patch, please.

Comment 9

[Michiel de Jong [:michielbdejong]]

Tests went green after another rebase.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/f07f32110edbbbd477519b12dc2ecd16343aacf0