Merge pull request #63 from balajismaniam/improve-test-coverage

Automatic merge from submit-queue

Improved unit test coverage.

Relates to #62. Coverage is now at 64%.
- Refactored code for testing purposes.
- Misc. comment changes.

main.go

@@ -70,6 +70,29 @@ func main() {
 		stderrLogger.Fatalf("main.version not set! Set -ldflags \"-X main.version `git describe --tags --dirty --always`\" during build or run.")
 	}
 
+	// Parse command-line arguments.
+	noPublish, sourcesArg, whiteListArg := argsParse(nil)
+
+	// Configure the parameters for feature discovery.
+	sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg)
+	if err != nil {
+		stderrLogger.Fatalf("error occured while configuring parameters: %s", err.Error())
+	}
+
+	// Get the set of feature labels.
+	labels := createFeatureLabels(sources, labelWhiteList)
+
+	helper := APIHelpers(k8sHelpers{})
+	// Update the node with the feature labels.
+	err = updateNodeWithFeatureLabels(helper, noPublish, labels)
+	if err != nil {
+		stderrLogger.Fatalf("error occured while updating node with feature labels: %s", err.Error())
+	}
+}
+
+// argsParse parses the command line arguments passed to the program.
+// The argument argv is passed only for testing purposes.
+func argsParse(argv []string) (noPublish bool, sourcesArg []string, whiteListArg string) {
 	usage := fmt.Sprintf(`%s.
 
   Usage:
@@ -92,14 +115,20 @@ func main() {
 		ProgramName,
 	)
 
-	arguments, _ := docopt.Parse(usage, nil, true,
+	arguments, _ := docopt.Parse(usage, argv, true,
 		fmt.Sprintf("%s %s", ProgramName, version), false)
 
 	// Parse argument values as usable types.
-	noPublish := arguments["--no-publish"].(bool)
-	sourcesArg := strings.Split(arguments["--sources"].(string), ",")
-	whiteListArg := arguments["--label-whitelist"].(string)
+	noPublish = arguments["--no-publish"].(bool)
+	sourcesArg = strings.Split(arguments["--sources"].(string), ",")
+	whiteListArg = arguments["--label-whitelist"].(string)
 
+	return noPublish, sourcesArg, whiteListArg
+}
+
+// configureParameters returns all the variables required to perform feature
+// discovery based on command line arguments.
+func configureParameters(sourcesArg []string, whiteListArg string) (sources []FeatureSource, labelWhiteList *regexp.Regexp, err error) {
 	enabledSources := map[string]struct{}{}
 	for _, s := range sourcesArg {
 		enabledSources[strings.TrimSpace(s)] = struct{}{}
@@ -113,20 +142,27 @@ func main() {
 		fakeSource{},
 	}
 
-	sources := []FeatureSource{}
+	sources = []FeatureSource{}
 	for _, s := range allSources {
 		if _, enabled := enabledSources[s.Name()]; enabled {
 			sources = append(sources, s)
 		}
 	}
 
-	// compile whiteListArg regex
-	labelWhiteList, err := regexp.Compile(whiteListArg)
+	// Compile whiteListArg regex
+	labelWhiteList, err = regexp.Compile(whiteListArg)
 	if err != nil {
-		stderrLogger.Fatalf("Error parsing whitelist regex (%s): %s", whiteListArg, err)
+		stderrLogger.Printf("error parsing whitelist regex (%s): %s", whiteListArg, err)
+		return nil, nil, err
 	}
 
-	labels := Labels{}
+	return sources, labelWhiteList, nil
+}
+
+// createFeatureLabels returns the set of feature labels from the enabled
+// sources and the whitelist argument.
+func createFeatureLabels(sources []FeatureSource, labelWhiteList *regexp.Regexp) (labels Labels) {
+	labels = Labels{}
 	// Add the version of this discovery code as a node label
 	versionLabel := fmt.Sprintf("%s/%s.version", Namespace, ProgramName)
 	labels[versionLabel] = version
@@ -148,21 +184,26 @@ func main() {
 			stdoutLogger.Printf("%s = %s", name, value)
 			// Skip if label doesn't match labelWhiteList
 			if !labelWhiteList.Match([]byte(name)) {
-				stderrLogger.Printf("%s does not match the whitelist (%s) and will not be published.", name, whiteListArg)
+				stderrLogger.Printf("%s does not match the whitelist (%s) and will not be published.", name, labelWhiteList.String())
 				continue
 			}
 			labels[name] = value
 		}
 	}
+	return labels
+}
 
-	// Update the node with the node labels, unless disabled via flags.
+// updateNodeWithFeatureLabels updates the node with the feature labels, unless
+// disabled via --no-publish flag.
+func updateNodeWithFeatureLabels(helper APIHelpers, noPublish bool, labels Labels) error {
 	if !noPublish {
-		helper := APIHelpers(k8sHelpers{})
 		err := advertiseFeatureLabels(helper, labels)
 		if err != nil {
-			stderrLogger.Fatalf("failed to advertise labels: %s", err.Error())
+			stderrLogger.Printf("failed to advertise labels: %s", err.Error())
+			return err
 		}
 	}
+	return nil
 }
 
 // getFeatureLabels returns node labels for features discovered by the

main_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"regexp"
 	"testing"
 
 	. "github.com/smartystreets/goconvey/convey"
@@ -52,19 +53,31 @@ func TestDiscoveryWithMockSources(t *testing.T) {
 		var mockClient *k8sclient.Clientset
 		var mockNode *api.Node
 
-		Convey("When I successfully advertise feature labels to a node", func() {
+		Convey("When I successfully update the node with feature labels", func() {
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient).Return(mockNode, nil).Once()
 			mockAPIHelper.On("AddLabels", mockNode, fakeFeatureLabels).Return().Once()
 			mockAPIHelper.On("RemoveLabels", mockNode, prefix).Return().Once()
 			mockAPIHelper.On("UpdateNode", mockClient, mockNode).Return(nil).Once()
-			err := advertiseFeatureLabels(testHelper, fakeFeatureLabels)
+			noPublish := false
+			err := updateNodeWithFeatureLabels(testHelper, noPublish, fakeFeatureLabels)
 
 			Convey("Error is nil", func() {
 				So(err, ShouldBeNil)
 			})
 		})
 
+		Convey("When I fail to update the node with feature labels", func() {
+			expectedError := errors.New("fake error")
+			mockAPIHelper.On("GetClient").Return(nil, expectedError)
+			noPublish := false
+			err := updateNodeWithFeatureLabels(testHelper, noPublish, fakeFeatureLabels)
+
+			Convey("Error is produced", func() {
+				So(err, ShouldEqual, expectedError)
+			})
+		})
+
 		Convey("When I fail to get a mock client while advertising feature labels", func() {
 			expectedError := errors.New("fake error")
 			mockAPIHelper.On("GetClient").Return(nil, expectedError)
@@ -103,6 +116,151 @@ func TestDiscoveryWithMockSources(t *testing.T) {
 	})
 }
 
+func TestArgsParse(t *testing.T) {
+	Convey("When parsing command line arguments", t, func() {
+		argv1 := []string{"--no-publish"}
+		argv2 := []string{"--sources=fake1,fake2,fake3"}
+		argv3 := []string{"--label-whitelist=.*rdt.*"}
+		argv4 := []string{"--no-publish", "--sources=fake1,fake2,fake3"}
+
+		Convey("When --no-publish flag is passed", func() {
+			noPublish, sourcesArg, whiteListArg := argsParse(argv1)
+
+			Convey("noPublish is set and sourcesArg is set to the default value", func() {
+				So(noPublish, ShouldBeTrue)
+				So(sourcesArg, ShouldResemble, []string{"cpuid", "rdt", "pstate"})
+				So(len(whiteListArg), ShouldEqual, 0)
+			})
+		})
+
+		Convey("When --sources flag is passed and set to some values", func() {
+			noPublish, sourcesArg, whiteListArg := argsParse(argv2)
+
+			Convey("sourcesArg is set to appropriate values", func() {
+				So(noPublish, ShouldBeFalse)
+				So(sourcesArg, ShouldResemble, []string{"fake1", "fake2", "fake3"})
+				So(len(whiteListArg), ShouldEqual, 0)
+			})
+		})
+
+		Convey("When --label-whitelist flag is passed and set to some value", func() {
+			noPublish, sourcesArg, whiteListArg := argsParse(argv3)
+
+			Convey("whiteListArg is set to appropriate value and sourcesArg is set to default value", func() {
+				So(noPublish, ShouldBeFalse)
+				So(sourcesArg, ShouldResemble, []string{"cpuid", "rdt", "pstate"})
+				So(whiteListArg, ShouldResemble, ".*rdt.*")
+			})
+		})
+
+		Convey("When --no-publish and --sources flag are passed and --sources flag is set to some value", func() {
+			noPublish, sourcesArg, whiteListArg := argsParse(argv4)
+
+			Convey("--no-publish is set and sourcesArg is set to appropriate values", func() {
+				So(noPublish, ShouldBeTrue)
+				So(sourcesArg, ShouldResemble, []string{"fake1", "fake2", "fake3"})
+				So(len(whiteListArg), ShouldEqual, 0)
+			})
+		})
+	})
+}
+
+func TestConfigureParameters(t *testing.T) {
+	Convey("When configuring parameters for node feature discovery", t, func() {
+
+		Convey("When no sourcesArg and whiteListArg are passed", func() {
+			sourcesArg := []string{}
+			whiteListArg := ""
+			emptyRegexp, _ := regexp.Compile("")
+			sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg)
+
+			Convey("Error should not be produced", func() {
+				So(err, ShouldBeNil)
+			})
+			Convey("No sources or labelWhiteList are returned", func() {
+				So(len(sources), ShouldEqual, 0)
+				So(labelWhiteList, ShouldResemble, emptyRegexp)
+			})
+		})
+
+		Convey("When sourcesArg is passed", func() {
+			sourcesArg := []string{"fake"}
+			whiteListArg := ""
+			emptyRegexp, _ := regexp.Compile("")
+			sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg)
+
+			Convey("Error should not be produced", func() {
+				So(err, ShouldBeNil)
+			})
+			Convey("Proper sources are returned", func() {
+				So(len(sources), ShouldEqual, 1)
+				So(sources[0], ShouldHaveSameTypeAs, fakeSource{})
+				So(labelWhiteList, ShouldResemble, emptyRegexp)
+			})
+		})
+
+		Convey("When invalid whiteListArg is passed", func() {
+			sourcesArg := []string{""}
+			whiteListArg := "*"
+			sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg)
+
+			Convey("Error is produced", func() {
+				So(sources, ShouldBeNil)
+				So(labelWhiteList, ShouldBeNil)
+				So(err, ShouldNotBeNil)
+			})
+		})
+
+		Convey("When valid whiteListArg is passed", func() {
+			sourcesArg := []string{""}
+			whiteListArg := ".*rdt.*"
+			expectRegexp, err := regexp.Compile(".*rdt.*")
+			sources, labelWhiteList, err := configureParameters(sourcesArg, whiteListArg)
+
+			Convey("Error should not be produced", func() {
+				So(err, ShouldBeNil)
+			})
+			Convey("Proper labelWhiteList is returned", func() {
+				So(len(sources), ShouldEqual, 0)
+				So(labelWhiteList, ShouldResemble, expectRegexp)
+			})
+		})
+	})
+}
+
+func TestCreateFeatureLabels(t *testing.T) {
+	Convey("When creating feature labels from the configured sources", t, func() {
+		Convey("When fake feature source is configured", func() {
+			emptyLabelWL, _ := regexp.Compile("")
+			fakeFeatureSource := FeatureSource(new(fakeSource))
+			sources := []FeatureSource{}
+			sources = append(sources, fakeFeatureSource)
+			labels := createFeatureLabels(sources, emptyLabelWL)
+
+			Convey("Proper fake labels are returned", func() {
+				So(len(labels), ShouldEqual, 4)
+				So(labels, ShouldContainKey, prefix+"-fake-fakefeature1")
+				So(labels, ShouldContainKey, prefix+"-fake-fakefeature2")
+				So(labels, ShouldContainKey, prefix+"-fake-fakefeature3")
+			})
+		})
+		Convey("When fake feature source is configured with a whitelist that doesn't match", func() {
+			emptyLabelWL, _ := regexp.Compile(".*rdt.*")
+			fakeFeatureSource := FeatureSource(new(fakeSource))
+			sources := []FeatureSource{}
+			sources = append(sources, fakeFeatureSource)
+			labels := createFeatureLabels(sources, emptyLabelWL)
+
+			Convey("fake labels are not returned", func() {
+				So(len(labels), ShouldEqual, 1)
+				So(labels, ShouldNotContainKey, prefix+"-fake-fakefeature1")
+				So(labels, ShouldNotContainKey, prefix+"-fake-fakefeature2")
+				So(labels, ShouldNotContainKey, prefix+"-fake-fakefeature3")
+			})
+		})
+	})
+}
+
 func TestAddLabels(t *testing.T) {
 	Convey("When adding labels", t, func() {
 		helper := k8sHelpers{}

sources.go

@@ -30,6 +30,8 @@ const (
 type cpuidSource struct{}
 
 func (s cpuidSource) Name() string { return "cpuid" }
+
+// Returns feature names for all the supported CPU features.
 func (s cpuidSource) Discover() ([]string, error) {
 	// Get the cpu features as strings
 	return cpuid.CPU.Features.Strings(), nil
@@ -43,7 +45,7 @@ type rdtSource struct{}
 
 func (s rdtSource) Name() string { return "rdt" }
 
-// Returns feature names for CMT, MBM and CAT if suppported.
+// Returns feature names for CMT and CAT if suppported.
 func (s rdtSource) Discover() ([]string, error) {
 	features := []string{}
 
@@ -59,7 +61,7 @@ func (s rdtSource) Discover() ([]string, error) {
 	if err := cmd.Run(); err != nil {
 		stderrLogger.Printf("support for RDT L3 allocation was not detected: %s", err.Error())
 	} else {
-		// RDT monitoring detected.
+		// RDT L3 cache allocation detected.
 		features = append(features, "RDTL3CA")
 	}
 
@@ -67,7 +69,7 @@ func (s rdtSource) Discover() ([]string, error) {
 	if err := cmd.Run(); err != nil {
 		stderrLogger.Printf("support for RDT L2 allocation was not detected: %s", err.Error())
 	} else {
-		// RDT monitoring detected.
+		// RDT L2 cache allocation detected.
 		features = append(features, "RDTL2CA")
 	}
 
@@ -81,6 +83,8 @@ func (s rdtSource) Discover() ([]string, error) {
 type pstateSource struct{}
 
 func (s pstateSource) Name() string { return "pstate" }
+
+// Returns feature names for p-state related features such as turbo boost.
 func (s pstateSource) Discover() ([]string, error) {
 	features := []string{}