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

Show file tree
Hide file tree
Changes from all 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
7 changes: 6 additions & 1 deletion src/rm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

if (!force) {
common.error("cannot remove '" + dir + "': Not a directory");
}
return;
}

// Now that we know everything in the sub-tree has been deleted, we can delete the main directory.
// Huzzah for the shopkeep.
Expand Down
76 changes: 73 additions & 3 deletions test/rm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,14 @@ test('file does not exist', t => {
t.is(result.stderr, 'rm: no such file or directory: asdfasdf');
});

test('cannot delete a directoy without recursive flag', t => {
test('cannot delete a directory without recursive flag', t => {
const result = shell.rm(`${t.context.tmp}/rm`);
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`));
const contents = fs.readdirSync(t.context.tmp);
t.true(contents.length > 0);
});

test('only an option', t => {
Expand All @@ -57,6 +60,25 @@ test('invalid option', t => {
t.is(result.stderr, 'rm: option not recognized: @');
});

test(
'cannot remove a symbolic link to a directory with trailing slash without -r flag',
t => {
utils.skipOnWin(t, () => {
// the trailing slash signifies that we want to delete the contents of the source directory,
// without removing the source directory itself or the link, which can only be done with the
// -r flag
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`));
const contents = fs.readdirSync(`${t.context.tmp}/rm/a_dir`);
t.true(contents.length > 0);
});
}
);

//
// Valids
//
Expand Down Expand Up @@ -259,27 +281,75 @@ test(
}
);

test('remove symbolic link to a file', 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`));
t.truthy(fs.existsSync(`${t.context.tmp}/file1`));
});

test('remove symbolic link to a dir', t => {
t.truthy(shell.test('-L', `${t.context.tmp}/rm/link_to_a_dir`));
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`));
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

t.falsy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`));
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`));
});

test('rm -rf on a symbolic link to a dir deletes its contents', t => {
test('rm -r, symlink to a dir, trailing slash', t => {
utils.skipOnWin(t, () => {
const result = shell.rm('-r', `${t.context.tmp}/rm/link_to_a_dir/`);
t.truthy(shell.error());
t.is(result.code, 1);
t.is(result.stderr, `rm: cannot remove '${t.context.tmp}/rm/link_to_a_dir/': Not a directory`);
// 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 -rf, symlink to a dir, trailing slash', t => {
utils.skipOnWin(t, () => {
const result = shell.rm('-rf', `${t.context.tmp}/rm/link_to_a_dir/`);
t.falsy(shell.error());
t.is(result.code, 0);

// 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('-r', `${t.context.tmp}/rm/link_to_a_dir`);
t.falsy(shell.error());
t.is(result.code, 0);
// The link should be deleted, but the dir and contents remain
t.falsy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`));
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`));
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir/a_file`));
});
});

test('rm -rf, symlink to a dir, no trailing slash', t => {
utils.skipOnWin(t, () => {
const result = shell.rm('-rf', `${t.context.tmp}/rm/link_to_a_dir`);
t.falsy(shell.error());
t.is(result.code, 0);
// The link should be deleted, but the dir and contents remain
t.falsy(fs.existsSync(`${t.context.tmp}/rm/link_to_a_dir`));
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir`));
t.truthy(fs.existsSync(`${t.context.tmp}/rm/a_dir/a_file`));
});
});

test('remove broken symbolic link', t => {
utils.skipOnWin(t, () => {
t.truthy(shell.test('-L', `${t.context.tmp}/rm/fake.lnk`));
Expand Down