iframe-proxy

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

nfischer

Mostly good. Thanks for taking this up.

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

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

+  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

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

  // 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

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

freitagbr commented May 10, 2018

Uh oh!

codecov-io commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freitagbr May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfischer commented Jul 4, 2018

Uh oh!

freitagbr commented Jul 12, 2018

Uh oh!

nfischer commented Jul 12, 2018

Uh oh!

freitagbr commented Jul 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented May 10, 2018 •

edited

Loading

freitagbr May 11, 2018 •

edited

Loading