Add tests for rm removing symlinks #849

freitagbr · 2018-05-10T05:27:04Z

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%.

codecov-io · 2018-05-10T06:30:02Z

Codecov Report

Merging #849 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #849   +/-   ##
======================================
  Coverage    95.8%   95.8%           
======================================
  Files          34      34           
  Lines        1262    1262           
======================================
  Hits         1209    1209           
  Misses         53      53

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4733a32...c678839. Read the comment docs.

nfischer

Mostly good. Thanks for taking this up.

nfischer · 2018-05-10T06:16:10Z

test/rm.js

@@ -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 => {


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 contents

Please 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.

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)

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?

Yes, I see the same behavior on my machine.

Also, rm -rf linkToDir/ will exit without error, but will remove neither the link nor the target.

nfischer · 2018-05-10T06:20:21Z

test/rm.js

@@ -308,3 +319,14 @@ test('remove fifo', t => {
    t.is(result.code, 0);
  });
 });
+
+test('remove symbolic link', t => {


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'.

nfischer · 2018-07-04T05:49:29Z

@freitagbr what state is this in?

freitagbr · 2018-07-12T05:25:07Z

@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.

nfischer · 2018-07-12T18:40:40Z

@freitagbr 2 tests fail on Windows, it looks like we should skip them.

freitagbr · 2018-07-12T18:42:30Z

My bad, I forgot to add skipOnWin to the new tests.

nfischer · 2018-07-12T18:44:22Z

test/rm.js

+  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


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"?

nfischer · 2018-07-12T18:48:07Z

test/rm.js

  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`));


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?

existsSync follows links. This pattern is found in several of the tests here, I will clarify them with comments.

nfischer · 2018-07-12T18:52:22Z

test/rm.js

    // 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`);


Should be -r not -rf

nfischer · 2018-07-12T18:58:46Z

src/rm.js

@@ -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) {


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.

test(rm): add tests for removing symlinks

64a682e

freitagbr added the test coverage label May 10, 2018

freitagbr requested a review from nfischer May 10, 2018 05:27

test(rm): skip symlink test on windows

c678839

nfischer requested changes May 10, 2018

View reviewed changes

Rename and move test, add assertions

1438bf1

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 added 2 commits July 11, 2018 22:14

Add tests for rm -r/-f on links to dirs

4167033

Properly handle rm -r on links to dirs with a trailing slash

042d17c

nfischer reviewed Jul 12, 2018

View reviewed changes

freitagbr added 2 commits July 17, 2018 22:21

Rephrase comment for clarity

4639ca9

Use -r in -r test

44ec920

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for rm removing symlinks #849

Add tests for rm removing symlinks #849

Add tests for rm removing symlinks #849

Are you sure you want to change the base?

Add tests for rm removing symlinks #849

Conversation

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment