feat(articles): Simple test enhancement #1659
Conversation
Simple test enhancement proposed by @jrodenbostel. Eases troubleshooting when Article save fails during test execution. Further reference here: meanjs/generator-meanjs#257 Fixes meanjs#1658
|
looks good |
| @@ -306,7 +306,10 @@ describe('Article CRUD tests', function () { | |||
| var articleObj = new Article(article); | |||
|
|
|||
| // Save the article | |||
| articleObj.save(function () { | |||
| articleObj.save(function (err) { | |||
| if (err) { | |||
There was a problem hiding this comment.
@jrodenbostel If the intention is to catch/handle unexpected (possibly random) errors here, then why not just fail the test when err exists? This test should only pass when all conditions and expectations are met, right?
Maybe I'm thinking about it wrong. I guess we have two options, either we assert that err doesn't exist to fail the test when it does exist, or we pass the err to the callback to propagate the actual error back to the test runner; perhaps, the former solves both?
There was a problem hiding this comment.
My intent was only to make diagnosing test failures easier when the failure is related to the article not being saved. When that save fails, the definitely fails, just not in a clear way.
Nothing crazy, but dealing with that was a enough of a hassle at the time to motivate me to submit this patch.
| articleObj.save(function () { | ||
| articleObj.save(function (err) { | ||
| if (err) { | ||
| done(err); |
There was a problem hiding this comment.
This should really be return done(err); instead.
|
@meanjs/contributors can you guys keep an eye on this PR. if it doesn't get addressed in a week or so let's just re-PR it and merge so we don't have many stale PRs. |
|
@codydaig I just pushed the requested changes. |
|
LGTM 👍 |
|
LGTM @mleanos Look good to you? |
|
👍 |
|
LGTM |
feat(articles): Simple test enhancement (meanjs#1659)
feat(articles): Simple test enhancement
Simple test enhancement proposed by @jrodenbostel. Eases troubleshooting when Article save fails during test execution.
Further reference here: meanjs/generator-meanjs#257
Fixes #1658