Fix promise assertions #360
Conversation
| @@ -165,35 +172,49 @@ Test.prototype._end = function (err) { | |||
| Test.prototype.exit = function () { | |||
| var self = this; | |||
|
|
|||
| // calculate total time spent in test | |||
| this.duration = globals.now() - this._timeStart; | |||
| function checkAssertLength() { | |||
There was a problem hiding this comment.
I'm not sure if this should be here, or it should be a method, or if it should be an external function which accepts self. Thoughts?
There was a problem hiding this comment.
I would make it a private instance method (prefixed with _). The function checks if assertion count matches plan count. I think the name could be clearer.
ariporad
force-pushed
the
fix-promise-assertions
branch
from
3146d33 to
26f6fa9
Compare
| @@ -25,11 +25,18 @@ function Test(title, fn) { | |||
|
|
|||
| this.title = title || fnName(fn) || '[anonymous]'; | |||
| this.fn = isGeneratorFn(fn) ? co.wrap(fn) : fn; | |||
| this.assertCount = 0; | |||
| this.assertPromises = []; | |||
There was a problem hiding this comment.
assertPromises -> assertions
|
@ariporad - Good work! A few minor nits, but overall very well done! |
|
@jamestalmage: Should be fixed now. |
|
@jamestalmage: Hang on a second actually, delay doesn't like node@<4.0.0 |
It's |
|
@sindresorhus: Yeah, I was trying to avoid that, but it's not that big a deal. |
| @@ -93,6 +93,7 @@ | |||
| "core-assert": "^0.1.0", | |||
| "debug": "^2.2.0", | |||
| "deeper": "^2.1.0", | |||
| "delay": "^1.2.0", | |||
There was a problem hiding this comment.
This should be a dev dependency.
ariporad
force-pushed
the
fix-promise-assertions
branch
2 times, most recently
from
2f67fb4 to
c7b0154
Compare
|
|
||
| if (!this.ended) { | ||
| this.ended = true; | ||
| Test.prototype.exit = function () { | ||
|
|
There was a problem hiding this comment.
@sindresorhus: Fixed
ariporad
force-pushed
the
fix-promise-assertions
branch
from
75ca556 to
8a37069
Compare
|
Ok, @jamestalmage, @sindresorhus: Everything should be fixed now. |
|
LGTM, great stuff! |
|
Let's get sindresorhus/delay#8 merged and published. Then use the updated function here. Once that happens, I think we are good to go. |
ariporad
force-pushed
the
fix-promise-assertions
branch
from
8a37069 to
149ceb3
Compare
|
Ok, I just rebased onto master. |
|
@ariporad [email protected] is now out. Can you update tests to use it? Other than that, this looks good to land. |
…xits, but is correct once all assertions have resolved
ariporad
force-pushed
the
fix-promise-assertions
branch
from
149ceb3 to
0063076
Compare
|
@sindresorhus: Ok. Fixed, and rebased onto master. Ready for merging. |
|
Landed. Really dig this fix @ariporad :) |
Hi All,
Here's the fix for #309, It has a good number of tests, but let me know if there's anything I should change.