Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests for rm removing symlinks #849

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add tests for rm removing symlinks #849

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
64a682e
test(rm): add tests for removing symlinks
freitagbr May 10, 2018
c678839
test(rm): skip symlink test on windows
freitagbr May 10, 2018
1438bf1
Rename and move test, add assertions
freitagbr May 11, 2018
4167033
Add tests for rm -r/-f on links to dirs
freitagbr May 11, 2018
042d17c
Properly handle rm -r on links to dirs with a trailing slash
freitagbr Jul 12, 2018
4639ca9
Rephrase comment for clarity
freitagbr Jul 18, 2018
44ec920
Use -r in -r test
freitagbr Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions test/rm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand 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 => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@freitagbr freitagbr May 11, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see the same behavior on my machine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

utils.skipOnWin(t, () => {
const result = shell.rm(`${t.context.tmp}/rm/link_to_a_dir/`);
t.truthy(shell.error());
t.is(result.code, 1);
t.is(result.stderr, 'rm: path is a directory');
t.truthy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`));
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`));
});
});

//
// Valids
//
Expand Down Expand Up @@ -308,3 +319,14 @@ test('remove fifo', t => {
t.is(result.code, 0);
});
});

test('remove symbolic link', t => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

utils.skipOnWin(t, () => {
t.truthy(shell.test('-L', `${t.context.tmp}/link`));
const result = shell.rm(`${t.context.tmp}/link`);
t.falsy(shell.error());
t.is(result.code, 0);
t.falsy(shell.test('-L', `${t.context.tmp}/link`));
t.falsy(fs.existsSync(`${t.context.tmp}/link`));
});
});