Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Add tests for rm removing symlinks #849
Conversation
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.
|
|
Mostly good. Thanks for taking this up. |
| @@ -57,6 +57,17 @@ test('invalid option', t => { | |||
| t.is(result.stderr, 'rm: option not recognized: @'); | |||
| }); | |||
|
|
|||
| test('remove symbolic link to a dir without -r fails', t => { | |||
nfischer
May 10, 2018
Member
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.
nfischer
May 10, 2018
Member
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)
freitagbr
May 11, 2018
•
Author
Contributor
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?
nfischer
Jul 10, 2018
Member
Also, rm -rf linkToDir/ will exit without error, but will remove neither the link nor the target.
| @@ -308,3 +319,14 @@ test('remove fifo', t => { | |||
| t.is(result.code, 0); | |||
| }); | |||
| }); | |||
|
|
|||
| test('remove symbolic link', t => { | |||
nfischer
May 10, 2018
Member
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.
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'.
|
@freitagbr what state is this in? |
|
@nfischer I've added tests for |
|
@freitagbr 2 tests fail on Windows, it looks like we should skip them. |
|
My bad, I forgot to add |
| 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 |
nfischer
Jul 12, 2018
Member
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"?
| 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`)); |
nfischer
Jul 12, 2018
Member
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?
freitagbr
Jul 18, 2018
Author
Contributor
existsSync follows links. This pattern is found in several of the tests here, I will clarify them with comments.
| // Both the link and original dir should remain, but contents are deleted | ||
| t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`)); | ||
| t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`)); | ||
| t.falsy(fs.existsSync(`${t.context.tmp}/rm/a_dir/a_file`)); | ||
| }); | ||
| }); | ||
|
|
||
| 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`); |
| @@ -45,7 +45,12 @@ function rmdirSyncRecursive(dir, force, fromSymlink) { | |||
|
|
|||
| // if was directory was referenced through a symbolic link, | |||
| // the contents should be removed, but not the directory itself | |||
| if (fromSymlink) return; | |||
| if (fromSymlink) { | |||
nfischer
Jul 12, 2018
Member
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.
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
-rflag. Improves test coverage from 95.71% to 98.57%.