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

proposal: testing: add Setargs to set os.Args #63607

Closed
mitar opened this issue Oct 18, 2023 · 9 comments
Closed

proposal: testing: add Setargs to set os.Args #63607

mitar opened this issue Oct 18, 2023 · 9 comments
Labels
Milestone

Comments

@mitar
Copy link
Contributor

mitar commented Oct 18, 2023

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.

@mitar mitar added the Proposal label Oct 18, 2023
@gopherbot gopherbot added this to the Proposal milestone Oct 18, 2023
@rittneje
Copy link

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
Copy link
Contributor Author

mitar commented Oct 18, 2023

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
Copy link
Member

mvdan commented Oct 18, 2023

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
Copy link
Contributor Author

mitar commented Oct 18, 2023

Only the main package should use it

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

@bcmills
Copy link
Contributor

bcmills commented Oct 23, 2023

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
Copy link
Contributor

rsc commented Oct 27, 2023

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
Copy link
Contributor

rsc commented Oct 27, 2023

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
Copy link
Contributor Author

mitar commented Oct 27, 2023

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.

@mitar mitar closed this as completed Oct 27, 2023
@rsc
Copy link
Contributor

rsc commented Nov 2, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

6 participants