Refactor FixtureProcesses

- Remove the logic from the constructors
- Have start take a configuration map for the fixture processes
- Move the testing on open ports closer to the actual start

pkg/framework/test/apiserver.go

@@ -16,10 +16,8 @@ import (
 type APIServer struct {
 	// The path to the apiserver binary
 	Path           string
-	EtcdURL        string
 	ProcessStarter simpleSessionStarter
 	CertDirManager certDirManager
-	APIServerURL   string
 	session        SimpleSession
 	stdOut         *gbytes.Buffer
 	stdErr         *gbytes.Buffer
@@ -33,24 +31,22 @@ type certDirManager interface {
 //go:generate counterfeiter . certDirManager
 
 // NewAPIServer creates a new APIServer Fixture Process
-func NewAPIServer(pathToAPIServer, apiServerURL, etcdURL string) *APIServer {
+func NewAPIServer(pathToAPIServer string) *APIServer {
 	starter := func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) {
 		return gexec.Start(command, out, err)
 	}
 
 	apiserver := &APIServer{
 		Path:           pathToAPIServer,
-		EtcdURL:        etcdURL,
 		ProcessStarter: starter,
 		CertDirManager: NewTempDirManager(),
-		APIServerURL:   apiServerURL,
 	}
 
 	return apiserver
 }
 
 // Start starts the apiserver, waits for it to come up, and returns an error, if occoured.
-func (s *APIServer) Start() error {
+func (s *APIServer) Start(config map[string]string) error {
 	s.stdOut = gbytes.NewBuffer()
 	s.stdErr = gbytes.NewBuffer()
 
@@ -59,7 +55,16 @@ func (s *APIServer) Start() error {
 		return err
 	}
 
-	url, err := url.Parse(s.APIServerURL)
+	etcdURL, ok := config["etcdURL"]
+	if !ok {
+		return fmt.Errorf("config setting 'etcdURL' not found")
+	}
+	apiServerURL, ok := config["apiServerURL"]
+	if !ok {
+		return fmt.Errorf("config setting 'apiServerURL' not found")
+	}
+
+	url, err := url.Parse(apiServerURL)
 	if err != nil {
 		return err
 	}
@@ -72,7 +77,7 @@ func (s *APIServer) Start() error {
 		"--admission-control-config-file=",
 		"--bind-address=0.0.0.0",
 		"--storage-backend=etcd3",
-		fmt.Sprintf("--etcd-servers=%s", s.EtcdURL),
+		fmt.Sprintf("--etcd-servers=%s", etcdURL),
 		fmt.Sprintf("--cert-dir=%s", certDir),
 		fmt.Sprintf("--insecure-port=%s", url.Port()),
 		fmt.Sprintf("--insecure-bind-address=%s", url.Hostname()),

pkg/framework/test/apiserver_test.go

@@ -20,6 +20,7 @@ var _ = Describe("Apiserver", func() {
 		fakeSession        *testfakes.FakeSimpleSession
 		fakeCertDirManager *testfakes.FakeCertDirManager
 		apiServer          *APIServer
+		apiServerConfig    map[string]string
 	)
 
 	BeforeEach(func() {
@@ -28,9 +29,13 @@ var _ = Describe("Apiserver", func() {
 
 		apiServer = &APIServer{
 			Path:           "",
-			EtcdURL:        "the etcd url",
 			CertDirManager: fakeCertDirManager,
 		}
+
+		apiServerConfig = map[string]string{
+			"apiServerURL": "http://this.is.the.API.server:8080",
+			"etcdURL":      "http://this.is.etcd:2345/",
+		}
 	})
 
 	Context("when given a path to a binary that runs for a long time", func() {
@@ -43,12 +48,12 @@ var _ = Describe("Apiserver", func() {
 			fakeSession.ExitCodeReturnsOnCall(1, 143)
 
 			apiServer.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) {
-				fmt.Fprint(err, "Serving insecurely on 127.0.0.1:8080")
+				fmt.Fprint(err, "Serving insecurely on this.is.the.API.server:8080")
 				return fakeSession, nil
 			}
 
 			By("Starting the API Server")
-			err := apiServer.Start()
+			err := apiServer.Start(apiServerConfig)
 			Expect(err).NotTo(HaveOccurred())
 
 			Eventually(apiServer).Should(gbytes.Say("Everything is fine"))
@@ -77,7 +82,7 @@ var _ = Describe("Apiserver", func() {
 				return fakeSession, nil
 			}
 
-			err := apiServer.Start()
+			err := apiServer.Start(apiServerConfig)
 			Expect(err).To(MatchError(ContainSubstring("Error on cert directory creation.")))
 			Expect(processStarterCounter).To(Equal(0))
 		})
@@ -89,7 +94,7 @@ var _ = Describe("Apiserver", func() {
 				return nil, fmt.Errorf("Some error in the apiserver starter.")
 			}
 
-			err := apiServer.Start()
+			err := apiServer.Start(apiServerConfig)
 			Expect(err).To(MatchError(ContainSubstring("Some error in the apiserver starter.")))
 		})
 	})

pkg/framework/test/democli/integration/integration_suite_test.go

@@ -42,8 +42,7 @@ var _ = BeforeSuite(func() {
 	Expect(assetsDir).NotTo(BeEmpty(),
 		"Could not determine assets directory (Hint: you can set $KUBE_ASSETS_DIR)")
 
-	fixtures, err = test.NewFixtures(filepath.Join(assetsDir, "etcd"), filepath.Join(assetsDir, "kube-apiserver"))
-	Expect(err).NotTo(HaveOccurred())
+	fixtures = test.NewFixtures(filepath.Join(assetsDir, "etcd"), filepath.Join(assetsDir, "kube-apiserver"))
 	err = fixtures.Start()
 	Expect(err).NotTo(HaveOccurred())
 })

pkg/framework/test/etcd.go

@@ -16,8 +16,6 @@ import (
 // Etcd knows how to run an etcd server. Set it up with the path to a precompiled binary.
 type Etcd struct {
 	Path           string
-	EtcdURL        string
-	EtcdPeerURL    string
 	ProcessStarter simpleSessionStarter
 	DataDirManager dataDirManager
 	session        SimpleSession
@@ -45,15 +43,13 @@ type SimpleSession interface {
 type simpleSessionStarter func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error)
 
 // NewEtcd constructs an Etcd Fixture Process
-func NewEtcd(pathToEtcd, etcdURL, etcdPeerURL string) *Etcd {
+func NewEtcd(pathToEtcd string) *Etcd {
 	starter := func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) {
 		return gexec.Start(command, out, err)
 	}
 
 	etcd := &Etcd{
 		Path:           pathToEtcd,
-		EtcdURL:        etcdURL,
-		EtcdPeerURL:    etcdPeerURL,
 		ProcessStarter: starter,
 		DataDirManager: NewTempDirManager(),
 	}
@@ -62,7 +58,7 @@ func NewEtcd(pathToEtcd, etcdURL, etcdPeerURL string) *Etcd {
 }
 
 // Start starts the etcd, waits for it to come up, and returns an error, if occoured.
-func (e *Etcd) Start() error {
+func (e *Etcd) Start(config map[string]string) error {
 	e.stdOut = gbytes.NewBuffer()
 	e.stdErr = gbytes.NewBuffer()
 
@@ -71,19 +67,28 @@ func (e *Etcd) Start() error {
 		return err
 	}
 
+	clientURL, ok := config["clientURL"]
+	if !ok {
+		return fmt.Errorf("config setting 'clientURL' not found")
+	}
+	peerURL, ok := config["peerURL"]
+	if !ok {
+		return fmt.Errorf("config setting 'peerURL' not found")
+	}
+
 	args := []string{
 		"--debug",
 		"--advertise-client-urls",
-		e.EtcdURL,
+		clientURL,
 		"--listen-client-urls",
-		e.EtcdURL,
+		clientURL,
 		"--listen-peer-urls",
-		e.EtcdPeerURL,
+		peerURL,
 		"--data-dir",
 		dataDir,
 	}
 
-	url, err := url.Parse(e.EtcdURL)
+	url, err := url.Parse(clientURL)
 	if err != nil {
 		return err
 	}

pkg/framework/test/etcd_test.go

@@ -20,6 +20,7 @@ var _ = Describe("Etcd", func() {
 		fakeSession        *testfakes.FakeSimpleSession
 		fakeDataDirManager *testfakes.FakeDataDirManager
 		etcd               *Etcd
+		etcdConfig         map[string]string
 	)
 
 	BeforeEach(func() {
@@ -28,9 +29,13 @@ var _ = Describe("Etcd", func() {
 
 		etcd = &Etcd{
 			Path:           "",
-			EtcdURL:        "our etcd url",
 			DataDirManager: fakeDataDirManager,
 		}
+
+		etcdConfig = map[string]string{
+			"clientURL": "http://this.is.etcd.listening.for.clients:1234",
+			"peerURL":   "http://this.is.etcd.listening.for.peers:1235",
+		}
 	})
 
 	Context("when given a path to a binary that runs for a long time", func() {
@@ -43,12 +48,12 @@ var _ = Describe("Etcd", func() {
 			fakeSession.ExitCodeReturnsOnCall(1, 143)
 
 			etcd.ProcessStarter = func(command *exec.Cmd, out, err io.Writer) (SimpleSession, error) {
-				fmt.Fprint(err, "serving insecure client requests on 127.0.0.1:2379")
+				fmt.Fprint(err, "serving insecure client requests on this.is.etcd.listening.for.clients:1234")
 				return fakeSession, nil
 			}
 
 			By("Starting the Etcd Server")
-			err := etcd.Start()
+			err := etcd.Start(etcdConfig)
 			Expect(err).NotTo(HaveOccurred())
 
 			Eventually(etcd).Should(gbytes.Say("Everything is dandy"))
@@ -77,7 +82,7 @@ var _ = Describe("Etcd", func() {
 				return fakeSession, nil
 			}
 
-			err := etcd.Start()
+			err := etcd.Start(etcdConfig)
 			Expect(err).To(MatchError(ContainSubstring("Error on directory creation.")))
 			Expect(processStarterCounter).To(Equal(0))
 		})
@@ -89,7 +94,7 @@ var _ = Describe("Etcd", func() {
 				return nil, fmt.Errorf("Some error in the starter.")
 			}
 
-			err := etcd.Start()
+			err := etcd.Start(etcdConfig)
 			Expect(err).To(MatchError(ContainSubstring("Some error in the starter.")))
 		})
 	})

pkg/framework/test/fixtures.go

@@ -24,62 +24,71 @@ type FixturesConfig struct {
 // This interface is potentially going to be expanded to e.g. allow access to the processes StdOut/StdErr
 // and other internals.
 type FixtureProcess interface {
-	Start() error
+	Start(config map[string]string) error
 	Stop()
 }
 
 //go:generate counterfeiter . FixtureProcess
 
 // NewFixtures will give you a Fixtures struct that's properly wired together.
-func NewFixtures(pathToEtcd, pathToAPIServer string) (*Fixtures, error) {
-	urls := map[string]string{
-		"etcdClients":      "",
-		"etcdPeers":        "",
-		"apiServerClients": "",
-	}
-	host := "127.0.0.1"
-
-	for name := range urls {
-		port, err := getFreePort(host)
-		if err != nil {
-			return nil, err
-		}
-		urls[name] = fmt.Sprintf("http://%s:%d", host, port)
-	}
-
+func NewFixtures(pathToEtcd, pathToAPIServer string) *Fixtures {
 	fixtures := &Fixtures{
-		Etcd:      NewEtcd(pathToEtcd, urls["etcdClients"], urls["etcdPeers"]),
-		APIServer: NewAPIServer(pathToAPIServer, urls["apiServerClients"], urls["etcdClients"]),
+		Etcd:      NewEtcd(pathToEtcd),
+		APIServer: NewAPIServer(pathToAPIServer),
 	}
 
-	fixtures.Config = FixturesConfig{
-		APIServerURL: urls["apiServerClients"],
-	}
-
-	return fixtures, nil
+	return fixtures
 }
 
 // Start will start all your fixtures. To stop them, call Stop().
 func (f *Fixtures) Start() error {
+	type configs map[string]string
+
+	etcdClientURL, err := getHTTPListenURL()
+	if err != nil {
+		return err
+	}
+	etcdPeerURL, err := getHTTPListenURL()
+	if err != nil {
+		return err
+	}
+	apiServerURL, err := getHTTPListenURL()
+	if err != nil {
+		return err
+	}
+
+	etcdConf := configs{
+		"peerURL":   etcdPeerURL,
+		"clientURL": etcdClientURL,
+	}
+	apiServerConf := configs{
+		"etcdURL":      etcdClientURL,
+		"apiServerURL": apiServerURL,
+	}
+
 	started := make(chan error)
-	starter := func(process FixtureProcess) {
-		started <- process.Start()
+	starter := func(process FixtureProcess, conf configs) {
+		started <- process.Start(conf)
 	}
-	processes := []FixtureProcess{
-		f.Etcd,
-		f.APIServer,
+	processes := map[FixtureProcess]configs{
+		f.Etcd:      etcdConf,
+		f.APIServer: apiServerConf,
 	}
 
-	for _, process := range processes {
-		go starter(process)
+	for process, config := range processes {
+		go starter(process, config)
 	}
 
-	for pendingProcesses := len(processes); pendingProcesses > 0; pendingProcesses-- {
+	for range processes {
 		if err := <-started; err != nil {
 			return err
 		}
 	}
 
+	f.Config = FixturesConfig{
+		APIServerURL: apiServerURL,
+	}
+
 	return nil
 }
 
@@ -90,6 +99,15 @@ func (f *Fixtures) Stop() error {
 	return nil
 }
 
+func getHTTPListenURL() (url string, err error) {
+	host := "127.0.0.1"
+	port, err := getFreePort(host)
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("http://%s:%d", host, port), nil
+}
+
 func getFreePort(host string) (int, error) {
 	addr, err := net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:0", host))
 	if err != nil {

pkg/framework/test/fixtures_test.go

@@ -12,8 +12,7 @@ import (
 
 var _ = Describe("Fixtures", func() {
 	It("can construct a properly wired Fixtures struct", func() {
-		f, err := NewFixtures("path to etcd", "path to apiserver")
-		Expect(err).NotTo(HaveOccurred())
+		f := NewFixtures("path to etcd", "path to apiserver")
 		Expect(f.Etcd.(*Etcd).Path).To(Equal("path to etcd"))
 		Expect(f.APIServer.(*APIServer).Path).To(Equal("path to apiserver"))
 	})

pkg/framework/test/integration/integration_test.go

@@ -4,9 +4,8 @@ import (
 	"net"
 	"time"
 
-	"net/url"
-
 	"fmt"
+	"net/url"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -15,34 +14,17 @@ import (
 
 var _ = Describe("The Testing Framework", func() {
 	It("Successfully manages the fixtures lifecycle", func() {
-		fixtures, err := test.NewFixtures(defaultPathToEtcd, defaultPathToApiserver)
-		Expect(err).NotTo(HaveOccurred())
+		fixtures := test.NewFixtures(defaultPathToEtcd, defaultPathToApiserver)
 
 		By("Starting all the fixture processes")
-		err = fixtures.Start()
+		err := fixtures.Start()
 		Expect(err).NotTo(HaveOccurred(), "Expected fixtures to start successfully")
 
-		var etcdURL, etcdPeerURL, apiServerURL *url.URL
-		etcd := fixtures.Etcd.(*test.Etcd)
-		apiServer := fixtures.APIServer.(*test.APIServer)
-
-		etcdURL, err = url.Parse(etcd.EtcdURL)
-		Expect(err).NotTo(HaveOccurred())
-		etcdPeerURL, err = url.Parse(etcd.EtcdPeerURL)
-		Expect(err).NotTo(HaveOccurred())
-		apiServerURL, err = url.Parse(apiServer.APIServerURL)
+		apiServerURL, err := url.Parse(fixtures.Config.APIServerURL)
 		Expect(err).NotTo(HaveOccurred())
 
-		isEtcdListening := isSomethingListeningOnPort(etcdURL.Host)
-		isEtcdPeerListening := isSomethingListeningOnPort(etcdPeerURL.Host)
 		isAPIServerListening := isSomethingListeningOnPort(apiServerURL.Host)
 
-		By("Ensuring Etcd is listening")
-		Expect(isEtcdListening()).To(BeTrue(),
-			fmt.Sprintf("Expected Etcd to listen on %s", etcdURL.Host))
-		Expect(isEtcdPeerListening()).To(BeTrue(),
-			fmt.Sprintf("Expected Etcd to listen for peers on %s", etcdPeerURL.Host))
-
 		By("Ensuring APIServer is listening")
 		Expect(isAPIServerListening()).To(BeTrue(),
 			fmt.Sprintf("Expected APIServer to listen on %s", apiServerURL.Host))
@@ -51,19 +33,13 @@ var _ = Describe("The Testing Framework", func() {
 		err = fixtures.Stop()
 		Expect(err).NotTo(HaveOccurred(), "Expected fixtures to stop successfully")
 
-		By("Ensuring Etcd is not listening anymore")
-		Expect(isEtcdListening()).To(BeFalse(), "Expected Etcd not to listen anymore")
-		Expect(isEtcdPeerListening()).To(BeFalse(), "Expected Etcd not to listen for peers anymore")
-
 		By("Ensuring APIServer is not listening anymore")
 		Expect(isAPIServerListening()).To(BeFalse(), "Expected APIServer not to listen anymore")
 	})
 
 	Measure("It should be fast to bring up and tear down the fixtures", func(b Benchmarker) {
 		b.Time("lifecycle", func() {
-			fixtures, err := test.NewFixtures(defaultPathToEtcd, defaultPathToApiserver)
-			Expect(err).NotTo(HaveOccurred())
-
+			fixtures := test.NewFixtures(defaultPathToEtcd, defaultPathToApiserver)
 			fixtures.Start()
 			fixtures.Stop()
 		})

pkg/framework/test/testfakes/fake_fixture_process.go

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