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.

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

Use command rm in alias_completion #1738

Merged
merged 3 commits into from
Dec 27, 2020
Merged

Conversation

NoahGorny
Copy link
Member

Closes #1354

@NoahGorny NoahGorny marked this pull request as ready for review December 13, 2020 23:26
@NoahGorny
Copy link
Member Author

Works, but the test always passes, as alias-completion exists early, which makes it harder to test it

@davidpfarrell
Copy link
Contributor

Does it exit early because there are no completions ie:

 (( ${#completions[@]} == 0 )) && return 0

?

If that were so, then if feels like the tests above it aren't really testing anything?

@NoahGorny
Copy link
Member Author

Does it exit early because there are no completions ie:

 (( ${#completions[@]} == 0 )) && return 0

?

If that were so, then if feels like the tests above it aren't really testing anything?

Yes.. Although I am sure I made it fail the test someway.. This needs to be fixed

@NoahGorny
Copy link
Member Author

@davidpfarrell The completion file I loaded was not a good one, replaced with a simpler completion file and now it should work

@NoahGorny NoahGorny merged commit 908fed9 into Bash-it:master Dec 27, 2020
@NoahGorny NoahGorny deleted the fix-1354 branch December 27, 2020 17:11
@@ -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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

removed '/tmp/alias_completion-<string>' printed on a new terminals
3 participants