[Bug] SetArgs doesn't reset flag value

[levisyin]

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
}

[marckhouzam]

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.

[levisyin]

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.

[marckhouzam]

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()

[levisyin]

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

[marckhouzam]

@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".

[levisyin]

@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)
}

---

1. As SliceValue type with calling Set will append to current value if this flag has changed.
2. For builtin implements of SliceValue, there is no way for us to get the separator of them, so I couldn't split DefValue string into []string for SliceValue.Replace([]string) error.
3. 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:

1. Reset value to default value
2. Reset changed mark to false

If this LGTY, I will draft a pr to implement this~

[levisyin]

@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😀

[marckhouzam]

@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:

1. Aren't the slices always separated by commas (,)? Or is there a way to choose a different separator?
2. 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 use Replace() with that value.

[levisyin]

@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