feat(cp): support update flag when recursing#889
Conversation
test/cp.js
Outdated
| }); | ||
| }); | ||
|
|
||
| test('-Ru no longer ignores -u', t => { |
There was a problem hiding this comment.
nit: -Ru respects the -u flag recursively
test/cp.js
Outdated
| test('-Ru no longer ignores -u', t => { | ||
| const [source, dest] = ['foo', 'bar'].map(s => `test/resources/cp/cp-Ru/${s}/file`); | ||
| // First, update the dest file | ||
| shell.touch(dest); |
There was a problem hiding this comment.
You also need to update the source directory to an older time stamp, right? Otherwise, we'll see race conditions in very-fresh checkouts.
There was a problem hiding this comment.
In a later comment, you've mentioned checking file contents. That would resolve this issue as well.
There was a problem hiding this comment.
Yeah, file contents is fine to use here as well.
test/cp.js
Outdated
| }); | ||
|
|
||
| test('-Ru no longer ignores -u', t => { | ||
| const [source, dest] = ['foo', 'bar'].map(s => `test/resources/cp/cp-Ru/${s}/file`); |
There was a problem hiding this comment.
You should copy to t.context.tmp. Treat test/resources/ as read-only.
|
@joshi-sh can you check the |
|
Running |
|
@joshi-sh it might not be a docs issue, but there might be modified or extra files leftover after running tests + lint + generate docs. Seems to be v8.x.y specific. |
nfischer
left a comment
nfischer
left a comment
There was a problem hiding this comment.
Most comments are optional, but I'd like to see if we can cleanup test setup.
test/cp.js
Outdated
| const destFile = `${destDir}/file`; | ||
| [sourceDir, destDir].forEach(d => shell.mkdir('-p', d)); | ||
| shell.ShellString('foo\n').to(sourceFile); | ||
| shell.ShellString('bar\n').to(destFile); |
There was a problem hiding this comment.
I would like to separate setup from test logic. Two (alternative) suggestions:
- Instead of setting up at runtime, commit this directory structure under
test/resources/, or - Add an empty line between setup code and test logic (possibly add a comment above such as
// Setup code)
You'll need to continue modifying the time stamps at runtime, as you have here.
test/cp.js
Outdated
| shell.ShellString('bar\n').to(destFile); | ||
| // Get the old mtime for dest | ||
| const oldTime = fs.statSync(destFile).mtimeMs; | ||
| // Send the old file to two days ago |
There was a problem hiding this comment.
nit: you can better document this by creating a constant TWO_DAYS_IN_MS.
You could then make this comment more descriptive: // Set the sourceFile to be older than destFile.
test/cp.js
Outdated
| shell.touch('-m', oldTime + (2 * 24 * 60 * 60 * 1000), sourceFile); | ||
| // Now, copy the old dir to the new one | ||
| shell.cp('-Ru', sourceDir, destDir); | ||
| // Get the new mtime for dest |
There was a problem hiding this comment.
nit: I think this comment is leftover from a previous commit (you're not checking mtimes anymore)
test/cp.js
Outdated
| const destDir = `${dir}/new`; | ||
| const destFile = `${destDir}/file`; | ||
| [sourceDir, destDir].forEach(d => shell.mkdir('-p', d)); | ||
| shell.ShellString('foo\n').to(sourceFile); |
There was a problem hiding this comment.
nit: file contents could be more descriptive. How about source file contents and destination file contents?
| }); | ||
| }); | ||
|
|
||
| test('-Ru respects the -u flag recursively (don\'t update newer file)', t => { |
There was a problem hiding this comment.
You might be able to replace this with a template literal to avoid escaping the apostrophe. I can't remember if our lint currently forbids this (see #788). If lint complains, just leave as-is.
Fixed the issue in #896. Please rebase. |
Codecov Report
@@ Coverage Diff @@
## master #889 +/- ##
=======================================
Coverage 95.81% 95.81%
=======================================
Files 34 34
Lines 1266 1266
=======================================
Hits 1213 1213
Misses 53 53
Continue to review full report at Codecov.
|
3 participants
Fixed
cp -Ruignoring-u, and added a test.Fixes #808