Add useful test wrapper functions to FakeExec #14

danwinship · 2017-08-09T16:12:31Z

Migrated from kubernetes/kubernetes#46537...

The fake implementation of pkg/util/exec for test programs is kind of really annoying to use. This PR adds some wrapper functions to make it easier, AND to automatically check that the passed-in command was what you expected (which some tests were doing but most weren't).

So:

fcmd := fakeexec.FakeCmd{
        CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
                // iptables version check
                func() ([]byte, error) { return []byte("iptables v1.9.22"), nil },
                // iptables-restore version check
                func() ([]byte, error) { return []byte("iptables-restore v1.9.22"), nil },
                // Success.
                func() ([]byte, error) { return []byte{}, nil },
        },
}
fexec := fakeexec.FakeExec{
        CommandScript: []fakeexec.FakeCommandAction{
                func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
                func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
                func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
        },
}

becomes

fexec := fakeexec.NewFakeExec(t, nil)
fexec.AddCommand("iptables", "--version").
	SetCombinedOutput("iptables v1.9.22", nil)
fexec.AddCommand("iptables-restore", "--version").
	SetCombinedOutput("iptables-restore v1.9.22", nil)
fexec.AddCommand("iptables", "-w2", "-N", "FOOBAR", "-t", "nat").
	SetCombinedOutput("", nil)

There's some discussion on the original kubernetes bug (which I'm going to rebase on top of this to cover only the "porting pkg/util to the new interface" part). The June 27 comment about it not passing "make verify" no longer seems to be a problem with the migration of fakeexec into a separate subdirectory.

apelisse · 2017-08-09T17:43:57Z

exec/testing/fake_exec.go

+
+// SetCombinedOutput provides the result of calling CombinedOutput() on a FakeCmd.
+// output can be either a []byte or a string (which will be converted to a []byte).
+func (fcmd *FakeCmd) SetCombinedOutput(output interface{}, err error) {


If you absolutely want output to be []byte, why don't you take a []byte type instead? It's not difficult to convert string to []byte for the caller anyway.

Literally every existing caller in kubernetes wants to pass a string rather than a []byte, so if you don't want it to accept both then I'd change the API to just take a string instead, and say people can use the lower-level API if they need to use []byte

Yeah, I just don't really like the interface{}. string is fine.

apelisse · 2017-08-09T17:46:35Z

exec/testing/fake_exec.go

+}
+
+// AssertExpectedCommands ensures that all of the commands added to fake via AddCommand were actually executed
+func (fake *FakeExec) AssertExpectedCommands() {


This function should probably be with other FakeExec function rather than here (if we keep it, see below)

Also, I'm really not a big fan of embedding testing.T in that class in the first place, and all the variables are public anyway, so why not leave that code inside the test?

Convenience. That's all any of this is about; making FakeExec more complicated so that test programs can be more simple.

Yeah, then it should probably panic instead, and the name could be better

apelisse · 2017-08-09T17:47:15Z

exec/testing/fake_exec.go

+}
+
+// SetRunOutput provides the result of calling Run() on a FakeCmd.
+// stdout and stderr can be either []byte or string (which will be converted to []byte).


Same as below, pass a []byte if that's what you want

apelisse · 2017-08-09T17:47:28Z

exec/testing/fake_exec.go

+	return fcmd
+}
+
+func outputAsBytes(output interface{}) []byte {


See below, remove this

apelisse · 2017-08-09T17:58:05Z

exec/testing/fake_exec.go

+// t.Fatal.) cmdsInPath is a list of command names for which LookPath will return a match
+// ("/fake-bin/commandname"); if your code does not use LookPath you can pass nil. (If you
+// need more complex behavior, you can override LookPathFunc on the returned FakeExec.)
+func NewFakeExec(t *testing.T, cmdsInPath []string) *FakeExec {


If you need more complex behavior, you can override LookPathFunc on the returned FakeExec.

Why would I call this function in the first place if I want to change LookPathFunc, considering that's pretty much the only added value of this function?

I think my point is: the value of this function is to provide a default LookPathFunc. Can you define a sort of LookPathFunc function somewhere that one could use (decouple creation of FakeExec from LookPath selection):

func MakeFakeBinLookPathFunc(cmdsInPath []string) func(cmd string) (string, error) { return func(cmd string) (string, error) { for _, pathCmd := range cmdsInPath { if pathCmd == cmd { return "/fake-bin/" + cmd, nil } } return "", &osexec.Error{Name: cmd, Err: osexec.ErrNotFound} } } func MyTest(t *testing.T) { fexec := myFakeExec{LookPathFunc: MakeFakeBinLookPathFunc([]string{ ... })} ... }

Edit: I'm bad at naming things.

Why would I call this function in the first place if I want to change LookPathFunc, considering that's pretty much the only added value of this function?

I guess I like having New methods rather than manually constructing somebody-else's-struct-type by hand. I had added func NewFakeExec(t *testing.T) *FakeExec before adding the cmdsInPath arg.

But yes, I could do it the way you suggested instead.

apelisse · 2017-08-09T18:23:07Z

exec/testing/fake_exec.go

 }

 type FakeCommandAction func(cmd string, args ...string) exec.Cmd

 func (fake *FakeExec) Command(cmd string, args ...string) exec.Cmd {
 	if fake.CommandCalls > len(fake.CommandScript)-1 {
-		panic(fmt.Sprintf("ran out of Command() actions. Could not handle command [%d]: %s args: %v", fake.CommandCalls, cmd, args))
+		fake.fail("ran out of Command() actions at %s executing %v", getCaller(1), append([]string{cmd}, args...))


Can we accumulate the errors in a Errors list inside the FakeExec instead? So that we don't depend on testing.T and we don't have to panic, and the test can check Errors however it wants to?

+1 prefer leaving the "fail" and the error reporting to the client.

Well, but the Command still has to do something when the code being tested calls CombinedOutput()/Run() on it. So you could have it return an error to the caller, but then you're testing how your code responds when it gets an error that doesn't correspond to anything it could actually get in the real world.

If you hit this point (or any of the other panic/fail points), then there is a bug in your test case, not in the code that it's testing. So the only reasonable behavior is to abort the test case.

I don't disagree with that, but then it should just probably just panic

(If using testing.T is absolutely unacceptable, then we can go back to using panic... I just preferred the t.Fatalf solution because it doesn't spew a gigantic pile of stack traces at you like panic does.)

(fwiw, wrote that last comment before seeing your response)

apelisse · 2017-08-09T20:51:21Z

exec/testing/fake_exec.go

+
+// AddCommand adds a Cmd to be returned by a future fake.Command(...) call. Call
+// SetCombinedOutput or SetRunOutput on the result to specify its result.
+func (fake *FakeExec) AddCommand(cmd string, args ...string) *FakeCmd {


I like that this makes things easier, but I'd decouple the command creation a little bit.

Can we have:

// Hides the logic of appending commands func (fake *FakeExec) AddCommand(cmd *FakeCmd) { ... }

And something else to create the command

// Hides the pain of creating commands func NewMatchingArgsCommand(cmd string, args ...string) *FakeCmd { return func(cmd string, args ...string) exec.Cmd { // Some of the logic you have in your command } }

Then it means that:

I can add command easily, but they don't have to follow your logic

I can invent new types of commands

If you want to add a command in a different way, you can just continue to use the existing FakeExec API. There is currently only 1 test program in all of kubernetes that wants to do something other than what the API above supports. (pkg/volume/git_repo/git_repo_test.go)

Yeah, but I think this would make it much easier to compose and re-use the code

apelisse · 2017-08-09T21:31:48Z

Also, this is complicated enough that we may want to test it?

danwinship · 2017-08-10T16:22:54Z

Also, this is complicated enough that we may want to test it?

Yes, although ironically, the code using testing.T can't really be tested. Though the panic-ing versions of those codepaths could be.

apelisse · 2017-08-10T16:24:11Z

Yes, although ironically, the code using testing.T can't really be tested.

;-)

fejta-bot · 2018-01-02T20:06:37Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

thockin · 2018-01-02T22:34:46Z

fejta-bot is coming after PRs now!

fejta-bot · 2018-01-02T23:25:46Z

Resistance is futile. Your issues and PRs will be assimilated. Their historical and technological distinctiveness will be added to our org.

fejta-bot · 2018-02-07T23:39:11Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

thockin · 2018-02-20T05:13:15Z

@danwinship is this still alive?

fejta-bot · 2018-03-22T06:12:54Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Add useful test wrapper functions to FakeExec

39885d9

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2017

danwinship mentioned this pull request Aug 9, 2017

Make FakeExec easier / less-boilerplatey / better kubernetes/kubernetes#46537

Closed

mengqiy assigned apelisse, mengqiy and monopole Aug 9, 2017

apelisse reviewed Aug 9, 2017

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 2, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 7, 2018

k8s-ci-robot closed this Mar 22, 2018

danwinship mentioned this pull request Mar 14, 2023

clean up kube-proxy stale-conntrack-entry handling, revert broken code kubernetes/kubernetes#115299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add useful test wrapper functions to FakeExec #14

Add useful test wrapper functions to FakeExec #14

danwinship commented Aug 9, 2017

apelisse Aug 9, 2017

danwinship Aug 10, 2017

apelisse Aug 10, 2017

apelisse Aug 9, 2017

danwinship Aug 10, 2017

apelisse Aug 10, 2017

apelisse Aug 9, 2017

apelisse Aug 9, 2017

apelisse Aug 9, 2017

danwinship Aug 10, 2017

apelisse Aug 9, 2017

monopole Aug 9, 2017

danwinship Aug 10, 2017

apelisse Aug 10, 2017

danwinship Aug 10, 2017

danwinship Aug 10, 2017

apelisse Aug 9, 2017

monopole Aug 9, 2017

danwinship Aug 10, 2017

apelisse Aug 10, 2017

apelisse commented Aug 9, 2017

danwinship commented Aug 10, 2017

apelisse commented Aug 10, 2017

fejta-bot commented Jan 2, 2018

thockin commented Jan 2, 2018

fejta-bot commented Jan 2, 2018

fejta-bot commented Feb 7, 2018

thockin commented Feb 20, 2018

fejta-bot commented Mar 22, 2018

Add useful test wrapper functions to FakeExec #14

Add useful test wrapper functions to FakeExec #14

Conversation

danwinship commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Aug 9, 2017

danwinship commented Aug 10, 2017

apelisse commented Aug 10, 2017

fejta-bot commented Jan 2, 2018

thockin commented Jan 2, 2018

fejta-bot commented Jan 2, 2018

fejta-bot commented Feb 7, 2018

thockin commented Feb 20, 2018

fejta-bot commented Mar 22, 2018