Refactor Etcd

- Store stdout,stderr in private buffers
- Configure the etcURL on construction instead of at start time
- Handle the creation of the temporary directory (for the data
  directory) internally

pkg/framework/test/etcd.go

@@ -1,9 +1,11 @@
 package test
 
 import (
+	"io/ioutil"
+	"os"
 	"os/exec"
 
-	"github.com/onsi/ginkgo"
+	"github.com/onsi/gomega"
 	"github.com/onsi/gomega/gbytes"
 	"github.com/onsi/gomega/gexec"
 )
@@ -12,24 +14,32 @@ import (
 type Etcd struct {
 	// The path to the etcd binary
 	Path           string
+	EtcdURL        string
 	session        *gexec.Session
+	stdOut         *gbytes.Buffer
+	stdErr         *gbytes.Buffer
+	tempDirManager TempDirManager
 }
 
 // Start starts the etcd, and returns a gexec.Session. To stop it again, call Terminate and Wait on that session.
-func (e *Etcd) Start(etcdURL string, datadir string) error {
+func (e *Etcd) Start() error {
+	e.tempDirManager = &tempDirManager{}
+
+	dataDir := e.tempDirManager.Create()
+
 	args := []string{
-		"--advertise-client-urls",
-		etcdURL,
-		"--data-dir",
-		datadir,
-		"--listen-client-urls",
-		etcdURL,
 		"--debug",
+		"--advertise-client-urls",
+		e.EtcdURL,
+		"--listen-client-urls",
+		e.EtcdURL,
+		"--data-dir",
+		dataDir,
 	}
 
 	command := exec.Command(e.Path, args...)
 	var err error
-	e.session, err = gexec.Start(command, ginkgo.GinkgoWriter, ginkgo.GinkgoWriter)
+	e.session, err = gexec.Start(command, e.stdOut, e.stdErr)
 	return err
 }
 
@@ -37,6 +47,8 @@ func (e *Etcd) Start(etcdURL string, datadir string) error {
 func (e *Etcd) Stop() {
 	if e.session != nil {
 		e.session.Terminate().Wait()
+		err := e.tempDirManager.Destroy()
+		gomega.Expect(err).NotTo(gomega.HaveOccurred())
 	}
 }
 
@@ -49,3 +61,32 @@ func (e *Etcd) ExitCode() int {
 func (e *Etcd) Buffer() *gbytes.Buffer {
 	return e.session.Buffer()
 }
+
+//------
+
+// TempDirManager knows how to create and destroy temporary directories.
+type TempDirManager interface {
+	Create() string
+	Destroy() error
+}
+
+//go:generate counterfeiter . TempDirManager
+
+type tempDirManager struct {
+	dir string
+}
+
+func (t *tempDirManager) Create() string {
+	var err error
+	t.dir, err = ioutil.TempDir("", "kube-test-framework")
+	gomega.ExpectWithOffset(2, err).NotTo(gomega.HaveOccurred(),
+		"expected to be able to create a temporary directory in the kube test framework")
+	return t.dir
+}
+
+func (t *tempDirManager) Destroy() error {
+	if t.dir != "" {
+		return os.RemoveAll(t.dir)
+	}
+	return nil
+}

pkg/framework/test/etcd_test.go

@@ -15,10 +15,13 @@ var _ = Describe("Etcd", func() {
 		It("can start and stop that binary", func() {
 			pathToFakeEtcd, err := gexec.Build("k8s.io/kubectl/pkg/framework/test/assets/fakeetcd")
 			Expect(err).NotTo(HaveOccurred())
-			etcd := &Etcd{Path: pathToFakeEtcd}
+			etcd := &Etcd{
+				Path:    pathToFakeEtcd,
+				EtcdURL: "our etcd url",
+			}
 
 			By("Starting the Etcd Server")
-			err = etcd.Start("our etcd url", "our data directory")
+			err = etcd.Start()
 			Expect(err).NotTo(HaveOccurred())
 
 			Eventually(etcd).Should(gbytes.Say("Everything is dandy"))
@@ -34,7 +37,7 @@ var _ = Describe("Etcd", func() {
 	Context("when no path is given", func() {
 		It("fails with a helpful error", func() {
 			etcd := &Etcd{}
-			err := etcd.Start("our etcd url", "")
+			err := etcd.Start()
 			Expect(err).To(MatchError(ContainSubstring("no such file or directory")))
 		})
 	})
@@ -44,7 +47,7 @@ var _ = Describe("Etcd", func() {
 			etcd := &Etcd{
 				Path: "./etcd.go",
 			}
-			err := etcd.Start("our etcd url", "")
+			err := etcd.Start()
 			Expect(err).To(MatchError(ContainSubstring("./etcd.go: permission denied")))
 		})
 	})

pkg/framework/test/fixtures.go

@@ -2,10 +2,6 @@ package test
 
 import (
 	"fmt"
-	"io/ioutil"
-	"os"
-
-	"github.com/onsi/gomega"
 )
 
 // Fixtures is a struct that knows how to start all your test fixtures.
@@ -14,12 +10,11 @@ import (
 type Fixtures struct {
 	Etcd      EtcdStartStopper
 	APIServer APIServerStartStopper
-	TempDirManager TempDirManager
 }
 
 // EtcdStartStopper knows how to start an Etcd. One good implementation is Etcd.
 type EtcdStartStopper interface {
-	Start(etcdURL, datadir string) error
+	Start() error
 	Stop()
 }
 
@@ -33,46 +28,21 @@ type APIServerStartStopper interface {
 
 //go:generate counterfeiter . APIServerStartStopper
 
-// TempDirManager knows how to create and destroy temporary directories.
-type TempDirManager interface {
-	Create() string
-	Destroy() error
-}
-
-//go:generate counterfeiter . TempDirManager
-
 // NewFixtures will give you a Fixtures struct that's properly wired together.
 func NewFixtures(pathToEtcd, pathToAPIServer string) *Fixtures {
+	etcdURL := "http://127.0.0.1:2379"
 	return &Fixtures{
-		Etcd:           &Etcd{Path: pathToEtcd},
-		APIServer:      &APIServer{Path: pathToAPIServer},
-		TempDirManager: &tempDirManager{},
+		Etcd: &Etcd{
+			Path:    pathToEtcd,
+			EtcdURL: etcdURL,
+		},
+		APIServer: &APIServer{Path:pathToAPIServer},
 	}
 }
 
-type tempDirManager struct {
-	dir string
-}
-
-func (t *tempDirManager) Create() string {
-	var err error
-	t.dir, err = ioutil.TempDir("", "kube-test-framework")
-	gomega.ExpectWithOffset(2, err).NotTo(gomega.HaveOccurred(),
-		"expected to be able to create a temporary directory in the kube test framework")
-	return t.dir
-}
-
-func (t *tempDirManager) Destroy() error {
-	if t.dir != "" {
-		return os.RemoveAll(t.dir)
-	}
-	return nil
-}
-
 // Start will start all your fixtures. To stop them, call Stop().
 func (f *Fixtures) Start() error {
-	tmpDir := f.TempDirManager.Create()
-	if err := f.Etcd.Start("http://127.0.0.1:2379", tmpDir); err != nil {
+	if err := f.Etcd.Start(); err != nil {
 		return fmt.Errorf("Error starting etcd: %s", err)
 	}
 	if err := f.APIServer.Start("http://127.0.0.1:2379"); err != nil {
@@ -85,5 +55,5 @@ func (f *Fixtures) Start() error {
 func (f *Fixtures) Stop() error {
 	f.APIServer.Stop()
 	f.Etcd.Stop()
-	return f.TempDirManager.Destroy()
+	return nil
 }

pkg/framework/test/fixtures_test.go

@@ -21,36 +21,24 @@ var _ = Describe("Fixtures", func() {
 		var (
 			fakeEtcdStartStopper      *testfakes.FakeEtcdStartStopper
 			fakeAPIServerStartStopper *testfakes.FakeAPIServerStartStopper
-			fakeTempDirManager        *testfakes.FakeTempDirManager
 			fixtures                  Fixtures
 		)
 		BeforeEach(func() {
 			fakeEtcdStartStopper = &testfakes.FakeEtcdStartStopper{}
 			fakeAPIServerStartStopper = &testfakes.FakeAPIServerStartStopper{}
-			fakeTempDirManager = &testfakes.FakeTempDirManager{}
 			fixtures = Fixtures{
 				Etcd:      fakeEtcdStartStopper,
 				APIServer: fakeAPIServerStartStopper,
-				TempDirManager: fakeTempDirManager,
 			}
 		})
 
 		It("can start them", func() {
-			fakeTempDirManager.CreateReturns("some temp dir")
-
 			err := fixtures.Start()
 			Expect(err).NotTo(HaveOccurred())
 
-			By("creating a temporary directory")
-			Expect(fakeTempDirManager.CreateCallCount()).To(Equal(1),
-				"the TempDirManager should be called exactly once")
-
 			By("starting Etcd")
 			Expect(fakeEtcdStartStopper.StartCallCount()).To(Equal(1),
 				"the EtcdStartStopper should be called exactly once")
-			url, datadir := fakeEtcdStartStopper.StartArgsForCall(0)
-			Expect(url).To(Equal("http://127.0.0.1:2379"))
-			Expect(datadir).To(Equal("some temp dir"))
 
 			By("starting APIServer")
 			Expect(fakeAPIServerStartStopper.StartCallCount()).To(Equal(1),
@@ -79,18 +67,7 @@ var _ = Describe("Fixtures", func() {
 			fixtures.Stop()
 			Expect(fakeEtcdStartStopper.StopCallCount()).To(Equal(1))
 			Expect(fakeAPIServerStartStopper.StopCallCount()).To(Equal(1))
-			Expect(fakeTempDirManager.DestroyCallCount()).To(Equal(1))
 		})
 
-		Context("when cleanup fails", func() {
-			It("still stops the services, and it bubbles up the error", func() {
-				fakeTempDirManager.DestroyReturns(fmt.Errorf("deletion failed"))
-				err := fixtures.Stop()
-				Expect(err).To(MatchError(ContainSubstring("deletion failed")))
-
-				Expect(fakeEtcdStartStopper.StopCallCount()).To(Equal(1))
-				Expect(fakeAPIServerStartStopper.StopCallCount()).To(Equal(1))
-			})
-		})
 	})
 })

pkg/framework/test/testfakes/fake_etcd_start_stopper.go

@@ -8,12 +8,9 @@ import (
 )
 
 type FakeEtcdStartStopper struct {
-	StartStub        func(etcdURL, datadir string) error
+	StartStub        func() error
 	startMutex       sync.RWMutex
-	startArgsForCall []struct {
-		etcdURL string
-		datadir string
-	}
+	startArgsForCall []struct{}
 	startReturns     struct {
 		result1 error
 	}
@@ -27,17 +24,14 @@ type FakeEtcdStartStopper struct {
 	invocationsMutex sync.RWMutex
 }
 
-func (fake *FakeEtcdStartStopper) Start(etcdURL string, datadir string) error {
+func (fake *FakeEtcdStartStopper) Start() error {
 	fake.startMutex.Lock()
 	ret, specificReturn := fake.startReturnsOnCall[len(fake.startArgsForCall)]
-	fake.startArgsForCall = append(fake.startArgsForCall, struct {
-		etcdURL string
-		datadir string
-	}{etcdURL, datadir})
-	fake.recordInvocation("Start", []interface{}{etcdURL, datadir})
+	fake.startArgsForCall = append(fake.startArgsForCall, struct{}{})
+	fake.recordInvocation("Start", []interface{}{})
 	fake.startMutex.Unlock()
 	if fake.StartStub != nil {
-		return fake.StartStub(etcdURL, datadir)
+		return fake.StartStub()
 	}
 	if specificReturn {
 		return ret.result1
@@ -51,12 +45,6 @@ func (fake *FakeEtcdStartStopper) StartCallCount() int {
 	return len(fake.startArgsForCall)
 }
 
-func (fake *FakeEtcdStartStopper) StartArgsForCall(i int) (string, string) {
-	fake.startMutex.RLock()
-	defer fake.startMutex.RUnlock()
-	return fake.startArgsForCall[i].etcdURL, fake.startArgsForCall[i].datadir
-}
-
 func (fake *FakeEtcdStartStopper) StartReturns(result1 error) {
 	fake.StartStub = nil
 	fake.startReturns = struct {