Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #405 - Added tests for chmod and fchmod using Promises #458

Merged
merged 4 commits into from Oct 9, 2018
Merged

Fix #405 - Added tests for chmod and fchmod using Promises #458

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4e991d
Added tests for chmod and fchmod using Promises
danielbogomazov Sep 23, 2018
d1d86f0
Remove unrelated files from pull request
danielbogomazov Sep 25, 2018
6e46bd6
Fixed spelling mistake
danielbogomazov Sep 26, 2018
39d8a6d
Removed redundant catch blocks
danielbogomazov Oct 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
64 changes: 64 additions & 0 deletions tests/spec/fs.chmod.spec.js
View file
Open in desktop
Expand Up @@ -71,3 +71,67 @@ describe('fs.chmod, fs.fchmod', function() {
});
});
});

describe('fsPromise.chmod', function() {
beforeEach(util.setup);
afterEach(util.setup);

it('should be a function', function() {
var fsPromise = util.fs().promises;
expect(typeof fsPromise.chmod).to.equal('function');
});

it('should return a promise', function() {
var fsPromise = util.fs().promises;
expect(fsPromise.chmod()).to.be.a('Promise');
});

it('should allow for updating mode of a given file', function() {
var fsPromise = util.fs().promises;

return fsPromise.open('/file', 'w')
Copy link
Contributor

@biskit1 biskit1 Sep 30, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the redundant .catch() blocks.
Thanks for the review.

.then( () => {
return fsPromise.chmod('/file', 0o444);
})
.then( () => {
return fsPromise.stat('/file');
})
.then( stats => {
expect(stats.mode & 0o444).to.equal(0o444);
})
.catch( err => { throw err; });
});
});

describe('fsPromise.fchmod', function() {
beforeEach(util.setup);
afterEach(util.setup);

it('should be a function', function() {
var fsPromise = util.fs().promises;
expect(typeof fsPromise.fchmod).to.equal('function');
});

it('should be a promise', function() {
var fsPromise = util.fs().promises;
expect(fsPromise.fchmod()).to.be.a('Promise');
});

it('should allow for updating mode of a given file', function() {
var fsPromise = util.fs().promises;
var fdesc;

return fsPromise.open('/file', 'w')
.then( fd => {
fdesc = fd;
return fsPromise.fchmod(fd, 0o777);
})
.then( () => {
return fsPromise.fstat(fdesc);
})
.then( stats => {
expect(stats.mode & 0o777).to.equal(0o777);
})
.catch( err => { throw err; });
});
});