Fix #405 - Added tests for chmod and fchmod using Promises #458
Conversation
There was a problem hiding this comment.
Hey, Great Job with the tests!
One thing I noticed is that you changed the .gitignore and package.json as well. You didn't mention that in your pull request, so I am guessing you did it by accident.
When you git add , just add the file that you want to change.
.gitignore
Outdated
| @@ -4,6 +4,7 @@ bower_components | |||
| *~ | |||
| dist/filer-issue225.js | |||
| .vscode | |||
| .DS_Store | |||
package.json
Outdated
| @@ -40,7 +40,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "chai": "^4.1.2", | |||
| "eslint": "^5.0.1", | |||
| "eslint": "^5.6.0", | |||
|
@coreyjjames |
| it('should allow for updating mode of a given file', function() { | ||
| var fsPromise = util.fs().promises; | ||
|
|
||
| return fsPromise.open('/file', 'w') |
There was a problem hiding this comment.
I'm wondering about how you've nested these .then() and .catch() statements.
Is there a better way these can be nested?
Maybe instead of doing the return within return, just call the functions and have the .then() and .catch() for that function nested separately?
i.e
var fsPromise = util.fs().promises;
return fsPromise.open('/file', 'w')
.then(()=>{
fsPromise.chmod('/file', 0o444)
.then(...)
.catch(...);
})
.catch(...);
I'm not sure what's considered best practice, or if it there is even a 'better' or 'worse' way to do it at all...
There was a problem hiding this comment.
Reading this article on promise chaining leads me to believe that this sort of nesting is fine. However, I think in this case if you have a .catch() that throws an error, any proceeding .then() is never run, and it will just jump to the next .catch(). (see rethrowing portion of the article)
There was a problem hiding this comment.
I've removed the redundant .catch() blocks.
Thanks for the review.
Added test cases for chmod and fchmod using Promises
Fix issue #405
Tests:
Note
I separated the chmod and fchmod tests to pinpoint any possible future problems. If we use the non-promise test model, it will only report on fchmod being broken - even if both functions break (since we test for fchmod before chmod). We should consider splitting up the non-promises tests too. Maybe put fchmod into its own file?
Also, should the mkdir test case be moved to the mkdir test file?
Even though the main point of the test is to check whether the correct mode is being set by the function, it makes more sense to me to place a mkdir test in a mkdir test file.