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

Make detection for test-binary more universal #2173

Merged
merged 2 commits into from
Dec 28, 2024

Conversation

thaJeztah
Copy link
Contributor

When running tests in verbose mode (or other options), tests involving Cobra may fail if the test does not explicitly set Command.args to an empty slice; in this case, Cobra defaults to using os.Args, which will contain arguments passed to the test (such as -v (verbose)).

Commits e576205 and 1ef0913 implemented a workaround for this when running (unit) tests for Cobra itself, but this check is specifig to Cobra (checking for cobra.test), and don't work on Windows (which will have a .exe extension),

This patch implements a more universal check, so that users of Cobra as a module also benefit from this workaround.

go1.21 and up provides a testing.Testing() utility (1); as the Cobra module still supports Go1.16 and up, an alternative implementation was added for older versions, based on golang.org/x/mod/lazyregexp 2.

@marckhouzam
Copy link
Collaborator

@thaJeztah Thanks for this. I had never dug into the history of this cobra.test check, and I'm glad you've explained it so well.

I'm trying to reproduce the issue by removing this cobra.test check but haven't been able to.
I added this test to cobra:

func Test1(t *testing.T) {
	root := &Command{Use: "root", Run: func(_ *Command, args []string) {}}

	if _, err := root.ExecuteC(); err != nil {
		t.Errorf("error: %v", err)
	}
}

Then removed the cobra.test check, and added a printout of the args that are read by Cobra. Then ran go test:

$ go test -v -run ^Test1$ github.com/spf13/cobra
=== RUN   Test1
args:  [-test.testlogfile=/var/folders/k0/lwnx9y_n7393cfx7mj7dct580000gq/T/go-build3346663322/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -test.run=^Test1$]
--- PASS: Test1 (0.00s)
PASS
ok  	github.com/spf13/cobra	(cached)

Notice that the test flags are processed by cobra, but the test still passes.
This led me to this piece of code in spf13/pflag, which explicitly ignores test.* flags.
https://github.com/spf13/pflag/blob/d5e0c0615acee7028e1e2740a11102313be88de1/flag.go#L1017

I wonder why things fail in your case?
Could you point me to a way to reproduce this issue so that I can see your PR at work?

@thaJeztah
Copy link
Contributor Author

Ah, yeah, I probably should've provided a good reproducer when I was working on this 😂 (It's been a while). Let me try come with one 👍

IIRC this happened either when the test was pre-compiled and run, but also in some cases when running tests from my IDE (I recall I ran into these when working on docker/cli, and re-generating the .golden files generated by gotest.tools.

@thaJeztah
Copy link
Contributor Author

I think I have an example (I saw I wrote one up in docker/cli#5224), and it was indeed when pre-compiling the tests;

Before this patch:

go test -c -o foo.test

./foo.test -test.run TestNoArgs
--- FAIL: TestNoArgs (0.00s)
    args_test.go:37: Unexpected output: Error: unknown command "TestNoArgs" for "c"
        Usage:
          c [flags]

        Flags:
          -h, --help   help for c

    args_test.go:40: Unexpected error: unknown command "TestNoArgs" for "c"
FAIL

After this patch:

go test -c -o foo.test

./foo.test -test.run TestNoArgs
PASS

I can update the commit message with that information 👍

When running tests in verbose mode (or other options), tests involving
Cobra may fail if the test does not explicitly set Command.args to an
empty slice; in this case, Cobra defaults to using `os.Args`, which
will contain arguments passed to the test (such as `-v` (verbose)).

Commits e576205 and 1ef0913
implemented a workaround for this when running (unit) tests for Cobra
itself, but this check is specifig to Cobra (checking for `cobra.test`),
and don't work on Windows (which will have a `.exe` extension),

This patch implements a more universal check, so that users of Cobra
as a module also benefit from this workaround.

go1.21 and up provides a `testing.Testing()` utility ([1]); as the Cobra
module still supports Go1.16 and up, an alternative implementation was
added for older versions, based on golang.org/x/mod/lazyregexp [2].

Before this patch:

Before this patch:

    go test -c -o foo.test

    ./foo.test -test.run TestNoArgs
    --- FAIL: TestNoArgs (0.00s)
        args_test.go:37: Unexpected output: Error: unknown command "TestNoArgs" for "c"
            Usage:
              c [flags]

            Flags:
              -h, --help   help for c

        args_test.go:40: Unexpected error: unknown command "TestNoArgs" for "c"
    FAIL

After this patch:

    go test -c -o foo.test

    ./foo.test -test.run TestNoArgs
    PASS

[1]: https://pkg.go.dev/testing#Testing
[2]: https://cs.opensource.google/go/x/mod/+/refs/tags/v0.19.0:internal/lazyregexp/lazyre.go;l=66-78

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Contributor Author

Updated the commit message for future reference 😄

@marckhouzam
Copy link
Collaborator

I think I have an example (I saw I wrote one up in docker/cli#5224), and it was indeed when pre-compiling the tests;

./foo.test -test.run TestNoArgs
--- FAIL: TestNoArgs (0.00s)
   args_test.go:37: Unexpected output: Error: unknown command "TestNoArgs" for "c"

[...]

Aha, I see. So the -test.* flags get ignored properly, but the flag values remain and become arguments to the cobra command.

I see that using the = form works, as I would expect: ./foo.test -test.run=TestNoArgs.
Let me review the proposed change and see.

command.go Outdated Show resolved Hide resolved
command_go120.go Outdated Show resolved Hide resolved
command_go121.go Outdated Show resolved Hide resolved
command_go120.go Show resolved Hide resolved
Signed-off-by: Marc Khouzam <[email protected]>
@marckhouzam
Copy link
Collaborator

I pushed a commit addressing the comments and adding a unit test.
Thanks @thaJeztah for this.

@marckhouzam marckhouzam added this to the 1.9.0 milestone Dec 28, 2024
Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah If you have comments on the latest commit I pushed, please let me know and we can fix it in a follow up.

@marckhouzam marckhouzam merged commit d1e9d85 into spf13:main Dec 28, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants