Skip to content

Commit

Permalink
getTimesDiffs updates and tests
Browse files Browse the repository at this point in the history
  • Loading branch information
phated committed Feb 4, 2016
1 parent befd3fc commit 3804e5b
Show file tree
Hide file tree
Showing 2 changed files with 157 additions and 1 deletion.
13 changes: 12 additions & 1 deletion lib/dest/fileOperations/getTimesDiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,20 @@ function getTimesDiff(fsStat, vinylStat) {
return;
}

var atime;
if (isValidDate(vinylStat.atime)) {
atime = vinylStat.atime;
} else {
atime = fsStat.atime;
}

if (!isValidDate(atime)) {
atime = undefined;
}

This comment has been minimized.

Copy link
@erikkemperman

erikkemperman Feb 4, 2016

Member

After the first if/else block, atime will always be valid, I think? So that would mean the second if is never true, which means that this new code does exactly the same as the ternary you removed?

This comment has been minimized.

Copy link
@phated

phated Feb 4, 2016

Author Member

(copy paste of the other answer in tests) So, I just tried to make this happen and was successful. Basically, if you call any of the node fs methods that update time and you give atime an invalid date, it updates the filesystem with an invalid date (terrible, I know) and then if you read it back in with one of the stat methods, it creates the Stats object with an invalid atime. So, I think we need to keep this logic to handle that edge case (our code will actually correct the problem, which is cool).

This comment has been minimized.

Copy link
@erikkemperman

erikkemperman Feb 4, 2016

Member

That is terrible, indeed. Shouldn't have to correct this kind of thing... But good to know!


var timesDiff = {
mtime: vinylStat.mtime,
atime: isValidDate(vinylStat.atime) ? vinylStat.atime : fsStat.atime,
atime: atime,
};

return timesDiff;
Expand Down
145 changes: 145 additions & 0 deletions test/fileOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ var expect = require('expect');

var isOwner = require('../lib/dest/fileOperations/isOwner');
var getModeDiff = require('../lib/dest/fileOperations/getModeDiff');
var getTimesDiff = require('../lib/dest/fileOperations/getTimesDiff');

function noop() {}

Expand Down Expand Up @@ -134,3 +135,147 @@ describe('getModeDiff', function() {
done();
});
});

describe('getTimesDiff', function() {

it('returns undefined if vinyl mtime is not a valid date', function(done) {
var fsStat = {
mtime: new Date(),
};
var vfsStat = {
mtime: new Date(undefined),
};

var result = getTimesDiff(fsStat, vfsStat);

expect(result).toEqual(undefined);

done();
});

it('returns undefined if vinyl mtime & atime are both equal to counterparts', function(done) {
var now = Date.now();
var fsStat = {
mtime: new Date(now),
atime: new Date(now),
};
var vfsStat = {
mtime: new Date(now),
atime: new Date(now),
};

var result = getTimesDiff(fsStat, vfsStat);

expect(result).toEqual(undefined);

done();
});

// TODO: is this proper/expected?
it('returns undefined if vinyl mtimes equals the counterpart and atimes are null', function(done) {

This comment has been minimized.

Copy link
@phated

phated Feb 4, 2016

Author Member

@erikkemperman @piranna is this test proper/expected/correct? Sorry, I amended my commit and lost the comments

var now = Date.now();
var fsStat = {
mtime: new Date(now),
atime: null,
};
var vfsStat = {
mtime: new Date(now),
atime: null,
};

var result = getTimesDiff(fsStat, vfsStat);

expect(result).toEqual(undefined);

done();
});

it('returns a diff object if mtimes do not match', function(done) {
var now = Date.now();
var then = now - 1000;
var fsStat = {
mtime: new Date(now),
};
var vfsStat = {
mtime: new Date(then),
};
var expected = {
mtime: new Date(then),
atime: undefined,
};

var result = getTimesDiff(fsStat, vfsStat);

expect(result).toEqual(expected);

done();
});

it('returns a diff object if atimes do not match', function(done) {
var now = Date.now();
var then = now - 1000;
var fsStat = {
mtime: new Date(now),
atime: new Date(now),
};
var vfsStat = {
mtime: new Date(now),
atime: new Date(then),
};
var expected = {
mtime: new Date(now),
atime: new Date(then),
};

var result = getTimesDiff(fsStat, vfsStat);

expect(result).toEqual(expected);

done();
});

it('returns the fs atime if the vinyl atime is invalid', function(done) {
var now = Date.now();
var fsStat = {
mtime: new Date(now),
atime: new Date(now),
};
var vfsStat = {
mtime: new Date(now),
atime: new Date(undefined),
};
var expected = {
mtime: new Date(now),
atime: new Date(now),
};

var result = getTimesDiff(fsStat, vfsStat);

expect(result).toEqual(expected);

done();
});

// TODO: is this proper/expected?
it('makes atime diff undefined if fs and vinyl atime are invalid', function(done) {

This comment has been minimized.

Copy link
@phated

phated Feb 4, 2016

Author Member

@erikkemperman @piranna is this test proper/expected/correct? Sorry, I amended my commit and lost the comments - I updated this implementation to return undefined in the case of an invalid date because I don't think Date.now() is correct and this is easier to test.

This comment has been minimized.

Copy link
@erikkemperman

erikkemperman Feb 4, 2016

Member

I'm not sure this test is sane -- isn't this mocking a fsStat that never actually happens? I.e. is the atime from fs.stat ever invalid?

This comment has been minimized.

Copy link
@phated

phated Feb 4, 2016

Author Member

So, I just tried to make this happen and was successful. Basically, if you call any of the node fs methods that update time and you give atime an invalid date, it updates the filesystem with an invalid date (terrible, I know) and then if you read it back in with one of the stat methods, it creates the Stats object with an invalid atime. So, I think we need to keep this logic to handle that edge case (our code will actually correct the problem, which is cool).

This comment has been minimized.

Copy link
@piranna

piranna Feb 4, 2016

Contributor

Shouldn't we open an issue on Node.js to fix that? It should never write an invalid value here...

This comment has been minimized.

Copy link
@phated

phated Feb 4, 2016

Author Member

This should probably be reported to node; However, I only use node 0.10.x so I'm not sure if it has been fixed on another version. No matter if it is reported and fixed in a new version, it won't be fixed in the 0.10.x branch and we support that far back. Furthermore, one of your PRs has already been introduced to vinyl-fs to work around this very scenario, so I think we already knew about it but maybe not this in-depth.

var now = Date.now();
var fsStat = {
mtime: new Date(now),
atime: new Date(undefined),
};
var vfsStat = {
mtime: new Date(now),
atime: new Date(undefined),
};
var expected = {
mtime: new Date(now),
atime: undefined,
};

var result = getTimesDiff(fsStat, vfsStat);

expect(result).toEqual(expected);

done();
});
});

0 comments on commit 3804e5b

Please sign in to comment.