Fix conflict with standard flag package on testing

[xthezealot]

This fixes #167 and #187.

How it works

If it's a test (binary name ends with .test), keep flags that doesn't start with -test. and remove them from os.Args (as we have no control over flag.Parse).

Like that, for a command like go test -timeout 5s -args --flagforkingpin, we keep -test.timeout=5s for flag.Parse and --flagforkingpin for kingpin.Parse.

---

I also tried defusing flag.Parse by replacing flag.CommandLine with a new flag.FlagSet using PanicOnError instead of ExitOnError and recover if panic error is flag provided but not defined.
But we loose -test. flags.

[alecthomas]

Is this something that needs solving? Isn't it just easier to use the bulitin flags package with tests?

[xthezealot]

"Use the bulit-in flag package with tests" means something like: "check that it's a test and do not call kingpin.Parse"...

Not only that's repetitive (must be done for each project having tests), but also stops us from using our app args/flags during tests: if we call go test -args <args for app>, the flag package complains that our args are unknown… and test fails.

Killing a feature of the go test tool should not be acceptable.

Another option is to duplicate our Kingpin flags into the flag package.
Or rather the opposite: setting a hidden -t flag for Kingpin.
But this is unbelievably ugly.

All in all, a lot of us have tests and use tools that require, for example, the -timeout flag (Travis CI, Atom and VS Code plugins, ...) so having a conflict between Kingpin and the standard library in a common workflow should definitely not be acceptable.

Personally, I'm even surprised that it has not been solved yet! 😛

[alecthomas]

"Use the bulit-in flag package with tests" means something like: "check that it's a test and do not call kingpin.Parse"...

Not exactly. It means if you have flags for modifying test behaviour, use the builtin flags package. If you're testing your application, use kingpin.CommandLine.Parse(myArgs) directly.

Can you give an example of when this would not work?

Killing a feature of the go test tool should not be acceptable.

It is not, but I'm not convinced this is the right solution.

[xthezealot]

Can you give an example of when this would not work?

Take this main.go file:

package main

import "github.com/alecthomas/kingpin"

func main() {
	kingpin.Parse()
}

Take this main_test.go file:

package main

import "testing"

func Test(t *testing.T) {
	main()
}

Run a test with any valid flag of the go test tool:

$ go test -timeout 30s

This one is used by every continuous integration service (like Travis CI) or editor plugin.

Test fails:

test.test: error: unknown short flag '-t', try --help
exit status 1
FAIL test 0.001s

Very simple use case, right? That's why I'm surprised.

If you have flags for modifying test behaviour, use the built-in flag package.

Originally, like in this example, I don't even want to mess with the -test.* flags passed by the testing library or change the behavior of my code during tests…
I just want my tests to work. :)

[alecthomas]

I just want my tests to work. :)

I agree, but this is not the best way to achieve that. A more resilient way is to not directly (or indirectly in your example) call kingpin.Parse(). If you want to test your application, refactor it so that you can call kingpin.Application.Parse(args) with whatever arguments you prefer during testing.

Adapting your example, this would be something like this:

package main

import (
  "os"

  "github.com/alecthomas/kingpin"
)

func run(args []string) {
  kingpin.CommandLine.Parse(args)
}

func main() {
  run(os.Args[1:])
}

Test:

package main

import "testing"

func Test(t *testing.T) {
	run([]string{"--some-flag"})
}

This is both safer and more flexible, as you can test your application with any variation of command line arguments you like.

I'm not trying to be argumentative for the sake of being difficult. My objection is that this approach is fragile and inflexible, and basically a hack that will likely break under some conditions.

[xthezealot]

OK, calling kingpin.CommandLine.Parse avoids kingpin.MustParse which, among others, complains about unknown flags.

But like that, it's a feature of Kingpin that you kill.
Moreover, this bypass is even more fragile as you don't check for errors.

So (I know, I will be painful till the end), what if I still need to print usage and exit when a user makes a mistake and, at the same time, be able to just run my tests during dev?
I mean, this is the most normal behavior we can imagine.

This approach is basically a hack.

I won't say the contrary.
That said, when you have an invasive package like flag, the workaround could not be clean.

[alecthomas]

But like that, it's a feature of Kingpin that you kill.

kingpin.Parse() is a convenience for the common case of using kingpin.Application, in the same way as flag.Parse() is a convenience around flag.FlagSet.

Moreover, this bypass is even more fragile as you don't check for errors.

That is just an omission in the example. In reality you would check the error return value from kingpin.CommandLine.Parse().

So (I know, I will be painful till the end), what if I still need to print usage and exit when a user makes a mistake and, at the same time, be able to just run my tests during dev?

Then you'll need to check the error return value and call kingpin.Usage():

func run(args []string) error {
  _, err := kingpin.CommandLine.Parse(args)
  if err != nil {
    return err
  }
  // more code...
  return nil
}

func main() {
  err := run(os.Args[1:])
  if err != nil {
    kingpin.Usage()
    os.Exit(1)
  }
}

Thanks for the PR. I think it might be good for me to update the docs to clarify "best practice" for this approach.

[xthezealot]

You would check the error return value from kingpin.CommandLine.Parse.

OK, and how do you differentiate an "unknown flag" error from the others?
Using a regular expression (^unknown [a-zA-Z]+ flag .+$)?
Please.

If you take all parsing errors with the same severity, you're just rewriting the kingpin.Parse function.
This is exactly what you just have done in the last example.

Worst, a part of the problem is not even solved because using the -args flag of go test (a significant field of the Go toolchain) will still lead to a relentless failure.
Your workaround is also an inflexible hack.

---

So, to sum up, I understand that your package conflicts with the customs and habits of the language we use… And you just want to delegate the tricky and repetitive code to the users of your package for a normal usage. This is kind of a philosophy.

[alecthomas]

OK, and how do you differentiate an "unknown flag" error from the others?
Using a regular expression (^unknown [a-zA-Z]+ flag .+$)?
Please.
Worst, a part of the problem is not even solved because using the -args flag of go test (a significant field of the Go toolchain) will still lead to a relentless failure.

You should not be parsing test arguments with kingpin. It's that simple.

There are two scenarios:

1. You are testing your application's argument parsing. In this case, use the solution I have provided.
2. You want to alter the behaviour of your tests via flags. In this case, use the standard flags package.

So, to sum up, I understand that your package conflicts with the customs and habits of the language we use…

No, it does not conflict with the language or habits of Go. The approach outlined above is the way you should be doing this with Go. Feel free to post your approach to golang-nuts and see what the response is.

And you just want to delegate the tricky and repetitive code to the users of your package for a normal usage. This is kind of a philosophy.

It's 9 extra lines of code. If your argument is that perhaps this code should be included in Kingpin as a ParseForTesting(args) function, then I might agree with you.