Closed Bug 1181687 Opened 9 years ago Closed 9 years ago

Use real Promises in the MockLazyLoader


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

Gonk (Firefox OS)
Not set


(Not tracked)



(Reporter: squib, Assigned: mbdejong)




(1 file)

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.

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.
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.
Attachment #8690774 - Flags: review?(squibblyflabbetydoo)
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.
Flags: needinfo?(sfoster)
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.
Flags: needinfo?(sfoster)
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!
Assignee: nobody → mbdejong
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.
Attachment #8690774 - Flags: review?(squibblyflabbetydoo) → review+
Attachment #8690774 - Flags: review?(ferjmoreno)
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.
Attachment #8690774 - Flags: review?(ferjmoreno) → review+
Tests went green after another rebase.
Flags: needinfo?(ferjmoreno)
Closed: 9 years ago
Flags: needinfo?(ferjmoreno)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.