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 #399 - added test cases for appendFile with promises #401

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@JoshuaRM
Copy link

commented Sep 18, 2018

New test cases have been added to the AppendFile tester, and updated to test using the fs.promises.appendFile method. This method now passes the following tests while using promises:

  •  should be a function
    
  •  should append a utf8 file without specifying utf8 in appendFile
    
  •  should append a utf8 file with "utf8" option to appendFile
    
  •  should append a utf8 file with {encoding: "utf8"} option to appendFile
    
  •  should append a binary file
    
  •  should follow symbolic links
    
  •  should work when file does not exist, and create the file
    

All of these test are successfully passed while using promises

@alexander-ponomaroff
Copy link
Contributor

left a comment

Created by accident, full comment below this one!

@alexander-ponomaroff
Copy link
Contributor

left a comment

Hello Joshua,

Excellent job on creating testing for fs.promises.appendFile() method. The issue that you filed ( #399 ) described the tests that you performed well, and your issue was not duplicated from another student. In your code, you covered all the tests specified in your issue. I ran your code to ensure that everything works as expected, and it does indeed work very nicely. Overall, very well done.

There is just one thing that I suggest changing and I will include some snippets of your code below, explaining what to change.

fs.promises.appendFile('/myfile', more, { encoding: 'utf8' })
.then(function() {
fs.promises.readFile('/myfile', { encoding: 'utf8' })
.then(function(data, error)

This comment has been minimized.

Copy link
@alexander-ponomaroff

alexander-ponomaroff Sep 28, 2018

Contributor

The error object needs to be passed into the catch statement. It never gets passed into the then statement, because it only takes one parameter, which is data. In your case the error object will not exist.

.then(function()
{
fs.promises.readFile('/mybinaryfile', 'ascii')
.then(function(data, error)

This comment has been minimized.

Copy link
@alexander-ponomaroff

alexander-ponomaroff Sep 28, 2018

Contributor

Same here.

.then(function()
{
fs.promises.readFile('/myFileLink', 'utf8')
.then(function(data, error)

This comment has been minimized.

Copy link
@alexander-ponomaroff

alexander-ponomaroff Sep 28, 2018

Contributor

Same here.

@jtj9817
Copy link

left a comment

The planned changes detailed in issue #399 were implemented successfully and I ran your branch changes on my comp to see if there's going to be any issues but I've encountered none. Good job! Although one recommendation that I can suggest, having also had issues with UTF-8 encoding and decoding in another software, is that you should try to use Japanese/Korean/Chinese characters in the string for the method that you want to test and see if it still works as those set of characters is what the UTF-8 encoding is supposed to cover compared to other characters encoding.

});
});
});
});

This comment has been minimized.

Copy link
@mphan6

mphan6 Sep 29, 2018

Hello,
Just wanted to leave a couple comments.

  1. Git is saying that it's expecting a newline after your commits.
  2. Also, for the sake of how I've seen everyone else using promises (style?), it seems the fs variable is being set with util.fs().promises so you wouldn't need to have promises after your fs variable repetitively.
    Not sure the second warrants a change, but the first might haha.
@humphd
Copy link
Contributor

left a comment

I started reviewing this, but you need to correct the same thing in all your tests. Can you see my comments, fix that in all tests, and then I'll re-review?

Thanks, this is looking good.

beforeEach(function(done) {
util.setup(function() {
var fs = util.fs();
fs.promises.writeFile('/myfile', 'This is a file.', { encoding: 'utf8' })

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Get rid of the done arg on line 131, and just return the promise like this:

util.setup(() => util.fs().promises.writeFile('/myfile', 'This is a file'));

fs.promises.appendFile('/myfile', more)
.then(function()
{

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Move your { to the same line as the function.

done();
});
})
.catch(function(error)

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

You can get rid of this I think, just let the test deal with the failed case.

expect(fs.promises.appendFile).to.be.a('function');
});

it('should append a utf8 file without specifying utf8 in appendFile', function(done) {

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Get rid of done here.

var contents = 'This is a file.';
var more = ' Appended.';

fs.promises.appendFile('/myfile', more)

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Add a return to the front of this, so you return this promise (instead of needing/using done).

fs.readFile('/myfile', 'utf8', function(error, data) {
expect(error).not.to.exist;
expect(data).to.equal(contents + more);
done();

This comment has been minimized.

Copy link
@humphd

humphd Oct 9, 2018

Contributor

Get rid of this.

humphd added a commit to humphd/filer that referenced this pull request Dec 11, 2018

@humphd

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

Finished and landed in d1dd5fe. Make sure you finish what you start.

@humphd humphd closed this Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.