[Bug] SetArgs doesn't reset flag value
levisyin opened this issue · 9 comments
See following test code. Call SetArgs
won't reset flag value, Execute()
also won't reset flag value too.
func TestSetArgs(t *testing.T) {
getCMD := func() *Command {
cmd := &Command{
Use: "testcmd",
RunE: func(cmd *Command, args []string) error {
a, _ := cmd.Flags().GetBool("a")
b, _ := cmd.Flags().GetBool("b")
c, _ := cmd.Flags().GetBool("c")
switch {
case a && b, a && c, b && c:
return fmt.Errorf("a,b,c only one could be true")
}
return nil
},
}
f := cmd.Flags()
f.BoolP("a", "a", false, "a,b,c only one could be true")
f.BoolP("b", "b", false, "a,b,c only one could be true")
f.Bool("c", false, "a,b,c only one could be true")
return cmd
}
cmd := getCMD()
// step 1
cmd.SetArgs([]string{
"--a=true",
})
assertNoErr(t, cmd.Execute()) // PASS
// step 2
cmd.SetArgs([]string{
"--b=true",
})
t.Log(cmd.Flags().Changed("a")) // true
assertNoErr(t, cmd.Execute()) // FAIL, output error msg is "a,b,c only one could be true"
// step 3
cmd.SetArgs([]string{
"--c=true",
})
assertNoErr(t, cmd.Execute()) // FAIL, output error msg is "a,b,c only one could be true"
// step 4
cmd.SetArgs([]string{
"--a=false",
"--b=false",
"--c=true",
})
assertNoErr(t, cmd.Execute()) // PASS
}
Thanks for the report @levisyin . Can you explain in what scenario this is a problem?
In a real program execution, cmd.Execute()
is only called once I believe, and therefore this problem will not happen.
On the next run of the program, the flags will all be reset.
Thanks for the report @levisyin . Can you explain in what scenario this is a problem? In a real program execution,
cmd.Execute()
is only called once I believe, and therefore this problem will not happen. On the next run of the program, the flags will all be reset.
Hi sir, we use this in test case. As SetArgs
comment described, SetArgs
is particularly useful when testing, so we can call SetArgs
to overrid arguments for the command, and call cmd.Execute() again in the same context.
Ok, I understand. This does seem limiting. I have to think about this a bit as there may be backwards-compatibility issues in changing the behaviour of SetArgs()
How about changing the func ParseFlags
as PR #2080 did, reset flags to default value before err := c.Flags().Parse(args)
, when flags were parsed before
@levisyin I actually hit this problem today when writing unit tests. What I did was reset the relevant flags myself in the tests.
I recommend you implement this solution in your own tests.
Changing things in Cobra could have unexpected impacts on existing users and that would be worse than the problem reported.
So, although I agree it can be annoying, since it can be fixed by the project using cobra, I will close this issue as "won't fix".
@marckhouzam Hi sir, I met another issue with this bug. It's too difficult to implement the solution in my own tests as following.
func SetArgs(cmd *cobra.Command, args []string) {
if cmd.Flags().Parsed() {
cmd.Flags().Visit(func(pf *pflag.Flag) {
if err := pf.Value.Set(pf.DefValue); err != nil {
panic(fmt.Errorf("reset argument[%s] value error %v", pf.Name, err))
}
})
}
cmd.SetArgs(args)
}
- As
SliceValue
type with callingSet
will append to current value if this flag has changed. - For builtin implements of
SliceValue
, there is no way for us to get the separator of them, so I couldn't splitDefValue string
into[]string
forSliceValue.Replace([]string) error
. - For custom implements for
SliceValue
, it's hard to get what separator they used.
So, can we add a new function Reset() error
to type Value interface
, like
type Value interface {
String() string
Set(string) error
Reset() error
Type() string
}
Every implementations implement Reset() error
should be:
- Reset value to default value
- Reset
changed
mark to false
If this LGTY, I will draft a pr to implement this~
@marckhouzam Hi sir, can we reopen this issue, I indeed couldn't find a better way for implementing the solution in my own project tests, could you please give me some suggestions😀
@levisyin type Value interface
is not part of Cobra. It is defined in a different project github.com/spf13/pflag
.
However, two thoughts on this:
- Aren't the slices always separated by commas (
,
)? Or is there a way to choose a different separator? - Since this is for your tests, you probably know the default value and not have to read it from
DefValue
. You therefore should be able to useReplace()
with that value.
@marckhouzam Hi sir, thanks for the reminder.
For thought 1, after I reviewed again, the builtin implements of SliceValue
are always separated by commas ,
, but for custom implements, we are not sure
For thought 2, since this is aimed to be a util function
for all tests, it shouldn't care about the test case itself IMO