Conversation
|
Same bug from #673 is affecting the windows tests here. Let's see if we can implement a fix. |
src/cp.js
Outdated
| if (path.relative(src, thisDest) === '') { | ||
| // a file cannot be copied to itself, but we want to continue copying other files, | ||
| // so save the error for later | ||
| identicalFiles.push("'" + thisDest + "' and '" + src + "' are the same file"); |
There was a problem hiding this comment.
Use common.error() here, with the continue: true option.
There was a problem hiding this comment.
Will this still set the result.stderr? And shell.error() to true?
There was a problem hiding this comment.
It sets result.stderr to the error output (it's a += operation, so it all collects up). shell.error() is some truthy value (it's currently the same string as result.stderr, but this is no longer guaranteed as part of the API and it may later change to an error code).
src/cp.js
Outdated
| common.error(err, { continue: cont }); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove this hunk, it shouldn't be necessary
| }); | ||
|
|
||
| test('copy file to same location', t => { | ||
| const result = shell.cp('resources/file1', 'resources'); |
There was a problem hiding this comment.
Does it make sense to also write a test case for shell.cp('resources/file1', 'resources/file1') (copy a file to its same name)?
There was a problem hiding this comment.
Yes, so I added a test for this.
| t.is(result.code, 1); | ||
| t.is( | ||
| result.stderr, | ||
| "cp: 'resources/file1' and 'resources/file1' are the same file\n" + |
There was a problem hiding this comment.
I think we should just check t.truthy(result.stderr) for now, with a TODO to update it later. Please tag issue #6 in the TODO.
There was a problem hiding this comment.
This test was fixed by rebasing off of master.
There was a problem hiding this comment.
I meant 681. But this comment can be ignored now 😄
There was a problem hiding this comment.
I figured. Once this is approved it should be good to merge.
da7a952 to
311ed4d
Compare
Codecov Report
@@ Coverage Diff @@
## master #679 +/- ##
==========================================
+ Coverage 94.32% 94.33% +0.01%
==========================================
Files 33 33
Lines 1180 1183 +3
==========================================
+ Hits 1113 1116 +3
Misses 67 67
Continue to review full report at Codecov.
|
If
cpis passed a destination that is location of a source file, in effect, copying a file to itself, the source file will become empty. This prevents this situation form happening by verifying that the destination is a different location than the source.Fixes #678