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.

Sign up for GitHub

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

Jump to bottom

adds check to see if command name and os.Args[0] is the same before... #155

Closed
wants to merge 1 commit into from
Closed

adds check to see if command name and os.Args[0] is the same before... #155

wants to merge 1 commit into from

Conversation

apriendeau
Copy link
Contributor

applying os.Args as it was leading appending false arguments from the test suite. This was causing the test suite to fail. All tests should be passing now.

@apriendeau
adds check to see if command name and os.Args[0] is the same before a…
ff4a381 
…pplying os.Args as it was leading appending false arguments from the test suite
@apriendeau apriendeau mentioned this pull request Sep 14, 2015
Closed
@eparis
Copy link
Collaborator

eparis commented Sep 15, 2015

what test was failing?

@apriendeau
Copy link
Contributor Author

The RunRoot with no args and the hidden command tests because it was trying
to use the go test args. You should see it on master currently.
On Tue, Sep 15, 2015 at 11:35 AM Eric Paris notifications@github.com
wrote:

what test was failing?

—
Reply to this email directly or view it on GitHub
#155 (comment).

@apriendeau
Copy link
Contributor Author

@eparis sorry, I wasn't at my computer but those are the two.
--- FAIL: TestRunnableRootCommandNilInput (0.00s)
cobra_test.go:656: Root Function was not called

Usage:
hide [secret string to print] [flags]

Flags:
--flagOnCommandLine="": about my flag

Error: unknown shorthand flag: 't' in -test.v=true
--- FAIL: TestHiddenCommandExecutes (0.00s)
command_test.go:21: Hidden command failed to run!

@eparis
Copy link
Collaborator

eparis commented Sep 15, 2015

so you ran go test -v ./... and the tests failed. But go test ./... works fine. Which is why I'm not seeing a problem and master seems happy? You are saying this patch will let me use go test -v ./... ?

@apriendeau
Copy link
Contributor Author

Yes because -v is just the verbose output of what you are running. The fact
that the suite was passing baffles me.
On Tue, Sep 15, 2015 at 3:58 PM Eric Paris notifications@github.com wrote:

so you ran go test -v ./... and the tests failed. But go test ./... works
fine. Which is why I'm not seeing a problem and master seems happy? You are
saying this patch will let me use go test -v ./... ?

—
Reply to this email directly or view it on GitHub
#155 (comment).

@eparis
Copy link
Collaborator

eparis commented Sep 15, 2015

The suite passes because it is correct and passing. Adding -v is actually what broke it!

What's happening is that go test is (behinds the scene) importing "testing" into our code. "testing" will then create new (go flags) called "--test.*" in our program. In the "testing" package init() (or some similar thing) it will change the -v you added to be -test.v in the command line arguments to our program. "Testing" will then call flag.Parse() so the "testing" package will will have access to its results. Ok all great.

BUT cobra doesn't use the golang "flag" package, it uses "pflag" which does its own command line argument parsing. The pflag command line has no idea what -test.v is. In fact, since the go test command uses a single - instead of a -- pflag believes the flag name is just t with an argument of est.v.

Whew. Ok, maybe that makes sense why go test works and travis was correctly passing, but go test -v fails. Given all that, I'll try to understand how your patch solves the problem that go test -v does not work.

Thanks for sticking with me!

@apriendeau
Copy link
Contributor Author

Course. The patch just checks if the command is actually the cobra command
before applying the os.Args. So it's just another layer of validation.
That's all.
On Tue, Sep 15, 2015 at 4:34 PM Eric Paris notifications@github.com wrote:

The suite passes because it is correct and passing. Adding -v is actually
what broke it!

What's happening is that go test is (behinds the scene) importing
"testing" into our code. "testing" will then create new (go flags) called
"--test.*" in our program. In the "testing" package init() (or some
similar thing) it will change the -v you added to be -test.v in the
command line arguments to our program. "Testing" will then call
flag.Parse() so the "testing" package will will have access to its
results. Ok all great.

BUT cobra doesn't use the golang "flag" package, it uses "pflag" which
does its own command line argument parsing. The pflag command line has no
idea what -test.v is. In fact, since the go test command uses a single -
instead of a -- pflag believes the flag name is just t with an argument
of est.v.

Whew. Ok, maybe that makes sense why go test works and travis was
correctly passing, but go test -v fails. Given all that, I'll try to
understand how your patch solves the problem that go test -v does not
work.

Thanks for sticking with me!

—
Reply to this email directly or view it on GitHub
#155 (comment).

@apriendeau
Copy link
Contributor Author

@eparis is there anything I need to do with this PR?

@apriendeau apriendeau closed this Oct 26, 2015
@anthonyfok
Copy link
Collaborator

Hi @apriendeau, @spf13 and @eparis,

Now that go test -v ./... is used as per commit 5c40aa8 for #157, Travis CI test is failing, as expected.

Should we revisit this Pull Request #155? :-)

@apriendeau
Copy link
Contributor Author

Sure. I can put a PR up for it today again.

@anthonyfok
Copy link
Collaborator

Cool, thanks! :-)

@anthonyfok anthonyfok mentioned this pull request Nov 7, 2015
Closed
@apriendeau apriendeau mentioned this pull request Nov 7, 2015
Merged
anthonyfok added a commit that referenced this pull request Nov 8, 2015
@anthonyfok
Limit the verbose test workaround to "cobra.test" only
e576205 
so that full path to the executable or a renamed executable
parses command-line arguments correctly as before.

Special thanks to @apriendeau for discovering "go test -v" failing
and for providing the initial workaround, see #155 and subsequent
discussions.
@jonreyna jonreyna mentioned this pull request Jan 14, 2016
Closed
@mosheco mosheco mentioned this pull request Mar 16, 2016
Closed
@urbainleverrier urbainleverrier mentioned this pull request Jan 27, 2020
Closed
lkingland added a commit to lkingland/func that referenced this pull request Mar 28, 2022
@lkingland
explicitly empty test command args
88c8f63 
See spf13/cobra#155

Errors can still be encountered when, for example, using precomiled
tests.  Explicitly setting consttucted command args to the empty slice
ensures hidding any futher edge cases.
lkingland added a commit to lkingland/func that referenced this pull request Mar 28, 2022
@lkingland
explicitly empty test command args
6ca171f 
See spf13/cobra#155

Errors can still be encountered when, for example, using precomiled
tests.  Explicitly setting constructed command args to the empty slice
ensures we avoid hitting any futher edge cases.
knative-prow bot pushed a commit to knative/func that referenced this pull request Mar 30, 2022
@lkingland
src: exported client constructor with options (#883)
9a0335d 
* update root and version structure and help text

* fix: limit openshift int test with tag

* refactor: commands to use simplifed, unified constructor

* fix ineffectual assignment lint error

* cleanup

* add repository to run command

* callout for forthcoming s2i builder impl

* lint errors

* re-add the deferred client factory

* remove setNamespaceFlag now that it is persistent

* avoid side-effect of global-mutating deploy tests

* reduce line-by-line difference for PR ease

* simplificaiton of tests and comment lines for further PR ease purposes

* reduce inconsequential differences for ease of PR

* tests to RootCommandConfig

* review comment updates

* fix lint errors

* replace stdlib Setenv in tests

Using t.Setenv will require an update to go1.17, which is out of scope
for this PR.

* pass ClientFactory throughout

* explicitly empty test command args

See spf13/cobra#155

Errors can still be encountered when, for example, using precomiled
tests.  Explicitly setting constructed command args to the empty slice
ensures we avoid hitting any futher edge cases.
salaboy pushed a commit to salaboy/kn-plugin-func that referenced this pull request Apr 14, 2022
@lkingland @salaboy
src: exported client constructor with options (knative#883)
ae215d0 
* update root and version structure and help text

* fix: limit openshift int test with tag

* refactor: commands to use simplifed, unified constructor

* fix ineffectual assignment lint error

* cleanup

* add repository to run command

* callout for forthcoming s2i builder impl

* lint errors

* re-add the deferred client factory

* remove setNamespaceFlag now that it is persistent

* avoid side-effect of global-mutating deploy tests

* reduce line-by-line difference for PR ease

* simplificaiton of tests and comment lines for further PR ease purposes

* reduce inconsequential differences for ease of PR

* tests to RootCommandConfig

* review comment updates

* fix lint errors

* replace stdlib Setenv in tests

Using t.Setenv will require an update to go1.17, which is out of scope
for this PR.

* pass ClientFactory throughout

* explicitly empty test command args

See spf13/cobra#155

Errors can still be encountered when, for example, using precomiled
tests.  Explicitly setting constructed command args to the empty slice
ensures we avoid hitting any futher edge cases.
@thaJeztah thaJeztah mentioned this pull request Jul 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apriendeau @eparis @anthonyfok