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

Use command rm in alias_completion #1738

Merged
merged 3 commits into from
Dec 27, 2020
Merged

Use command rm in alias_completion #1738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5753e4
test: Add test for aliases rm for alias_completion
Dec 13, 2020
1c05d71
alias-completion: Use 'command rm' instead of rm to avoid aliases
Dec 13, 2020
b19c2b9
test: Change alias-completion completion file to capistrano
Dec 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugins/available/alias-completion.plugin.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,5 +91,5 @@ function alias_completion {
echo "$new_completion" >> "$tmp_file"
fi
done < <(alias -p | sed -Ene "s/$alias_regex/\2 '\3' '\4'/p")
source "$tmp_file" && rm -f "$tmp_file"
source "$tmp_file" && command rm -f "$tmp_file"
}; alias_completion
9 changes: 8 additions & 1 deletion test/plugins/alias-completion.plugin.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ load ../../lib/helpers

cite _about _param _example _group _author _version

load ../../completion/available/git.completion
load ../../completion/available/capistrano.completion

@test "alias-completion: See that aliases with double quotes and brackets do not break the plugin" {
alias gtest="git log --graph --pretty=format:'%C(bold)%h%Creset%C(magenta)%d%Creset %s %C(yellow)<%an> %C(cyan)(%cr)%Creset' --abbrev-commit --date=relative"
Expand All @@ -22,3 +22,10 @@ load ../../completion/available/git.completion

assert_success
}

@test "alias-completion: See that having aliased rm command does not output unnecessary output" {
alias rm='rm -v'
Copy link
Contributor

Choose a reason for hiding this comment

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

In order for the test to be useful, we need a way to prove that command rm gets invoked.
A quick idea:

  • Path-munge a test-specific rm that does something like mv $1 $1.rm (would probably need more logic)
  • Make the alias invoke something like false
  • Confirm that file.rm exists
    If the alias gets called, then file.rm will not exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's probably easier to just make the alias to mv the file, and assert that it does not exist afterwards

Copy link
Contributor

Choose a reason for hiding this comment

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

Well we want the test to confirm a few things:

  1. The script section that contains rm was executed
  2. The rm command was invoked
  3. The rm alias was not invoked

Passing the test requires confirming some observable behavior that only exists when all of these are true.

The existing refute_output check for example, is poor because, although it can confirm if #3 fails, it can't confirm that #1 passes.

So by path-munging a custom rm script, you get the ability to create some type of observable outcome that proves it was invoked. Additionally, in this case, having the alias invoke the path-munged command could muddy the test, unless it somehow affects the observed results.

NOTE: I actually don't know if trying to accurately test this behavior is worth the effort

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidpfarrell take a look at #1757, I think this PR addresses this problem

load ../../plugins/available/alias-completion.plugin

refute_output
}