Closed
Bug 1080466
Opened 9 years ago
Closed 9 years ago
Add a `setTimeout` for Promise
Categories
(Toolkit :: Async Tooling, defect)
Toolkit
Async Tooling
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: Yoric, Assigned: akshendra521994, Mentored)
References
Details
(Whiteboard: [lang=js][good second bug])
Attachments
(1 file, 9 obsolete files)
7.40 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
Let's provide a version of Timer.jsm's `setTimeout` for `Promise`.
Reporter | ||
Comment 1•9 years ago
|
||
The code lives here: http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Timer.jsm?from=Timer.jsm#l1 The objective of this bug is to extend Timer.jsm with a new function `promiseTimeout(delay)` that returns a `Promise` resolved after `delay` milliseconds. Internally, this can be implemented with `setTimeout` (already defined in the same module). This is a simple bug if you understand `Promise`.
Mentor: dteller
Whiteboard: [lang=js][good second bug]
Comment 2•9 years ago
|
||
Hmm. Without prejudice as to choice of good function names, I was thinking something more like: PromiseUtils.resolveOrTimeout(aPromise, delay, ?message) that would return a Promise that resolves/rejects with the result of aPromise if it settles before 'delay' milliseconds pass, or rejects with a "Timed Out: message" error if aPromise does not settle within 'delay' ms.
Assignee | ||
Comment 3•9 years ago
|
||
Please clarify if I am wrong. What you want is that a new function is added to times.jsm that takes a delay and Promise as arguments. And after the delay, calls the resolve method of the Promise object. or Something on the lines of Comment #2, check if the Promise is settled after the delay of time or not. If not then rejects with the error message.
Reporter | ||
Comment 4•9 years ago
|
||
Let's do what's Irving suggested in Comment 2. Once we have a first version, we will be able to revisit things if we find out that it's not exactly what we want.
Assignee | ||
Comment 5•9 years ago
|
||
When I add the function in Timer.jsm, I get the following error
> TypeError: window.resolveOrTimeout is not a function
Any suggestions?
Reporter | ||
Comment 6•9 years ago
|
||
Why would you want to use `window.resolveOrTimeout`? Indeed, there is no such function.
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #6) > Why would you want to use `window.resolveOrTimeout`? Indeed, there is no > such function. I added a function named resolveOrTimeout in Timer.jsm, how do I access it. I thought that the other function declared in that module were accessible through the window object. So, I tried window.resolveOrTimeout.
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8507842 -
Flags: review?(dteller)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8507842 -
Attachment is obsolete: true
Attachment #8507842 -
Flags: review?(dteller)
Attachment #8507861 -
Flags: review?(dteller)
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8507861 [details] [diff] [review] 1080466.patch Review of attachment 8507861 [details] [diff] [review]: ----------------------------------------------------------------- Let's try something simpler. Also, you should add a test to http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_timer.js to make sure that your function works. ::: toolkit/modules/Timer.jsm @@ +7,5 @@ > /** > * JS module implementation of nsIDOMJSWindow.setTimeout and clearTimeout. > */ > > +this.EXPORTED_SYMBOLS = ["setTimeout", "clearTimeout", "awesome"]; "awesome"? Did you forget changing something here, by any chance? @@ +14,5 @@ > const Ci = Components.interfaces; > const Cu = Components.utils; > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); You don't use Services.jsm, so don't import it. @@ +21,5 @@ > +const STATUS_RESOLVED = 1; > +const STATUS_REJECTED = 2; > + > +const salt = Math.floor(Math.random() * 100); > +const N_INTERNALS = "{private:internals:" + salt + "}"; You don't need to hit the internals of Promise for such a simple function. @@ +71,5 @@ > + }); > + } > + }); > + return resultingPromise; > +} \ No newline at end of file You can do something much, much, much simpler, that doesn't need to hit the internals of `Promise` – especially since these internals are going to change soon. I believe that you only need - one call to `setTimeout`; - one call to `new Promise()`; - one call to `Promise.race`.
Attachment #8507861 -
Flags: review?(dteller)
Assignee | ||
Comment 11•9 years ago
|
||
> this.resolveOrTimeout = function resolveOrTimeout(aPromise, aMilliseconds, aMessage) {
> return new Promise(function(resolve, reject) {
> Promise.race([aPromise]).then(resolve, reject);
> this.setTimeout(
> function() {
> reject(Error("Timeout: " + aMessage));
> }, aMilliseconds);
> });
> }
I am using this function and this seems to work, tried the internals of the function seperately. But when I call the function the received object rejects with an empty object as response. Is there something wrong with the function?
Reporter | ||
Comment 12•9 years ago
|
||
Your code seems to work for me. How are you testing it? Note, by the way, that when you call `Promise.race(list)`, you obtain a promise that is resolved/rejected once the first promise of `list` is resolved/rejected. Here, your list consists in a single promise, so there is no race. In other words, if `aPromise` is a `Promise`, `Promise.race([aPromise])` is basically the same thing as `aPromise`.
Reporter | ||
Comment 13•9 years ago
|
||
By the way, in the future, please use "Need more information from" to make sure that I see your message. Otherwise, it risks getting lost in the ~800 e-mails/day I receive.
Assignee | ||
Comment 14•9 years ago
|
||
I tried it using the following code in an html file and that worked https://pastebin.mozilla.org/6862416 But when I receive the object in the test file like below https://pastebin.mozilla.org/6862432 In this case it get rejected with an empty object.
Reporter | ||
Comment 16•9 years ago
|
||
Well, the `do_check_eq` will fail because `Error` is not a string, but other than that, it looks liike the test should work. What's the output? Note: needinfo worked nicely.
Flags: needinfo?(dteller)
Assignee | ||
Comment 17•9 years ago
|
||
The problem is that the received promise seems to reject instantly, it does not wait at all and that too with an empty object. It tries to compare something like {} == "Sorry", which obviously fails. I even tried something very simple like just providing a resolve or reject after a delay inside the resolveOrTimeout(), again the result does not seem to get propogated to the object received in the test file, it only gets rejected with an empty object.
Assignee | ||
Comment 18•9 years ago
|
||
The received object gets instantly rejected with Error (oops the empty object was actually an error object) ie a TypeError and says "This is undefined".
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(dteller)
Reporter | ||
Comment 19•9 years ago
|
||
I assume that "the received object" is `p1` in https://pastebin.mozilla.org/6862432, right? If so, why don't you test with a promise that never resolves, e.g. `new Promise(resolve => {})` or equivalently `new Promise(function() {})`?
Flags: needinfo?(dteller)
Assignee | ||
Comment 20•9 years ago
|
||
"the received object" is not p1, sorry for my bad choice of words. Its the Promise that is returned from the resolveOrTimeout() function. This object gets rejected instantly with a TypeError: This is not defined. Ans as you said, I also tried with the Promise that does not resolves at all. Same problem.
Assignee | ||
Comment 21•9 years ago
|
||
do_test_pending(); imported.resolveOrTimeout(p1, 300, "Sorry").then(null, function(error) { do_check_eq(error.name + error.message,"Timeout: Sorry"); do_test_finished(); }); error.name + error.message; // this says TypeError this is undefined Ans this happens instantly no matter how much delay is pass.
Flags: needinfo?(dteller)
Reporter | ||
Comment 22•9 years ago
|
||
Ah, ok. In that case, the error message contains your answer: `this` is not defined. Apparently, you are trying to execute code that uses `this` in a context where it is not defined. To fix this, you can either capture `this.setTimeout` in a global variable `setTimeout`, or switch to the arrow syntax for your anonymous functions, e.g. new Promise((resolve, reject) => { // Here, `this` is the same as outside the call to `new Promise`. });
Assignee | ||
Comment 24•9 years ago
|
||
:) Thanks for the help.
Attachment #8507861 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8510268 -
Flags: review?(dteller)
Reporter | ||
Comment 25•9 years ago
|
||
Comment on attachment 8510268 [details] [diff] [review] 1080466.patch Review of attachment 8510268 [details] [diff] [review]: ----------------------------------------------------------------- Good first version. I realize that this function should probably go to a new module `PromiseUtils.jsm`, in the same directory as `Timer.jsm`. Also, let's move the tests to a new file `test_promiseutils.js`. You will need to update moz.build and xpcshell.ini so that they build your new files. Also, don't forget to add a commit message, e.g. "Bug 1080466: Implement resolveOrTimeout;r=yoric" ::: toolkit/modules/Timer.jsm @@ +40,5 @@ > gTimeoutTable.delete(aId); > } > } > + > +this.resolveOrTimeout = function resolveOrTimeout(aPromise, aMilliseconds, aMessage) { This function needs documentation. @@ +44,5 @@ > +this.resolveOrTimeout = function resolveOrTimeout(aPromise, aMilliseconds, aMessage) { > + return new Promise((resolve, reject) => { > + aPromise.then(resolve, reject); > + this.setTimeout( > + function() { Let's use the arrow syntax here, too. this.setTimeout(() => reject(...), aMilliseconds) ::: toolkit/modules/tests/xpcshell/test_timer.js @@ +26,5 @@ > do_check_true(timeout2 > 0, "setTimeout returns a positive number"); > do_check_neq(timeout1, timeout2, "Calling setTimeout again returns a different value"); > > imported.clearTimeout(timeout1); > + Your tests should go to a new function, e.g. `function test_resolveOrTimeout`. @@ +27,5 @@ > do_check_neq(timeout1, timeout2, "Calling setTimeout again returns a different value"); > > imported.clearTimeout(timeout1); > + > + // test for the resolveOrTimeout : case where the passed Promise is not resolved within the pasesed time Nit: Please remove whitespace at the end of the line. @@ +34,5 @@ > + imported.setTimeout( > + function() { > + resolve("p1 is resolved"); > + }, 10000); > + }); These days, we prefer the arrow syntax for anonymous functions, e.g. let p1 = new Promise(resolve => imported.setTimeout( () => resolve("p1 is resolved"), 10000 ) ) @@ +36,5 @@ > + resolve("p1 is resolved"); > + }, 10000); > + }); > + > + do_test_pending(); Once you have moved your tests to a new file, I suggest you use `add_task` to chain your asynchronous tests, rather than do_test_pending/do_test_finished, this will be much more readable and robust. @@ +37,5 @@ > + }, 10000); > + }); > + > + do_test_pending(); > + imported.resolveOrTimeout(p1, 300, "Sorry").then(null, Once you have moved to `add_task`, you will be able to write try { yield imported.resolveOrTimeout(...); ... } catch (ex) { ... } @@ +40,5 @@ > + do_test_pending(); > + imported.resolveOrTimeout(p1, 300, "Sorry").then(null, > + function(error) { > + do_check_eq(error,"Timeout: Sorry"); > + do_test_finished(); Nit: whitespace. @@ +43,5 @@ > + do_check_eq(error,"Timeout: Sorry"); > + do_test_finished(); > + }); > + > + // test for the resolveOrTimeout : case where the passed Promise is resolved within the pasesed time Nit: whitespace. @@ +56,5 @@ > + do_test_pending(); > + imported.resolveOrTimeout(p2, 300, "Sorry").then( > + function(val) { > + do_check_eq(val, "p2 is resolved"); > + do_test_finished(); Nit: whitespace.
Attachment #8510268 -
Flags: review?(dteller) → feedback+
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8510268 -
Attachment is obsolete: true
Attachment #8510394 -
Flags: review?(dteller)
Reporter | ||
Comment 27•9 years ago
|
||
Comment on attachment 8510394 [details] [diff] [review] 1080466.patch Review of attachment 8510394 [details] [diff] [review]: ----------------------------------------------------------------- Good code. Now, let's build on that :) ::: toolkit/modules/PromiseUtils.jsm @@ +1,1 @@ > +/* */ Please add the standard headers: https://www.mozilla.org/MPL/headers/ @@ +1,5 @@ > +/* */ > + > +"use strict" > + > +this.EXPORTED_SYMBOLS = ["resolveOrTimeout"]; Let's put everything in an object PromiseUtils and export that symbol instead. @@ +4,5 @@ > + > +this.EXPORTED_SYMBOLS = ["resolveOrTimeout"]; > + > +let imported = {}; > +Components.utils.import("resource://gre/modules/Timer.jsm", imported); Just import to `this`. Also, please remove the whitespace at the end of the line. @@ +8,5 @@ > +Components.utils.import("resource://gre/modules/Timer.jsm", imported); > + > +/* resolveOrTimeout returns a Promise that will get settled with the result of the "aPromise" > + * IF "aPromise" settles within the "aMillisecond" delay that is passed > + * Otherwise the returned Promise will get rejected with a message "Timeout: " + aMessage. */ Let's rephrase, reformat and take the opportunity to change `aMessage` into something a bit more generic. /** * A simple timeout mechanism. * * Example: * resolveOrTimeout(myModule.shutdown(), 1000, new Error("The module took too long to shutdown")); * * @param {Promise} promise The Promise that should resolve/reject quickly. * @param {number} delay A delay after which to stop waiting for `aPromise`, in milliseconds. * @param {function} rejection If `aPromise` hasn't resolved/rejected after `delay`, * a value used to construct the rejection. * * @return {Promise} A promise that behaves as `promise`, if `promise` is * resolved/rejected within `delay` ms, or rejects with `rejection()` otherwise. */ Also, please rename `aPromise`, `aMilliseconds`, `aMessage` - prefixing with "a" is a style we now try to avoid in new code. @@ +12,5 @@ > + * Otherwise the returned Promise will get rejected with a message "Timeout: " + aMessage. */ > +this.resolveOrTimeout = function resolveOrTimeout(aPromise, aMilliseconds, aMessage) { > + return new Promise((resolve, reject) => { > + aPromise.then(resolve, reject); > + imported.setTimeout(() => reject("Timeout: " + aMessage), aMilliseconds); You will need to change the value passed to `reject()` to match the above documentation. ::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js @@ +1,1 @@ > +"use strict"; Please add the public domain boilerplate https://www.mozilla.org/MPL/headers/ @@ +1,4 @@ > +"use strict"; > + > +let impPromiseUtils = {}; > +let impTimer = {}; You don't need these. Just import to `this`. @@ +7,5 @@ > + > +// Tests for PromiseUtils.jsm > +function run_test() { > + add_task(test_Promise1); > + add_task(test_Promise2); function run_test() { run_next_test(); } @@ +12,5 @@ > + run_next_test(); > +} > + > +// This generator tests for the case when the passed Promise("p1" here) settels after the delay passed to resolveOrTimeout > +function test_Promise1() { The preferred style is add_task(function* test_foo() { }); Also, the name of this test is not very clear. This should probably be `test_timeout`. @@ +17,5 @@ > + let p1 = new Promise((resolve, reject) => impTimer.setTimeout(() => resolve("p1 is resolved"), 10000)); > + try { > + yield impPromiseUtils.resolveOrTimeout(p1, 300, "Sorry"); > + } > + catch(ex) { Could you make this `} catch (ex) {`? @@ +18,5 @@ > + try { > + yield impPromiseUtils.resolveOrTimeout(p1, 300, "Sorry"); > + } > + catch(ex) { > + do_check_eq(ex, "Timeout: Sorry", "Timeout occured because the passed promise does not settles within time"); These days, we use `Assert.equal` instead of `do_check_eq`. @@ +19,5 @@ > + yield impPromiseUtils.resolveOrTimeout(p1, 300, "Sorry"); > + } > + catch(ex) { > + do_check_eq(ex, "Timeout: Sorry", "Timeout occured because the passed promise does not settles within time"); > + } This is not going to test what you want, as you don't make sure that your code actually launches an exception. You should use `Assert.rejects`, as defined here: http://dxr.mozilla.org/mozilla-central/search?q=proto.rejects&case=true @@ +29,5 @@ > + try { > + yield impPromiseUtils.resolveOrTimeout(p2, 300, "Sorry").then((val) => do_check_eq(val, "p2 is resolved", "The values of p2 is here because p2 settled within time")); > + } > + catch(ex) {} > +} \ No newline at end of file I would like the following tests: - when `promise` resolves quickly, the result resolves quickly; - when `promise` rejects quickly, the result rejects quickly; - when `promise` never resolves/rejects, the result rejects with `rejection()` – let's try this when `rejection()` returns a string, `undefined` or an object; - when `promise` never resolves/rejects and `rejection()` throws an Error, the result rejects with that error; - when we pass something other than a promise as first argument, we get a TypeError; - when we pass something other than a number as second argument, we get a TypeError; - when we pass something other than a function as third argument, we get a TypeError.
Attachment #8510394 -
Flags: review?(dteller) → feedback+
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → akshendra521994
Flags: needinfo?(dteller)
Reporter | ||
Comment 29•9 years ago
|
||
Done. Oh, I forgot: let's make the third argument optional. If it not provided, we should throw `new Error("Promise timeout")` in case of timeout.
Assignee | ||
Comment 30•9 years ago
|
||
*rejection()* is going to a function, which will return the value to be passed as the reason of rejection. And if this *rejection()* throws an error then it will be the reason of rejection. Is what I understoodd correct?
Assignee | ||
Comment 31•9 years ago
|
||
About using Assert.rejects, can you explain a little more why it should be used. Because all the tests are running just fine even without it.
Attachment #8510394 -
Attachment is obsolete: true
Attachment #8511484 -
Flags: review?(dteller)
Assignee | ||
Updated•9 years ago
|
Attachment #8511484 -
Flags: review?(dteller)
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8511484 -
Attachment is obsolete: true
Attachment #8511495 -
Flags: review?(dteller)
Comment 33•9 years ago
|
||
Comment on attachment 8511495 [details] [diff] [review] 1080466.patch Review of attachment 8511495 [details] [diff] [review]: ----------------------------------------------------------------- A drive-by - bug in general, I'm not clear on the use-case for this bug. I've seen the implementation of comment 1 in a number of tests, but I'm yet to come across code that uses this second pattern (and best I can tell, this new function isn't generally suitable for code which wants comment 1). Is there an identified use-case, or is this just a matter of "just incase we need it in the future it will be ready"? ::: toolkit/modules/PromiseUtils.jsm @@ +30,5 @@ > + } > + > + // throw a TypeError if <delay> is not a number > + if (typeof delay != "number" || delay < 0) { > + throw new TypeError("< resolveOrTimeout(promise, delay[, rejection] > 'delay' must be a positive number"); nit: a fair but of trailing whitespace in this patch. @@ +35,5 @@ > + } > + > + var rejectionReason = new Error("Promise Timeout"); > + // if <rejection> provided but is not a function then throw a TypeError > + if (arguments.length > 2 && typeof rejection != "function") { can't we just say "if rejection && typeof..." (or an explicit check for undefined if that floats your boat)? Then something like: rejection = rejection || () => new Error("..."); But - even then, we are unconditionally and immediately calling the rejection function if one is passed - why not just have the optional argument be the rejection reason itself? @@ +50,5 @@ > + } > + > + return new Promise((resolve, reject) => { > + promise.then(resolve, reject); > + setTimeout(() => reject(rejectionReason), delay); I'd expect to see the timer killed on resolution. @@ +51,5 @@ > + > + return new Promise((resolve, reject) => { > + promise.then(resolve, reject); > + setTimeout(() => reject(rejectionReason), delay); > + }); this line needs a dedent
Reporter | ||
Comment 34•9 years ago
|
||
Comment on attachment 8511495 [details] [diff] [review] 1080466.patch Review of attachment 8511495 [details] [diff] [review]: ----------------------------------------------------------------- This gets better with each version. You should really double-check your tests, though, most of them don't do what you expect. ::: toolkit/modules/PromiseUtils.jsm @@ +22,5 @@ > + * > + * @return {Promise} A promise that behaves as `promise`, if `promise` is > + * resolved/rejected within `delay` ms, or rejects with `rejection()` otherwise. > + */ > + resolveOrTimeout : (promise, delay, rejection) => { Nit: Generally, we keep the => notation for anonymous functions, so I'd suggest using `function(promise, delay, rejection)` here. @@ +33,5 @@ > + if (typeof delay != "number" || delay < 0) { > + throw new TypeError("< resolveOrTimeout(promise, delay[, rejection] > 'delay' must be a positive number"); > + } > + > + var rejectionReason = new Error("Promise Timeout"); You should avoid `var` in our code, and rather use `let`. Also, don't create `rejectionReason` if you don't need it, as creating exceptions is rather expensive. Rather, use a default argument for `rejection`, e.g. `function (promise, delay, rejection = new Error("Promise Timeout"))` This will help you simplify your code. @@ +39,5 @@ > + if (arguments.length > 2 && typeof rejection != "function") { > + throw new TypeError("< resolveOrTimeout(promise, delay[, rejection] > 'rejection' must be a function"); > + } > + // or other wise find the reason for rejection using the funtion passed > + else if(arguments.length > 2){ Nit: `} else {` @@ +50,5 @@ > + } > + > + return new Promise((resolve, reject) => { > + promise.then(resolve, reject); > + setTimeout(() => reject(rejectionReason), delay); Ah, right, as Mark mentioned: please call `clearTimeout` on resolution of `promise`. This will not change the result, but it will clear up memory, etc. ::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js @@ +8,5 @@ > + > +// Tests for PromiseUtils.jsm > +function run_test() { > + run_next_test(); > +} Nit: whitespace. @@ +12,5 @@ > +} > + > +/* This generator tests for the case when first argument to resolveOrTimeout > + * that should be a Promise is not a promise. */ > +add_task(function test_argument_not_promise() { You are using a deprecated syntax. These days, it's `function*`. @@ +16,5 @@ > +add_task(function test_argument_not_promise() { > + try { > + yield PromiseUtils.resolveOrTimeout("string", 200); > + } > + catch(ex) { Nit: `} catch (ex) {`. @@ +18,5 @@ > + yield PromiseUtils.resolveOrTimeout("string", 200); > + } > + catch(ex) { > + Assert.equal(ex.message, "< resolveOrTimeout(promise, delay[, rejection] > 'promise' must be a Promise object" > + , "The first argument to resolveOrTimeout is not a promise"); If no exception is raised, this test will also pass. To make sure that the exception is raised, you should use `Assert.throws`. @@ +20,5 @@ > + catch(ex) { > + Assert.equal(ex.message, "< resolveOrTimeout(promise, delay[, rejection] > 'promise' must be a Promise object" > + , "The first argument to resolveOrTimeout is not a promise"); > + } > +}); No need to split all these TypeError tests across so many tasks, just gather them in `function* test_wrong_arguments`. @@ +57,5 @@ > + yield PromiseUtils.resolveOrTimeout(p, 200); > + } > + catch(ex) { > + Assert.equal(ex.message, "Promise Timeout" > + , "The third argument is not given and the returned promise is rejected"); If `resolveOrTimeout` doesn't reject, this test will also pass. To make sure that the rejection happens, you should use `Assert.rejects`. @@ +70,5 @@ > + yield PromiseUtils.resolveOrTimeout(p, 200); > + } > + catch(ex) { > + Assert.equal(ex, "Promise is resolved" > + , "The passed promise resolves quickly"); That test doesn't make sense.
Attachment #8511495 -
Flags: review?(dteller) → feedback+
Assignee | ||
Comment 35•9 years ago
|
||
Attachment #8511495 -
Attachment is obsolete: true
Attachment #8512576 -
Flags: feedback?(dteller)
Reporter | ||
Comment 36•9 years ago
|
||
Comment on attachment 8512576 [details] [diff] [review] 1080466.patch Review of attachment 8512576 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/PromiseUtils.jsm @@ +22,5 @@ > + * > + * @return {Promise} A promise that behaves as `promise`, if `promise` is > + * resolved/rejected within `delay` ms, or rejects with `rejection()` otherwise. > + */ > + resolveOrTimeout : function(promise, delay, rejection) { Let's not propagate undefined stuff, so that should be `rejection = null`. ::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js @@ +17,5 @@ > + let p = new Promise((resolve, reject) => {}); > + > + // for the first argument > + try { > + yield Assert.throws (() => PromiseUtils.resolveOrTimeout("string", 200)); That's not how `Assert.throws` works. See http://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js#12 for some examples of using Assert.throws. @@ +30,5 @@ > + } catch(ex) { > + Assert.equal(ex.name + " : " + ex.message, "second argument <delay> must be a positive number" > + , "The second argument to resolveOrTimeout is not a number"); > + } > + Nit: whitespace.
Attachment #8512576 -
Flags: feedback?(dteller) → feedback+
Assignee | ||
Comment 37•9 years ago
|
||
Attachment #8512576 -
Attachment is obsolete: true
Attachment #8512696 -
Flags: feedback?(dteller)
Reporter | ||
Comment 38•9 years ago
|
||
Comment on attachment 8512696 [details] [diff] [review] 1080466.patch Review of attachment 8512696 [details] [diff] [review]: ----------------------------------------------------------------- We're getting there :) ::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js @@ +11,5 @@ > + run_next_test(); > +} > + > +/* This generator tests for the case when first argument to resolveOrTimeout > + * are not of correct type */ Nit: Can you remove "This generator" from all your comments? Also, this comment is not true anymore. @@ +16,5 @@ > +add_task(function* test_wrong_arguments() { > + let p = new Promise((resolve, reject) => {}); > + > + // for the first argument > + Assert.throws (() => PromiseUtils.resolveOrTimeout("string", 200), /first argument <promise> must be a Promise object/); Nit: No whitespace after `throws`. @@ +36,5 @@ > +/* This generator tests for the case when the passed promise resolves quickly > + * In that case the returned promise also resolves with the same value */ > +add_task(function* test_resolve_quickly() { > + let p = new Promise((resolve, reject) => setTimeout(() => resolve("Promise is resolved"), 20)); > + PromiseUtils.resolveOrTimeout(p, 200).then((val) => Assert.equal(val, "Promise is resolved")); Your code should work, but I would prefer the following, which depends less on our sophisticated machinery for catching uncaught rejections. let result = yield PromiseUtils.resolveOrTimeout(p, 200); Assert.equal(result, "Promise is resolved"); @@ +43,5 @@ > +/* This generator tests for the case when the passed promise rejects quickly > + * In that case the returned promise also rejects with the same value */ > +add_task(function* test_reject_quickly() { > + let p = new Promise((resolve, reject) => setTimeout(() => reject("Promise is rejected"), 20)); > + PromiseUtils.resolveOrTimeout(p, 200).then(null, (val) => Assert.equal(val, "Promise is rejected")); No, this won't work. Here, you should use `Assert.rejects`. @@ +46,5 @@ > + let p = new Promise((resolve, reject) => setTimeout(() => reject("Promise is rejected"), 20)); > + PromiseUtils.resolveOrTimeout(p, 200).then(null, (val) => Assert.equal(val, "Promise is rejected")); > +}); > + > +/* This generator tests for the case when the passed promise doesn't settles Nit: "doesn't settle". @@ +50,5 @@ > +/* This generator tests for the case when the passed promise doesn't settles > + * ans rejection returns a string */ > +add_task(function* test_rejection_function() { > + let p = new Promise((resolve, reject) => {}); > + Nit: Whitespace. @@ +59,5 @@ > + })); > + } catch(ex) { > + Assert.equal(ex, "Rejection returned a string" > + , "The rejection function returned a string"); > + } Since you're already using `Assert.rejects`, this `try/catch` doesn't make sense. Same thing below. @@ +60,5 @@ > + } catch(ex) { > + Assert.equal(ex, "Rejection returned a string" > + , "The rejection function returned a string"); > + } > + Nit: Whitespace.
Attachment #8512696 -
Flags: feedback?(dteller) → feedback+
Assignee | ||
Comment 39•9 years ago
|
||
Attachment #8512696 -
Attachment is obsolete: true
Attachment #8512762 -
Flags: review?(dteller)
Reporter | ||
Comment 40•9 years ago
|
||
Comment on attachment 8512762 [details] [diff] [review] 1080466.patch Review of attachment 8512762 [details] [diff] [review]: ----------------------------------------------------------------- Almost there :) ::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js @@ +23,5 @@ > + Assert.throws(() => PromiseUtils.resolveOrTimeout(p, 200, "string"), /third optional argument <rejection> must be a function/); > +}); > + > +/* Tests for the case when the optional third argument is not provided > + * In that case the retrned promise rejects with a default Error */ Nit: "the returned promise" @@ +26,5 @@ > +/* Tests for the case when the optional third argument is not provided > + * In that case the retrned promise rejects with a default Error */ > +add_task(function* test_optional_third_argument() { > + let p = new Promise((resolve, reject) => {}); > + Assert.rejects(PromiseUtils.resolveOrTimeout(p, 200), /Promise Timeout/); `Assert.rejects` returns a Promise, so you need to call `yield` here and in every other call to `Assert.rejects`. @@ +45,5 @@ > + Assert.rejects(PromiseUtils.resolveOrTimeout(p, 200), /Promise is rejected/); > +}); > + > +/* Tests for the case when the passed promise doesn't settle > + * ans rejection returns a string */ Nit: "and rejection". Also, the comment isn't true anymore. @@ +56,5 @@ > + > + // for rejection returning object > + Assert.rejects(PromiseUtils.resolveOrTimeout(p, 200, () => { > + return {Name:"Promise"}; > + })); Does that work?
Attachment #8512762 -
Flags: review?(dteller) → feedback+
Assignee | ||
Comment 41•9 years ago
|
||
Fingers Crossed :)
Attachment #8512762 -
Attachment is obsolete: true
Attachment #8514310 -
Flags: review?(dteller)
Reporter | ||
Comment 42•9 years ago
|
||
Comment on attachment 8514310 [details] [diff] [review] 1080466_setTimout_for_promise.patch Review of attachment 8514310 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me \o/ Let's run this through the Try server: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b17608191ef
Attachment #8514310 -
Flags: review?(dteller) → review+
Reporter | ||
Comment 43•9 years ago
|
||
By the way, once this bug has landed, you should take a look at bug 1093021, which is more work based on Promise.
Reporter | ||
Comment 44•9 years ago
|
||
This looks good to me. Let's land that patch.
Keywords: checkin-needed
Comment 45•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/26bc4268f868
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][good second bug] → [lang=js][good second bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/26bc4268f868
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good second bug][fixed-in-fx-team] → [lang=js][good second bug]
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•