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
fix: (fish-completion) correct value enum help text #5111
Conversation
@@ -84,6 +84,18 @@ fn value_hint() { | |||
); | |||
} | |||
|
|||
#[test] | |||
fn value_help() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've recently added support for end-to-end testing of completions.
Instead of this test could you
- Move the
choice
arg toexamples/exhaustive.rs
under thequote
subcommand? - Can you add additional asserts to
fn complete
at the end of the file to do end-to-end verification of this behavior?
I would ask all of this be done in one commit and do whatever it takes to make it pass (even if it shows bad output) and then put the fix commit on top of it. That will help show to both you and me that the test is testing the correct situation and that the behavior is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in summary, we need a failing commit first and then a fix for it. Effectively, there would only be two commits. Please confirm my understanding @epage. I will make the suggested changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be explicit, it should be a failing commit in that it shows the via passing tests rather than tests failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides that, yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood @epage. Will raise a new PR and link here.
Opened #5114 |
Resolves #5101