proposal: testing: add Setargs to set os.Args

[mitar]

os.Args is similar to os.Getenv that it is a global state of the process. testing package provides a function Setenv on T which tries to help tests which use environment variables:

• It makes sure that such tests are not run in parallel.
• It makes sure to restore env after the test.

Currently, nothing similar exist for os.Args. But sometimes it is necessary to do so to be able to test CLI parsing (especially in tools which expect to read from os.Args. Current option to do the change to os.Args yourself and then restore it has a downside that it does not check if t.Parallel has been called.

I understand that global state is tricky, but making a function which checks for parallel execution could help here.

[rittneje]

Normally we just make a helper function and then test that, like so:

func ParseArgs() {
    parseArgs(os.Args)
}

func parseArgs(args []string) {
    ...
}

---

func TestParseArgs(t *testing.T) {
    parseArgs([]string{ ... })
}

[mitar]

That works when the function uses os.Args at a top-level or near it at least. But if it is used somewhere deep inside, it becomes tricky to pass it all the way there, especially if that is needed just for testing.

[mvdan]

I see https://pkg.go.dev/testing#T.Setenv and #62516 at a different level as this proposal, because rewriting one's code to avoid relying on the global state is non-trivial.

For example, there's no easy type to replace environment variables with; there's func(string) string mimicking GetEnv, but it doesn't support listing/iterating like Environ, nor lookup with a boolean like LookupEnv, and []string makes lookups more involved. And with the current directory, one might have to thread it through a number of dependency func calls, which can become a tricky task.

os.Args seems significantly simpler. Only the main package should use it, so dependencies are less likely to be a problem. And it's trivial to add an args []string parameter to replace it with.

[mitar]

Only the main package should use it

Also logging packages can use, to extract for example command line used.

[bcmills]

sometimes it is necessary to do so to be able to test CLI parsing

In general you can already test CLI parsing in a couple of ways:

1. Execute the program as a subprocess the same way a user would, and check its output for the expected behavior. One simple way to do that is to have the test's TestMain invoke the main function depending on whether an environment variable is set, as is currently done in the tests for cmd/go, cmd/covdata, and cmd/cover: https://cs.opensource.google/go/go/+/master:src/cmd/covdata/tool_test.go;l=49-52;drc=8ffc931eae8bb7a4654695be39d95b62d369ee5c

2. Factor out your argument parsing into a function that accepts an explicit []string argument (instead of implicitly using the global os.Args) and perhaps either accepts or returns an explicit *flag.FlagSet (instead of implicitly using the global flag.CommandLine). Then you can call that function in an ordinary, parallelizable Go test without mutating shared state.

[rsc]

The main reason to set an environment variable in a test is that either (1) some code checks the environment variable directly, or (2) some code runs a subprocess that inherits the environment (rather than creating its own custom one for the exec) and that subprocess needs the variable.

Neither of these applies to os.Args. Normal functions don't read from os.Args, and os.Args is not inherited by exec.

This seems like significant complexity and an implicit suggestion that code should be doing things that it really should not be doing.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[mitar]

I see many arguments against it. I will then retract this proposal.

In my own project I ended up simply having a bash script provide sets of arguments and checking exit codes each time. While using Coverage profiling support for integration tests to obtain coverage.

[rsc]

This proposal has been declined as retracted.
— rsc for the proposal review group