Fix AVA hanging when test is used incorrectly
#600
Conversation
|
By analyzing the blame information on this pull request, we identified @Carnubak and @BarryThePenguin to be potential reviewers |
| if (typeof test.fn === 'undefined') { | ||
| throw new Error('Test must be specified'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe this check would be better suited in lib/runner.js#L52-L77
|
Just tried it, also works, and I wouldn't have to mess with |
|
@sotojuan yes, please add the guards in the runner. |
|
Please make sure to rebase on |
|
Um, don't think I did anything to cause the failing in 0.12. When I test with 0.12 in my computer, everything passes. |
|
LGTM |
| @@ -65,6 +65,10 @@ optionChain(chainableMethods, function (opts, title, fn) { | |||
| throw new TypeError('Expected a function. Use `test.todo()` for tests without a function.'); | |||
| } | |||
|
|
|||
| if (typeof fn === 'undefined') { | |||
There was a problem hiding this comment.
Maybe typeof fn !== 'function'?
| @@ -65,6 +65,10 @@ optionChain(chainableMethods, function (opts, title, fn) { | |||
| throw new TypeError('Expected a function. Use `test.todo()` for tests without a function.'); | |||
| } | |||
|
|
|||
| if (typeof fn !== 'function') { | |||
| throw new TypeError('Expected a function. Use `test.todo()` for tests without a function.'); | |||
There was a problem hiding this comment.
You now have the exact same error defined twice. Put it in a variable and reuse it.
|
@sotojuan could you please add a comment when you push changes? There are no notifications for pushes. |
| @@ -65,6 +65,10 @@ optionChain(chainableMethods, function (opts, title, fn) { | |||
| throw new TypeError('Expected a function. Use `test.todo()` for tests without a function.'); | |||
There was a problem hiding this comment.
Instead of adding a new error message, why don't we just remove opts.skipped && from here? Unless I'm missing something? // @novemberborn
There was a problem hiding this comment.
Yea that should be fine.
Would be good to have a test for this new behavior as well.
|
@novemberborn Sorry about that, I always forget! I'll make the changes and add the tests later today :-) |
|
@novemberborn ping 🐤 |
|
Thanks @sotojuan :) |
Fixes #574.
This is just what works now, I am sure there's a better way. Stack trace is ugly but it's better than hanging. I also had to update some
TestCollectiontest stuff so those could still pass.Just wanted to share my approach!