-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Add a util/exec interface for testing execs. #1592
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/* | ||
Copyright 2014 Google Inc. All rights reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
// Package exec provides an injectable interface and implementations for running commands. | ||
package exec |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/* | ||
Copyright 2014 Google Inc. All rights reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package exec | ||
|
||
import ( | ||
osexec "os/exec" | ||
"syscall" | ||
) | ||
|
||
// Interface is an interface that presents a subset of the os/exec API. Use this | ||
// when you want to inject fakeable/mockable exec behavior. | ||
type Interface interface { | ||
// Command returns a Cmd instance which can be used to run a single command. | ||
// This follows the pattern of package os/exec. | ||
Command(cmd string, args ...string) Cmd | ||
} | ||
|
||
// Cmd is an interface that presents an API that is very similar to Cmd from os/exec. | ||
// As more functionality is needed, this can grow. Since Cmd is a struct, we will have | ||
// to replace fields with get/set method pairs. | ||
type Cmd interface { | ||
// CombinedOutput runs the command and returns its combined standard output | ||
// and standard error. This follows the pattern of package os/exec. | ||
CombinedOutput() ([]byte, error) | ||
} | ||
|
||
// ExitError is an interface that presents an API similar to os.ProcessState, which is | ||
// what ExitError from os/exec is. This is designed to make testing a bit easier and | ||
// probably loses some of the cross-platform properties of the underlying library. | ||
type ExitError interface { | ||
String() string | ||
Error() string | ||
Exited() bool | ||
ExitStatus() int | ||
} | ||
|
||
// Implements Interface in terms of really exec()ing. | ||
type executor struct{} | ||
|
||
// New returns a new Interface which will os/exec to run commands. | ||
func New() Interface { | ||
return &executor{} | ||
} | ||
|
||
// Command is part of the Interface interface. | ||
func (executor *executor) Command(cmd string, args ...string) Cmd { | ||
return (*cmdWrapper)(osexec.Command(cmd, args...)) | ||
} | ||
|
||
// Wraps exec.Cmd so we can capture errors. | ||
type cmdWrapper osexec.Cmd | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider changing this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it will help one conversion, anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It fixed on site, trading one unreadable aspect for another, but sure. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I take it back. Go still confuses me here.
I guess I can't have pointers to pointer types? Why the hell not? This is staying as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, you would change all the receiver types to not be pointers, it's already a pointer, no need to be redundant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no can do. I run afoul of one of the rules in go: Can't declare methods on pointer values Can't have a value-receiver method for a pointer value Cmd is returned as a pointer, so unless I force users to deref it, which is different than the standard lib, it's stuck. This is new territory in Go's spec, so maybe I am just mis-comprehending it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sigh Giving up on this, I'm sure there's some simple reason why go is so picky about what you can define receivers on but it is sure annoying. |
||
|
||
// CombinedOutput is part of the Cmd interface. | ||
func (cmd *cmdWrapper) CombinedOutput() ([]byte, error) { | ||
out, err := (*osexec.Cmd)(cmd).CombinedOutput() | ||
if err != nil { | ||
ee, ok := err.(*osexec.ExitError) | ||
if !ok { | ||
return out, err | ||
} | ||
// Force a compile fail if exitErrorWrapper can't convert to ExitError. | ||
var x ExitError = &exitErrorWrapper{ee} | ||
return out, x | ||
} | ||
return out, nil | ||
} | ||
|
||
// exitErrorWrapper is an implementation of ExitError in terms of os/exec ExitError. | ||
// Note: standard exec.ExitError is type *os.ProcessState, which already implements Exited(). | ||
type exitErrorWrapper struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Effectively you've got pointer-to-pointer-to thing here, just as a nit I wonder if the below isn't a bit simpler and less indirect.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then I don't inherit the Error() method, resulting net more code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can make this a value receiver, if it makes you happier. Done |
||
*osexec.ExitError | ||
} | ||
|
||
// ExitStatus is part of the ExitError interface. | ||
func (eew exitErrorWrapper) ExitStatus() int { | ||
ws, ok := eew.Sys().(syscall.WaitStatus) | ||
if !ok { | ||
panic("can't call ExitStatus() on a non-WaitStatus exitErrorWrapper") | ||
} | ||
return ws.ExitStatus() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
/* | ||
Copyright 2014 Google Inc. All rights reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package exec | ||
|
||
import ( | ||
"testing" | ||
) | ||
|
||
func TestExecutorNoArgs(t *testing.T) { | ||
ex := New() | ||
|
||
cmd := ex.Command("/bin/true") | ||
out, err := cmd.CombinedOutput() | ||
if err != nil { | ||
t.Errorf("expected success, got %+v", err) | ||
} | ||
if len(out) != 0 { | ||
t.Errorf("expected no output, got %q", string(out)) | ||
} | ||
|
||
cmd = ex.Command("/bin/false") | ||
out, err = cmd.CombinedOutput() | ||
if err == nil { | ||
t.Errorf("expected failure, got nil error") | ||
} | ||
if len(out) != 0 { | ||
t.Errorf("expected no output, got %q", string(out)) | ||
} | ||
ee, ok := err.(ExitError) | ||
if !ok { | ||
t.Errorf("expected an ExitError, got %+v", err) | ||
} | ||
if ee.Exited() { | ||
if code := ee.ExitStatus(); code != 1 { | ||
t.Errorf("expected exit status 1, got %d", code) | ||
} | ||
} | ||
|
||
cmd = ex.Command("/does/not/exist") | ||
out, err = cmd.CombinedOutput() | ||
if err == nil { | ||
t.Errorf("expected failure, got nil error") | ||
} | ||
if ee, ok := err.(ExitError); ok { | ||
t.Errorf("expected non-ExitError, got %+v", ee) | ||
} | ||
} | ||
|
||
func TestExecutorWithArgs(t *testing.T) { | ||
ex := New() | ||
|
||
cmd := ex.Command("/bin/echo", "stdout") | ||
out, err := cmd.CombinedOutput() | ||
if err != nil { | ||
t.Errorf("expected success, got %+v", err) | ||
} | ||
if string(out) != "stdout\n" { | ||
t.Errorf("unexpected output: %q", string(out)) | ||
} | ||
|
||
cmd = ex.Command("/bin/sh", "-c", "echo stderr > /dev/stderr") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. General comment on this test file: this definitely breaks platform independence. Did our tests run and pass on windows boxes before now? If so, we should probably talk about whether that's a desirable property before we remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is important, obviously. If it is, let someone show me why :) |
||
out, err = cmd.CombinedOutput() | ||
if err != nil { | ||
t.Errorf("expected success, got %+v", err) | ||
} | ||
if string(out) != "stderr\n" { | ||
t.Errorf("unexpected output: %q", string(out)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to wrap ExitError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExitError is an alias of *os.ProcessState.
os.ProcessState has no public fields.
Thus, I can not mock exec errors. Sigh. Go's library is very nice in some regards and totally untestable in others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, os/exec is nice to use but a testability disaster :(
I think previously we've been testing things at a higher level (like the "DockerPuller" thing) instead of writing this generic exec testability wrapper. After seeing such a thing written out, I'm not convinced that's the wrong approach. But I'm not sure what you're going to do with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets wrapped into the iptables runner, which then can be faked to make testing services possible.
But somewhere at the bottom of the stack, someone needs to exec(). I was surprised I did not find a pre-made lib for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another approach would be to write an interface for iptables instead, with a real and fake implementation. For testing the real implementation, it'd be nice to actually run the commands (in a chroot/container or something?) and perform assertions on the iptables config file (that is what you're changing, right?). Otherwise, given this os/exec wrapping layer, I suspect you'll end up testing that some specific set of commands are called, which is better than nothing but is more of a change-detector than a correctness-detector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iptables makes a bunch of syscalls. I literally need to make sure that some specific set of commands was run. Deeper correctness testing will have to come from an e2e test, no way around it.