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

Issue #436 - Implement basic fs.copyFile tests without flags #481

Closed
wants to merge 2 commits into from
Closed

Issue #436 - Implement basic fs.copyFile tests without flags #481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4bd604
Added tests for future fs.copyFile implementation and adjusted index.…
Mordax Sep 23, 2018
4c4ffd4
Fixed lint issues in copyFile test
Mordax Sep 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ require('./spec/errors.spec');
require('./spec/fs.shell.spec');
require('./spec/fs.chmod.spec');
require('./spec/fs.chown.spec');
require('./spec/fs.copyFile.spec');

// Filer.FileSystem.providers.*
require('./spec/providers/providers.spec');
Expand Down
52 changes: 52 additions & 0 deletions tests/spec/fs.copyFile.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
var util = require('../lib/test-utils.js');
var expect = require('chai').expect;

describe('fs.copyFile', function(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add .skip() to this, since we know we can't pass these tests yet.

https://mochajs.org/#inclusive-tests

beforeEach(function(done){
util.setup(function() {
var fs = util.fs();
fs.writeFile('/srcfile', 'This is a src file.', function(error){
if(error) throw error;
fs.writeFile('/destfile', 'This is a dest file.', function(error){
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend isolating the creation of the /destfile to the test case that requires it to limit global state in the tests, especially if it's only used in one test. If it becomes necessary for multiple test cases, we can always move it back into the beforeEach() step.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, please move all these writeFile calls to the individual tests that need them. It's best if tests have as few pre conditions as possible, and everything gets done in the test itself.

if(error) throw error;
done();
});
});
});
});
afterEach(util.cleanup);

it('should be a function', function() {
var fs = util.fs();
expect(fs.copyFile).to.be.a('function');
});

it('should return an error if the src path does not exist', function(done){
var fs = util.fs();
var src = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is prone to being incorrect changed in the future, since it looks like you just forgot to initialize your variable. You need to document that you meant to do this. A few options:

  1. var src; // intentionally left undefined
  2. fs.copyFile(undefined /* intentionally undefined */, '/dest.txt', function(error) {

var dest = 'dest.txt';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consistency - here you declare dest as a variable, but on line 41 you use the filename directly in the function call. Since the destination filename is so short, I'd suggest putting it inline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, inline this. Also, paths in Filer must be absolute: /dest.txt


fs.copyFile(src, dest, function(error){
expect(error).to.exist;
expect(error.code).to.equal('ENOENT');
done();
});

});

it('should copy file successfully', function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This tests if /destfile has been overwritten with the contents of /srcfile, which is great! What about the case where /destfile does not already exist?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add some extra info to line 37 to indicate ...when destination does not exist

var fs = util.fs();
var src = 'This is a src file.';

fs.copyFile('/srcfile', '/destfile', function(error) {
if(error) throw error;

fs.readFile('/destfile', function(error, data){
expect(error).not.to.exist;
expect(data).to.equal(src);
done();
});
});
});

});
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: file should end with a blank line