From bd87c76ab880ace66c0e81da21818323db00161f Mon Sep 17 00:00:00 2001 From: Ben Sandler Date: Tue, 28 Jul 2015 20:55:36 +0000 Subject: [PATCH 1/5] Added osWrapper to visualization.go to enable testing of runPerlScript --- visualization/mocks.go | 31 ++++++++++++++++++++++++- visualization/visualization.go | 31 ++++++++++++++++++++++--- visualization/visualization_test.go | 36 +++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 4 deletions(-) diff --git a/visualization/mocks.go b/visualization/mocks.go index f5ee020..4c5ee03 100644 --- a/visualization/mocks.go +++ b/visualization/mocks.go @@ -20,7 +20,36 @@ package visualization -import "github.com/stretchr/testify/mock" +import ( + "os/exec" + + "github.com/stretchr/testify/mock" +) + +type mockOSWrapper struct { + mock.Mock +} + +func (m *mockOSWrapper) execLookPath(_a0 string) (string, error) { + println(_a0) + ret := m.Called(_a0) + + r0 := ret.Get(0).(string) + r1 := ret.Error(1) + + return r0, r1 +} +func (m *mockOSWrapper) cmdOutput(_a0 *exec.Cmd) ([]byte, error) { + ret := m.Called(_a0) + + var r0 []byte + if ret.Get(0) != nil { + r0 = ret.Get(0).([]byte) + } + r1 := ret.Error(1) + + return r0, r1 +} type mockExecutor struct { mock.Mock diff --git a/visualization/visualization.go b/visualization/visualization.go index a543ded..fc8114e 100644 --- a/visualization/visualization.go +++ b/visualization/visualization.go @@ -47,17 +47,32 @@ type defaultVisualizer struct { executor executor } +type osWrapper interface { + execLookPath(string) (string, error) + cmdOutput(*exec.Cmd) ([]byte, error) +} + +type defaultOSWrapper struct{} + type executor interface { createFile(string, []byte) error runPerlScript(string) ([]byte, error) } -type defaultExecutor struct{} +type defaultExecutor struct { + osWrapper osWrapper +} + +func newExecutor() executor { + return &defaultExecutor{ + osWrapper: new(defaultOSWrapper), + } +} // NewVisualizer returns a visualizer struct with default fileCreator func NewVisualizer() Visualizer { return &defaultVisualizer{ - executor: new(defaultExecutor), + executor: newExecutor(), } } @@ -90,7 +105,7 @@ func (e *defaultExecutor) runPerlScript(graphInput string) ([]byte, error) { possibilities := []string{"flamegraph.pl", cwd + "/flamegraph.pl", "flame-graph-gen"} perlScript := "" for _, path := range possibilities { - perlScript, err = exec.LookPath(path) + perlScript, err = e.osWrapper.execLookPath(path) // found a valid script if err == nil { break @@ -101,7 +116,17 @@ func (e *defaultExecutor) runPerlScript(graphInput string) ([]byte, error) { } cmd := exec.Command(perlScript, os.Stdin.Name()) cmd.Stdin = strings.NewReader(graphInput) + out, err := e.osWrapper.cmdOutput(cmd) + return out, err +} + +// execLookPath is a tiny wrapper around exec.LookPath to enable test mocking +func (w *defaultOSWrapper) execLookPath(path string) (fullPath string, err error) { + return exec.LookPath(path) +} +// cmdOutput is a tiny wrapper around cmd.Output to enable test mocking +func (w *defaultOSWrapper) cmdOutput(cmd *exec.Cmd) ([]byte, error) { return cmd.Output() } diff --git a/visualization/visualization_test.go b/visualization/visualization_test.go index b3ec64e..64f82ad 100644 --- a/visualization/visualization_test.go +++ b/visualization/visualization_test.go @@ -97,6 +97,42 @@ func TestGenerateFlameGraphExecError(t *testing.T) { mockExecutor.AssertExpectations(t) } +func TestRunPerlScriptDoesExist(t *testing.T) { + mockOSWrapper := new(mockOSWrapper) + executor := defaultExecutor{ + osWrapper: mockOSWrapper, + } + cwd, _ := os.Getwd() + mockOSWrapper.On("execLookPath", "flamegraph.pl").Return("", errors.New("DNE")).Once() + mockOSWrapper.On("execLookPath", cwd+"/flamegraph.pl").Return("", errors.New("DNE")).Once() + mockOSWrapper.On("execLookPath", "flame-graph-gen").Return("/somepath/flame-graph-gen", nil).Once() + + mockOSWrapper.On("cmdOutput", mock.AnythingOfType("*exec.Cmd")).Return([]byte("output"), nil).Once() + + out, err := executor.runPerlScript("some graph input") + + assert.Equal(t, []byte("output"), out) + assert.NoError(t, err) + mockOSWrapper.AssertExpectations(t) +} + +func TestRunPerlScriptDoesNotExist(t *testing.T) { + mockOSWrapper := new(mockOSWrapper) + executor := defaultExecutor{ + osWrapper: mockOSWrapper, + } + cwd, _ := os.Getwd() + mockOSWrapper.On("execLookPath", "flamegraph.pl").Return("", errors.New("DNE")).Once() + mockOSWrapper.On("execLookPath", cwd+"/flamegraph.pl").Return("", errors.New("DNE")).Once() + mockOSWrapper.On("execLookPath", "flame-graph-gen").Return("", errors.New("DNE")).Once() + + out, err := executor.runPerlScript("some graph input") + + assert.Equal(t, 0, len(out)) + assert.Error(t, err) + mockOSWrapper.AssertExpectations(t) +} + // Smoke test the NewVisualizer method func TestNewVisualizer(t *testing.T) { assert.NotNil(t, NewVisualizer()) From 0f07335f541db6eb08c4b1b12d954634bd0fe3f7 Mon Sep 17 00:00:00 2001 From: Ben Sandler Date: Tue, 28 Jul 2015 22:01:37 +0000 Subject: [PATCH 2/5] Added osWrapper to main.go to enable testing of runPprofCommand --- main.go | 25 ++++++++++++++++++++++--- main_test.go | 33 +++++++++++++++++++++++++++++++++ mocks.go | 18 ++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/main.go b/main.go index 1325ebc..d10e2f2 100644 --- a/main.go +++ b/main.go @@ -59,11 +59,19 @@ type validator interface { type defaultValidator struct{} +type osWrapper interface { + cmdOutput(*exec.Cmd) ([]byte, error) +} + +type defaultOSWrapper struct{} + type pprofer interface { runPprofCommand(args ...string) ([]byte, error) } -type defaultPprofer struct{} +type defaultPprofer struct { + osWrapper osWrapper +} // newTorcher returns a torcher struct with a default commander func newTorcher() *torcher { @@ -76,12 +84,18 @@ func newTorcher() *torcher { func newCommander() commander { return &defaultCommander{ validator: new(defaultValidator), - pprofer: new(defaultPprofer), + pprofer: newPprofer(), grapher: graph.NewGrapher(), visualizer: visualization.NewVisualizer(), } } +func newPprofer() pprofer { + return &defaultPprofer{ + osWrapper: new(defaultOSWrapper), + } +} + // main is the entry point of the application func main() { t := newTorcher() @@ -196,7 +210,7 @@ func (p *defaultPprofer) runPprofCommand(args ...string) ([]byte, error) { var buf bytes.Buffer cmd := exec.Command("go", allArgs...) cmd.Stderr = &buf - out, err := cmd.Output() + out, err := p.osWrapper.cmdOutput(cmd) if err != nil { return nil, err } @@ -212,6 +226,11 @@ func (p *defaultPprofer) runPprofCommand(args ...string) ([]byte, error) { return out, nil } +// cmdOutput is a tiny wrapper around cmd.Output to enable test mocking +func (w *defaultOSWrapper) cmdOutput(cmd *exec.Cmd) ([]byte, error) { + return cmd.Output() +} + // validateArgument validates a given command line argument with regex. If the // argument does not match the expected format, this function returns an error. func (v *defaultValidator) validateArgument(argument, regex, errorMessage string) error { diff --git a/main_test.go b/main_test.go index be77b87..63585c8 100644 --- a/main_test.go +++ b/main_test.go @@ -21,6 +21,7 @@ package main import ( + "errors" "testing" "github.com/codegangsta/cli" @@ -160,6 +161,38 @@ func TestValidateArgumentPass(t *testing.T) { }) } +func TestRunPprofCommand(t *testing.T) { + mockOSWrapper := new(mockOSWrapper) + pprofer := defaultPprofer{ + osWrapper: mockOSWrapper, + } + + mockOSWrapper.On("cmdOutput", mock.AnythingOfType("*exec.Cmd")).Return([]byte("output"), nil).Once() + + sampleArgs := []string{"-seconds", "15", "http://localhost:8080"} + out, err := pprofer.runPprofCommand(sampleArgs...) + + assert.Equal(t, []byte("output"), out) + assert.NoError(t, err) + mockOSWrapper.AssertExpectations(t) +} + +func TestRunPprofCommandUnderlyingError(t *testing.T) { + mockOSWrapper := new(mockOSWrapper) + pprofer := defaultPprofer{ + osWrapper: mockOSWrapper, + } + + mockOSWrapper.On("cmdOutput", mock.AnythingOfType("*exec.Cmd")).Return(nil, errors.New("pprof underlying error")).Once() + + sampleArgs := []string{"-seconds", "15", "http://localhost:8080"} + out, err := pprofer.runPprofCommand(sampleArgs...) + + assert.Equal(t, 0, len(out)) + assert.Error(t, err) + mockOSWrapper.AssertExpectations(t) +} + func TestNewTorcher(t *testing.T) { assert.NotNil(t, newTorcher()) } diff --git a/mocks.go b/mocks.go index 64e10c8..9f178a7 100644 --- a/mocks.go +++ b/mocks.go @@ -21,6 +21,8 @@ package main import ( + "os/exec" + "github.com/codegangsta/cli" "github.com/stretchr/testify/mock" ) @@ -74,6 +76,22 @@ type mockPprofer struct { mock.Mock } +type mockOSWrapper struct { + mock.Mock +} + +func (m *mockOSWrapper) cmdOutput(_a0 *exec.Cmd) ([]byte, error) { + ret := m.Called(_a0) + + var r0 []byte + if ret.Get(0) != nil { + r0 = ret.Get(0).([]byte) + } + r1 := ret.Error(1) + + return r0, r1 +} + func (m *mockPprofer) runPprofCommand(args ...string) ([]byte, error) { ret := m.Called(args) From aa7083686a2559378d22f0f0432babaeebabd1a2 Mon Sep 17 00:00:00 2001 From: Ben Sandler Date: Tue, 28 Jul 2015 22:09:39 +0000 Subject: [PATCH 3/5] Added test to ensure go-torch handles a pprof bug --- main_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/main_test.go b/main_test.go index 63585c8..9caee7b 100644 --- a/main_test.go +++ b/main_test.go @@ -193,6 +193,25 @@ func TestRunPprofCommandUnderlyingError(t *testing.T) { mockOSWrapper.AssertExpectations(t) } +// 'go tool pprof' doesn't exit on errors with nonzero status codes. This test +// ensures that go-torch will detect undrlying errors despite the pprof bug. +// See pprof issue here https://github.com/golang/go/issues/11510 +func TestRunPprofCommandHandlePprofErrorBug(t *testing.T) { + mockOSWrapper := new(mockOSWrapper) + pprofer := defaultPprofer{ + osWrapper: mockOSWrapper, + } + + mockOSWrapper.On("cmdOutput", mock.AnythingOfType("*exec.Cmd")).Return([]byte{}, nil).Once() + + sampleArgs := []string{"-seconds", "15", "http://localhost:8080"} + out, err := pprofer.runPprofCommand(sampleArgs...) + + assert.Equal(t, 0, len(out)) + assert.Error(t, err) + mockOSWrapper.AssertExpectations(t) +} + func TestNewTorcher(t *testing.T) { assert.NotNil(t, newTorcher()) } From acea47afe33ae94d408f995dae96a698c03d8b45 Mon Sep 17 00:00:00 2001 From: Ben Sandler Date: Tue, 28 Jul 2015 22:25:04 +0000 Subject: [PATCH 4/5] Removed unnecessary code from struct declarations --- graph/graph.go | 6 +++--- main.go | 4 ++-- visualization/visualization.go | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/graph/graph.go b/graph/graph.go index bb68eec..e8b8caf 100644 --- a/graph/graph.go +++ b/graph/graph.go @@ -46,8 +46,8 @@ type Grapher interface { } type defaultGrapher struct { - searcher searcher - collectionGetter collectionGetter + searcher + collectionGetter } type searchArgs struct { @@ -64,7 +64,7 @@ type searcher interface { } type defaultSearcher struct { - pathStringer pathStringer + pathStringer } type collectionGetter interface { diff --git a/main.go b/main.go index d10e2f2..d006a8d 100644 --- a/main.go +++ b/main.go @@ -39,7 +39,7 @@ import ( ) type torcher struct { - commander commander + commander } type commander interface { @@ -70,7 +70,7 @@ type pprofer interface { } type defaultPprofer struct { - osWrapper osWrapper + osWrapper } // newTorcher returns a torcher struct with a default commander diff --git a/visualization/visualization.go b/visualization/visualization.go index fc8114e..2343ba5 100644 --- a/visualization/visualization.go +++ b/visualization/visualization.go @@ -44,7 +44,7 @@ type Visualizer interface { } type defaultVisualizer struct { - executor executor + executor } type osWrapper interface { @@ -60,7 +60,7 @@ type executor interface { } type defaultExecutor struct { - osWrapper osWrapper + osWrapper } func newExecutor() executor { From e4ab64aec18f88939a93460e577ad58d0a8d3d70 Mon Sep 17 00:00:00 2001 From: Ben Sandler Date: Tue, 28 Jul 2015 22:30:15 +0000 Subject: [PATCH 5/5] Fail immediately if os.Getwd returns an error in tests --- visualization/visualization_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/visualization/visualization_test.go b/visualization/visualization_test.go index 64f82ad..aed1316 100644 --- a/visualization/visualization_test.go +++ b/visualization/visualization_test.go @@ -102,7 +102,10 @@ func TestRunPerlScriptDoesExist(t *testing.T) { executor := defaultExecutor{ osWrapper: mockOSWrapper, } - cwd, _ := os.Getwd() + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err.Error()) + } mockOSWrapper.On("execLookPath", "flamegraph.pl").Return("", errors.New("DNE")).Once() mockOSWrapper.On("execLookPath", cwd+"/flamegraph.pl").Return("", errors.New("DNE")).Once() mockOSWrapper.On("execLookPath", "flame-graph-gen").Return("/somepath/flame-graph-gen", nil).Once() @@ -121,7 +124,10 @@ func TestRunPerlScriptDoesNotExist(t *testing.T) { executor := defaultExecutor{ osWrapper: mockOSWrapper, } - cwd, _ := os.Getwd() + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err.Error()) + } mockOSWrapper.On("execLookPath", "flamegraph.pl").Return("", errors.New("DNE")).Once() mockOSWrapper.On("execLookPath", cwd+"/flamegraph.pl").Return("", errors.New("DNE")).Once() mockOSWrapper.On("execLookPath", "flame-graph-gen").Return("", errors.New("DNE")).Once()