Add tests for rm removing symlinks - #849
Conversation
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #849 +/- ##
======================================
Coverage 95.8% 95.8%
======================================
Files 34 34
Lines 1262 1262
======================================
Hits 1209 1209
Misses 53 53Continue to review full report at Codecov.
|
Sorry, something went wrong.
nfischer
left a comment
There was a problem hiding this comment.
Mostly good. Thanks for taking this up.
Sorry, something went wrong.
| t.is(result.stderr, 'rm: option not recognized: @'); | ||
| }); | ||
|
|
||
| test('remove symbolic link to a dir without -r fails', t => { |
There was a problem hiding this comment.
This statement is only correct because of the trailing slash:
$ rm link_to_a_dir/ # fails
$ rm link_to_a_dir # succeeds (it removes the link itself, does not affect dir or contentsPlease clarify this in the test name, and please include a more verbose comment explaining the significance of the trailing slash.
Also, we have an existing test which handles the case with no trailing slash and no -r (yay), however that test case needs an assertion. Currently, it asserts that the destination dir still exists, but it should instead assert that the contents of that destination dir still exist. The destination always survives an rm. If you can, please fix that test too.
Sorry, something went wrong.
There was a problem hiding this comment.
Other ways we might test link-handling:
- -r, link to a dir, trailing slash (delete contents and the link)
- -rf, link to a dir, trailing slash (this is already implemented, but I mention it here for completeness)
- -r, link to a dir, no trailing slash (deletes only the link, not the destination, nor the destination's contents)
- -rf, link to a dir, no trailing slash (same as above, probably not necessary to copy this out)
Sorry, something went wrong.
There was a problem hiding this comment.
I've tested the first one and I think the behavior is a little different (linux 4.4.0, bash 4.3.48):
- -r, link to a dir, trailing slash: rm: cannot remove 'link/': Not a directory
Can you tell me if you get the same behavior in bash?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, I see the same behavior on my machine.
Sorry, something went wrong.
There was a problem hiding this comment.
Also, rm -rf linkToDir/ will exit without error, but will remove neither the link nor the target.
Sorry, something went wrong.
| }); | ||
| }); | ||
|
|
||
| test('remove symbolic link', t => { |
There was a problem hiding this comment.
nit: remove symbolic link to file
Does this actually require skipOnWin? I would be surprised if this is so, because remove symbolic link to a dir passes without that.
Also, it might be nice to move this up closer to that test case.
Sorry, something went wrong.
Renames 'remove symbolic link' test to 'remove symbolic link to a file', moves the test near a related test, and adds assertions to the test 'remove symbolic link to a dir'.
|
@nfischer I've added tests for rm -r slash/, rm -rf slash/ rm -r noslash, and rm -rf noslash. I also found that the behavior of rm('-r', 'slash/') was incorrect, so I fixed it. PTAL. |
Sorry, something went wrong.
|
@freitagbr 2 tests fail on Windows, it looks like we should skip them. |
Sorry, something went wrong.
|
My bad, I forgot to add skipOnWin to the new tests. |
Sorry, something went wrong.
| t => { | ||
| utils.skipOnWin(t, () => { | ||
| // the trailing slash signifies that we want to delete the source | ||
| // directory and its contents, which can only be done with the -r flag |
There was a problem hiding this comment.
to delete the source directory and its contents
Can you rephrase this as "delete the contents of the source directory, without removing the source directory itself or the link"?
Sorry, something went wrong.
| const result = shell.rm(`${t.context.tmp}/rm/link_to_a_dir`); | ||
| t.falsy(shell.error()); | ||
| t.is(result.code, 0); | ||
| t.falsy(shell.test('-L', `${t.context.tmp}/rm/link_to_a_dir`)); |
There was a problem hiding this comment.
I think this is already covered by the next line? Or, does existsSync follow links? If there's a reason, can you add a comment?
Sorry, something went wrong.
There was a problem hiding this comment.
existsSync follows links. This pattern is found in several of the tests here, I will clarify them with comments.
Sorry, something went wrong.
|
|
||
| test('rm -r, symlink to a dir, no trailing slash', t => { | ||
| utils.skipOnWin(t, () => { | ||
| const result = shell.rm('-rf', `${t.context.tmp}/rm/link_to_a_dir`); |
There was a problem hiding this comment.
Should be -r not -rf
Sorry, something went wrong.
| // if was directory was referenced through a symbolic link, | ||
| // the contents should be removed, but not the directory itself | ||
| if (fromSymlink) return; | ||
| if (fromSymlink) { |
There was a problem hiding this comment.
As I understand, the issue is that link_to_a_dir refers to something, but link_to_a_dir/ does not (because only directories may be optionally referred to with a trailing slash). But in both cases, link_to_a_dir references a directory, and so I would expect fromSymlink to be in true in both cases.
It sounds like we should rename the variable, or we should add a comment explaining what it actually represents.
Sorry, something went wrong.
Fixes #796
Adds a test for removing a symbolic link to a file, and for failing to remove the contents of a directory through a symbolic link without the -r flag. Improves test coverage from 95.71% to 98.57%.