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

test(bash): Add a bunch of tests for bash special characters. #25

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

JeffFaer
Copy link
Contributor

Right now, these tests fail. They will pass after spf13/cobra#2126 has been merged.

This PR is based on top of #24

@marckhouzam
Copy link
Owner

I’m trying to look into this one, but I’m having docker issues. Some update site is down right now. I’ll try again later on.

Copy link
Owner

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

I've merged #24, so you can rebase this on the new main

@JeffFaer
Copy link
Contributor Author

JeffFaer commented Nov 8, 2024

I've merged #24, so you can rebase this on the new main

Done!

@marckhouzam
Copy link
Owner

@JeffFaer Could you sign-ff the two last commits, or else the DCO check fails.

testprog/testprog.go Outdated Show resolved Hide resolved
tests/bash/comp-tests.bash Outdated Show resolved Hide resolved
tests/bash/comp-test-lib.bash Show resolved Hide resolved
tests/bash/comp-tests.bash Outdated Show resolved Hide resolved
@JeffFaer JeffFaer force-pushed the special_characters branch 2 times, most recently from a6e57de to 2810560 Compare December 5, 2024 04:38
@JeffFaer JeffFaer requested a review from marckhouzam December 5, 2024 04:38
@marckhouzam
Copy link
Owner

marckhouzam commented Dec 27, 2024

I've pushed a new commit to this PR to help in testing: 14557ce

@marckhouzam
Copy link
Owner

@JeffFaer I'm almost ready to merge this but one of your commit does not have the "Signed-off-by" and fails the DCO check. Could you fix that and push it back please?

@marckhouzam
Copy link
Owner

@JeffFaer I have pushed a commit on top of yours which adds some tests for special-characters when using completion descriptions.

I'm happy with this PR and will merge as soon as you can repush it to properly add the signed-off to your commit.
Actually, if you can squash all your commits into one, that would be even better.

@JeffFaer
Copy link
Contributor Author

Done!

Also, avoid changing COMP_TYPE.

Signed-off-by: Marc Khouzam <[email protected]>
_completionTests_verifyCompletion "testprog prefix nospace b" "bear bearpaw" nospace
_completionTests_verifyCompletion "testprog prefix nofile b" "bear bearpaw" nofile
# COMP_TYPE does not get set by bash 3
if [ $BASH_VERSINFO != 3 ]; then
Copy link
Owner

Choose a reason for hiding this comment

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

It turns out that there is no point in running the tests that set COMP_TYPE for bash 3 because COMP_TYPE only gets set by bash 4. So the tests were passing because we were setting COMP_TYPE ourselves, but it is not representative of what bash 3 does. So I have disable then for bash 3.

Copy link
Owner

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Well this was quite the learning experience.
Thanks @JeffFaer for your work.
I have added a commit on top of your to tweak a few things, and I feel we are in a good state with this set of new tests.

@marckhouzam marckhouzam merged commit 4376445 into marckhouzam:main Jan 21, 2025
1 check passed
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.

2 participants