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

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

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Mordax

Mordax commented Sep 24, 2018

Closes #436 kind of, implements non-flag testing for future fs.copyFile implementation.

Checks whether it's a function, checks if error is thrown if source that is being copied doesn't exist, checks if file was copied successfully.

@Mordax

This comment has been minimized.

Mordax commented Sep 24, 2018

Travis is supposed to fail due to no fs.copyFile implementation, just fyi.

@Mordax Mordax changed the title from Implement basic fs.copyFile tests without flags to Implement basic fs.copyFile tests without flags - Issue #436 Sep 24, 2018

@Mordax Mordax changed the title from Implement basic fs.copyFile tests without flags - Issue #436 to Issue #436 - Implement basic fs.copyFile tests without flags Sep 24, 2018

@0xazure

Thanks for adding these much-needed tests! Adding to our safety net for refactoring is super appreciated.

Just a couple comments and minor nits, and looking forward to seeing the implementation.

});
it('should copy file successfully', function(done) {

This comment has been minimized.

@0xazure

0xazure Sep 24, 2018

Contributor

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?

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

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

});
});
});

This comment has been minimized.

@0xazure

0xazure Sep 24, 2018

Contributor

nit: file should end with a blank line

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){

This comment has been minimized.

@0xazure

0xazure Sep 24, 2018

Contributor

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.

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

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.

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

This comment has been minimized.

@0xazure

0xazure Sep 24, 2018

Contributor

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.

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

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

var util = require('../lib/test-utils.js');
var expect = require('chai').expect;
describe('fs.copyFile', function(){

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

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

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

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){

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

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.

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

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

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

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

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

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) {
});
it('should copy file successfully', function(done) {

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment