From 564a7e519ab72c5ebca7ab21fb8e155819aacd3c Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Mon, 25 Jan 2016 10:49:54 +0100 Subject: [PATCH] Sanitize S2IBuilder tests And: - Kill code duplication in pkg/build/builder tests - Repair godocs - Remove dead code --- pkg/build/builder/dockerutil_test.go | 16 ++-- pkg/build/builder/sti_test.go | 127 ++++++++------------------- 2 files changed, 47 insertions(+), 96 deletions(-) diff --git a/pkg/build/builder/dockerutil_test.go b/pkg/build/builder/dockerutil_test.go index ffb52f08c979..259dc8c22c3d 100644 --- a/pkg/build/builder/dockerutil_test.go +++ b/pkg/build/builder/dockerutil_test.go @@ -10,6 +10,11 @@ type FakeDocker struct { pushImageFunc func(opts docker.PushImageOptions, auth docker.AuthConfiguration) error buildImageFunc func(opts docker.BuildImageOptions) error removeImageFunc func(name string) error + + buildImageCalled bool + pushImageCalled bool + removeImageCalled bool + errPushImage error } func (d *FakeDocker) BuildImage(opts docker.BuildImageOptions) error { @@ -18,25 +23,22 @@ func (d *FakeDocker) BuildImage(opts docker.BuildImageOptions) error { } return nil } - func (d *FakeDocker) PushImage(opts docker.PushImageOptions, auth docker.AuthConfiguration) error { + d.pushImageCalled = true if d.pushImageFunc != nil { return d.pushImageFunc(opts, auth) } - return nil + return d.errPushImage } - func (d *FakeDocker) RemoveImage(name string) error { if d.removeImageFunc != nil { return d.removeImageFunc(name) } return nil } - func (d *FakeDocker) CreateContainer(opts docker.CreateContainerOptions) (*docker.Container, error) { - return nil, nil + return &docker.Container{}, nil } - func (d *FakeDocker) DownloadFromContainer(id string, opts docker.DownloadFromContainerOptions) error { return nil } @@ -47,7 +49,7 @@ func (d *FakeDocker) RemoveContainer(opts docker.RemoveContainerOptions) error { return nil } func (d *FakeDocker) InspectImage(name string) (*docker.Image, error) { - return nil, nil + return &docker.Image{}, nil } func TestDockerPush(t *testing.T) { verifyFunc := func(opts docker.PushImageOptions, auth docker.AuthConfiguration) error { diff --git a/pkg/build/builder/sti_test.go b/pkg/build/builder/sti_test.go index 5b1c1d8c127c..4b3d8dd14b32 100644 --- a/pkg/build/builder/sti_test.go +++ b/pkg/build/builder/sti_test.go @@ -5,107 +5,66 @@ import ( "strings" "testing" - docker "github.com/fsouza/go-dockerclient" kapi "k8s.io/kubernetes/pkg/api" "github.com/openshift/origin/pkg/build/api" "github.com/openshift/origin/pkg/client/testclient" "github.com/openshift/origin/pkg/generate/git" s2iapi "github.com/openshift/source-to-image/pkg/api" - "github.com/openshift/source-to-image/pkg/api/validation" s2ibuild "github.com/openshift/source-to-image/pkg/build" ) -type testDockerClient struct { - buildImageCalled bool - pushImageCalled bool - removeImageCalled bool - errPushImage error -} - -func (client testDockerClient) BuildImage(opts docker.BuildImageOptions) error { - return nil -} - -func (client testDockerClient) PushImage(opts docker.PushImageOptions, auth docker.AuthConfiguration) error { - client.pushImageCalled = true - return client.errPushImage -} - -func (client testDockerClient) RemoveImage(name string) error { - return nil -} - -func (client testDockerClient) CreateContainer(opts docker.CreateContainerOptions) (*docker.Container, error) { - return nil, nil -} - -func (client testDockerClient) DownloadFromContainer(id string, opts docker.DownloadFromContainerOptions) error { - return nil -} - -func (client testDockerClient) PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error { - return nil -} - -func (client testDockerClient) RemoveContainer(opts docker.RemoveContainerOptions) error { - return nil -} -func (client testDockerClient) InspectImage(name string) (*docker.Image, error) { - return nil, nil -} - +// testStiBuilderFactory is a mock implementation of builderFactory. type testStiBuilderFactory struct { getStrategyErr error buildError error } -type testStiConfigValidator struct { - errors []validation.ValidationError -} - -// Mock S2I builder factory implementation. Just returns mock S2I builder instances ot error (if set) +// Builder implements builderFactory. It returns a mock S2IBuilder instance that +// returns specific errors. func (factory testStiBuilderFactory) Builder(config *s2iapi.Config, overrides s2ibuild.Overrides) (s2ibuild.Builder, error) { - // if there is error set, return this error + // Return a strategy error if non-nil. if factory.getStrategyErr != nil { return nil, factory.getStrategyErr } return testBuilder{buildError: factory.buildError}, nil } +// testBuilder is a mock implementation of s2iapi.Builder. type testBuilder struct { buildError error } -// Build is a mock implementation for STI builder, returns nil result and error if any +// Build implements s2iapi.Builder. It always returns a mocked build error. func (builder testBuilder) Build(config *s2iapi.Config) (*s2iapi.Result, error) { return nil, builder.buildError } -// creates mock implemenation of STI builder, instrumenting different parts of a process to return errors -func makeStiBuilder( - errPushImage error, - getStrategyErr error, - buildError error, - validationErrors []validation.ValidationError) S2IBuilder { - return *newS2IBuilder( - testDockerClient{ - errPushImage: errPushImage, +type testS2IBuilderConfig struct { + errPushImage error + getStrategyErr error + buildError error +} + +// newTestS2IBuilder creates a mock implementation of S2IBuilder, instrumenting +// different parts to return specific errors according to config. +func newTestS2IBuilder(config testS2IBuilderConfig) *S2IBuilder { + return newS2IBuilder( + &FakeDocker{ + errPushImage: config.errPushImage, }, "/docker.socket", testclient.NewSimpleFake().Builds(""), makeBuild(), git.NewRepository(), - testStiBuilderFactory{getStrategyErr: getStrategyErr, buildError: buildError}, - testStiConfigValidator{errors: validationErrors}, + testStiBuilderFactory{ + getStrategyErr: config.getStrategyErr, + buildError: config.buildError, + }, + runtimeConfigValidator{}, ) } -// ValidateConfig is a mock implementation for config validator. returns error if set or nil -func (validator testStiConfigValidator) ValidateConfig(config *s2iapi.Config) []validation.ValidationError { - return validator.errors -} - func makeBuild() *api.Build { return &api.Build{ Spec: api.BuildSpec{ @@ -136,40 +95,30 @@ func makeBuild() *api.Build { func TestDockerBuildError(t *testing.T) { expErr := errors.New("Artificial exception: Error building") - s2iBuilder := makeStiBuilder(expErr, nil, nil, make([]validation.ValidationError, 0)) - err := s2iBuilder.Build() - if err == nil { - t.Error("Artificial error expected from build process") - } else { - if !strings.Contains(err.Error(), expErr.Error()) { - t.Errorf("Artificial error expected from build process: \n Returned error: %s\n Expected error: %s", err.Error(), expErr.Error()) - } + s2iBuilder := newTestS2IBuilder(testS2IBuilderConfig{ + buildError: expErr, + }) + if err := s2iBuilder.Build(); err != expErr { + t.Errorf("s2iBuilder.Build() = %v; want %v", err, expErr) } } func TestPushError(t *testing.T) { expErr := errors.New("Artificial exception: Error pushing image") - s2iBuilder := makeStiBuilder(nil, nil, expErr, make([]validation.ValidationError, 0)) - err := s2iBuilder.Build() - if err == nil { - t.Error("Artificial error expected from build process") - } else { - if !strings.Contains(err.Error(), expErr.Error()) { - t.Errorf("Artificial error expected from build process: \n Returned error: %s\n Expected error: %s", err.Error(), expErr.Error()) - } + s2iBuilder := newTestS2IBuilder(testS2IBuilderConfig{ + errPushImage: expErr, + }) + if err := s2iBuilder.Build(); !strings.HasSuffix(err.Error(), expErr.Error()) { + t.Errorf("s2iBuilder.Build() = %v; want %v", err, expErr) } } -// Test error creating s2i builder func TestGetStrategyError(t *testing.T) { expErr := errors.New("Artificial exception: config error") - s2iBuilder := makeStiBuilder(nil, expErr, nil, make([]validation.ValidationError, 0)) - err := s2iBuilder.Build() - if err == nil { - t.Error("Artificial error expected from build process") - } else { - if !strings.Contains(err.Error(), expErr.Error()) { - t.Errorf("Artificial error expected from build process: \n Returned error: %s\n Expected error: %s", err.Error(), expErr.Error()) - } + s2iBuilder := newTestS2IBuilder(testS2IBuilderConfig{ + getStrategyErr: expErr, + }) + if err := s2iBuilder.Build(); err != expErr { + t.Errorf("s2iBuilder.Build() = %v; want %v", err, expErr) } }