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.

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

fix issue#392-- add test for promises.rename() to rename existing file #416

Open
wants to merge 5 commits into
base: master
from

Conversation

@Michael-Overall
Copy link

Michael-Overall commented Sep 20, 2018

Adds test for promises.rename() to tests/spec/fs.rename,spec.js to check renaming an existing file.

Closely mimics logic for an existing test that checks if the the Async rename() function renames an existing file (so no fs.promises functions are used for open(), close(), or stat() ).

Closes #392

/cc @humphd

Copy link
Contributor

humphd left a comment

I left a few comments on this. It's looking good, and I think we can make it even better.

fs.close(fd, function(error) {
if(error) throw error;

fs.promises.rename('/myfile', '/myotherfile').then(

This comment has been minimized.

Copy link
@humphd

humphd Sep 20, 2018

Contributor

I think it's confusing that you're mixing callbacks with promises in this test. Since you're testing the promise version, can you switch all these over to use promises instead?

This comment has been minimized.

Copy link
@Michael-Overall

Michael-Overall Sep 27, 2018

Author

Replaced all async functions with their promise equivalents (commit: 5abefb6). I also noticed later that my IDE added some spaces after all occurrences of "function" and "if" so this was corrected in 98611f4

if(error) throw error;

fs.promises.rename('/myfile', '/myotherfile').then(
function(){

This comment has been minimized.

Copy link
@humphd

humphd Sep 20, 2018

Contributor

With promises, we should be able to get rid of this hack, and instead use Promise.all()

This comment has been minimized.

Copy link
@Michael-Overall

Michael-Overall Sep 27, 2018

Author

Implemented Promise.all() version in 5abefb6. Using promises certainly cleaned up the code.

var complete2 = false;
var fs = util.fs();

function maybeDone() {

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Instead of doing things this way, with a done callback and a bunch of booleans to track what is and isn't finished, we can just rely on Promises to do it. Change your test code like this:

  1. Remove the done callback arg on line 155. We won't use it (see 2.)
  2. Add a return to the front of line 167 below, so you are returning your promise. This will tell it when the test is done.
  3. See my inline changes below

fs.promises.open('/myfile', 'w+')
.then((fd)=>fs.promises.close(fd))
.then(fs.promises.rename('/myfile', '/myotherfile'))

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Should be:

.then(() => fs.promises.rename('/myfile', '/myotherfile'))
fs.promises.open('/myfile', 'w+')
.then((fd)=>fs.promises.close(fd))
.then(fs.promises.rename('/myfile', '/myotherfile'))
.then(Promise.all(fs.promises.stat('/myfile')

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor
.then(() => fs.promises.stat('/myfile')
.then((fd)=>fs.promises.close(fd))
.then(fs.promises.rename('/myfile', '/myotherfile'))
.then(Promise.all(fs.promises.stat('/myfile')
.then( (result)=> expect(result).not.to.exist, (error) => expect(error).to.exist)

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor
.catch(error => expect(error).to.exist)
.then(() => fs.promises.stat('/myotherfile'))
...and so on
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.