[FAB-3473] Improve UT coverage for peer/clilogging

This CR reworks some of the peer/clilogging commands. With these changes in place and updates to the unit tests, the unit test coverage for the package improves from 25% to 66%. Further work is still needed to improve the coverage for this package and other peer CLI packages but this is at least a start. Change-Id: I77df0ca7af7565953ea43e287067d530a7a9dc6d Signed-off-by: Will Lahti <[email protected]>
hyperledger · May 1, 2017 · f1437a8 · f1437a8
1 parent 47778ba
commit f1437a8
Show file tree

Hide file tree

Showing 8 changed files with 180 additions and 166 deletions.
diff --git a/peer/clilogging/common.go b/peer/clilogging/common.go
@@ -19,10 +19,31 @@ package clilogging
 import (
 	"github.com/hyperledger/fabric/common/errors"
 	"github.com/hyperledger/fabric/peer/common"
+	pb "github.com/hyperledger/fabric/protos/peer"
 
 	"github.com/spf13/cobra"
 )
 
+// LoggingCmdFactory holds the clients used by LoggingCmd
+type LoggingCmdFactory struct {
+	AdminClient pb.AdminClient
+}
+
+// InitCmdFactory init the LoggingCmdFactory with default admin client
+func InitCmdFactory() (*LoggingCmdFactory, error) {
+	var err error
+	var adminClient pb.AdminClient
+
+	adminClient, err = common.GetAdminClient()
+	if err != nil {
+		return nil, err
+	}
+
+	return &LoggingCmdFactory{
+		AdminClient: adminClient,
+	}, nil
+}
+
 func checkLoggingCmdParams(cmd *cobra.Command, args []string) error {
 	var err error
 	if cmd.Name() == "revertlevels" {

diff --git a/peer/clilogging/getlevel.go b/peer/clilogging/getlevel.go
@@ -17,42 +17,36 @@ limitations under the License.
 package clilogging
 
 import (
-	"github.com/hyperledger/fabric/peer/common"
 	pb "github.com/hyperledger/fabric/protos/peer"
 
 	"github.com/spf13/cobra"
 	"golang.org/x/net/context"
 )
 
-func getLevelCmd() *cobra.Command {
-	return loggingGetLevelCmd
-}
+func getLevelCmd(cf *LoggingCmdFactory) *cobra.Command {
+	var loggingGetLevelCmd = &cobra.Command{
+		Use:   "getlevel <module>",
+		Short: "Returns the logging level of the requested module logger.",
+		Long:  `Returns the logging level of the requested module logger`,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return getLevel(cf, cmd, args)
+		},
+	}
 
-var loggingGetLevelCmd = &cobra.Command{
-	Use:   "getlevel <module>",
-	Short: "Returns the logging level of the requested module logger.",
-	Long:  `Returns the logging level of the requested module logger`,
-	Run: func(cmd *cobra.Command, args []string) {
-		getLevel(cmd, args)
-	},
+	return loggingGetLevelCmd
 }
 
-func getLevel(cmd *cobra.Command, args []string) (err error) {
+func getLevel(cf *LoggingCmdFactory, cmd *cobra.Command, args []string) (err error) {
 	err = checkLoggingCmdParams(cmd, args)
-
-	if err != nil {
-		logger.Warningf("Error: %s", err)
-	} else {
-		adminClient, err := common.GetAdminClient()
-		if err != nil {
-			logger.Warningf("%s", err)
-			return err
+	if err == nil {
+		if cf == nil {
+			cf, err = InitCmdFactory()
+			if err != nil {
+				return err
+			}
 		}
-
-		logResponse, err := adminClient.GetModuleLogLevel(context.Background(), &pb.LogLevelRequest{LogModule: args[0]})
-
+		logResponse, err := cf.AdminClient.GetModuleLogLevel(context.Background(), &pb.LogLevelRequest{LogModule: args[0]})
 		if err != nil {
-			logger.Warningf("Error retrieving log level")
 			return err
 		}
 		logger.Infof("Current log level for peer module '%s': %s", logResponse.LogModule, logResponse.LogLevel)

diff --git a/peer/clilogging/logging.go b/peer/clilogging/logging.go
@@ -29,10 +29,10 @@ const loggingFuncName = "logging"
 var logger = flogging.MustGetLogger("cli/logging")
 
 // Cmd returns the cobra command for Logging
-func Cmd() *cobra.Command {
-	loggingCmd.AddCommand(getLevelCmd())
-	loggingCmd.AddCommand(setLevelCmd())
-	loggingCmd.AddCommand(revertLevelsCmd())
+func Cmd(cf *LoggingCmdFactory) *cobra.Command {
+	loggingCmd.AddCommand(getLevelCmd(cf))
+	loggingCmd.AddCommand(setLevelCmd(cf))
+	loggingCmd.AddCommand(revertLevelsCmd(cf))
 
 	return loggingCmd
 }

diff --git a/peer/clilogging/logging_test.go b/peer/clilogging/logging_test.go
@@ -16,108 +16,83 @@
 
 package clilogging
 
-import "testing"
-
-// TestGetLevelEmptyParams tests the parameter checking for getlevel, which
-// should return an error when no parameters are provided
-func TestGetLevelEmptyParams(t *testing.T) {
-	var args []string
-
-	err := checkLoggingCmdParams(getLevelCmd(), args)
-
-	if err == nil {
-		t.FailNow()
-	}
+import (
+	"testing"
+
+	"github.com/hyperledger/fabric/peer/common"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+)
+
+type testCase struct {
+	name      string
+	args      []string
+	shouldErr bool
 }
 
-// TestGetLevel tests the parameter checking for getlevel, which should
-// should return a nil error when one (or more) parameters are provided
-func TestGetLevel(t *testing.T) {
-	args := make([]string, 1)
-	args[0] = "peer"
-
-	err := checkLoggingCmdParams(getLevelCmd(), args)
-
-	if err != nil {
-		t.Fatal(err)
+func initLoggingTest(command string) (*cobra.Command, *LoggingCmdFactory) {
+	adminClient := common.GetMockAdminClient(nil)
+	mockCF := &LoggingCmdFactory{
+		AdminClient: adminClient,
 	}
-}
-
-// TestSetLevelEmptyParams tests the parameter checking for setlevel, which
-// should return an error when no parameters are provided
-func TestSetLevelEmptyParams(t *testing.T) {
-	var args []string
-
-	err := checkLoggingCmdParams(setLevelCmd(), args)
-
-	if err == nil {
-		t.FailNow()
+	var cmd *cobra.Command
+	if command == "getlevel" {
+		cmd = getLevelCmd(mockCF)
+	} else if command == "setlevel" {
+		cmd = setLevelCmd(mockCF)
+	} else if command == "revertlevels" {
+		cmd = revertLevelsCmd(mockCF)
+	} else {
+		// should only happen when there's a typo in a test case below
 	}
+	return cmd, mockCF
 }
 
-// TestSetLevelEmptyParams tests the parameter checking for setlevel, which
-// should return an error when only one parameter is provided
-func TestSetLevelOneParam(t *testing.T) {
-	args := make([]string, 1)
-	args[0] = "peer"
-
-	err := checkLoggingCmdParams(setLevelCmd(), args)
-
-	if err == nil {
-		t.FailNow()
+func runTests(t *testing.T, command string, tc []testCase) {
+	cmd, _ := initLoggingTest(command)
+	assert := assert.New(t)
+	for i := 0; i < len(tc); i++ {
+		t.Run(tc[i].name, func(t *testing.T) {
+			cmd.SetArgs(tc[i].args)
+			err := cmd.Execute()
+			if tc[i].shouldErr {
+				assert.NotNil(err)
+			}
+			if !tc[i].shouldErr {
+				assert.Nil(err)
+			}
+		})
 	}
 }
 
-// TestSetLevelEmptyParams tests the parameter checking for setlevel, which
-// should return an error when an invalid log level is provided
-func TestSetLevelInvalid(t *testing.T) {
-	args := make([]string, 2)
-	args[0] = "peer"
-	args[1] = "invalidlevel"
-
-	err := checkLoggingCmdParams(setLevelCmd(), args)
-
-	if err == nil {
-		t.FailNow()
-	}
+// TestGetLevel tests getlevel with various parameters
+func TestGetLevel(t *testing.T) {
+	var tc []testCase
+	tc = append(tc,
+		testCase{"NoParameters", []string{}, true},
+		testCase{"Valid", []string{"peer"}, false},
+	)
+	runTests(t, "getlevel", tc)
 }
 
-// TestSetLevelEmptyParams tests the parameter checking for setlevel, which
-// should return a nil error when two parameters, the second of which is a
-// valid log level, are provided
+// TestStLevel tests setlevel with various parameters
 func TestSetLevel(t *testing.T) {
-	args := make([]string, 2)
-	args[0] = "peer"
-	args[1] = "debug"
-
-	err := checkLoggingCmdParams(setLevelCmd(), args)
-
-	if err != nil {
-		t.Fatal(err)
-	}
+	var tc []testCase
+	tc = append(tc,
+		testCase{"NoParameters", []string{}, true},
+		testCase{"OneParameter", []string{"peer"}, true},
+		testCase{"Valid", []string{"peer", "warning"}, false},
+		testCase{"InvalidLevel", []string{"peer", "invalidlevel"}, true},
+	)
+	runTests(t, "setlevel", tc)
 }
 
-// TestRevertLevels tests the parameter checking for revertlevels, which
-// should return a nil error when zero parameters are provided
+// TestRevertLevels tests revertlevels with various parameters
 func TestRevertLevels(t *testing.T) {
-	var args []string
-
-	err := checkLoggingCmdParams(revertLevelsCmd(), args)
-
-	if err != nil {
-		t.Fatal(err)
-	}
-}
-
-// TestRevertLevels_extraParameter tests the parameter checking for setlevel, which
-// should return an error when any amount of parameters are provided
-func TestRevertLevels_extraParameter(t *testing.T) {
-	args := make([]string, 1)
-	args[0] = "extraparameter"
-
-	err := checkLoggingCmdParams(revertLevelsCmd(), args)
-
-	if err == nil {
-		t.FailNow()
-	}
+	var tc []testCase
+	tc = append(tc,
+		testCase{"Valid", []string{}, false},
+		testCase{"ExtraParameter", []string{"peer"}, true},
+	)
+	runTests(t, "revertlevels", tc)
 }
diff --git a/peer/clilogging/revertlevels.go b/peer/clilogging/revertlevels.go
@@ -20,40 +20,33 @@ import (
 	"golang.org/x/net/context"
 
 	"github.com/golang/protobuf/ptypes/empty"
-	"github.com/hyperledger/fabric/peer/common"
 
 	"github.com/spf13/cobra"
 )
 
-func revertLevelsCmd() *cobra.Command {
+func revertLevelsCmd(cf *LoggingCmdFactory) *cobra.Command {
+	var loggingRevertLevelsCmd = &cobra.Command{
+		Use:   "revertlevels",
+		Short: "Reverts the logging levels to the levels at the end of peer startup.",
+		Long:  `Reverts the logging levels to the levels at the end of peer startup`,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return revertLevels(cf, cmd, args)
+		},
+	}
 	return loggingRevertLevelsCmd
 }
 
-var loggingRevertLevelsCmd = &cobra.Command{
-	Use:   "revertlevels",
-	Short: "Reverts the logging levels to the levels at the end of peer startup.",
-	Long:  `Reverts the logging levels to the levels at the end of peer startup`,
-	Run: func(cmd *cobra.Command, args []string) {
-		revertLevels(cmd, args)
-	},
-}
-
-func revertLevels(cmd *cobra.Command, args []string) (err error) {
+func revertLevels(cf *LoggingCmdFactory, cmd *cobra.Command, args []string) (err error) {
 	err = checkLoggingCmdParams(cmd, args)
-
-	if err != nil {
-		logger.Warningf("Error: %s", err)
-	} else {
-		adminClient, err := common.GetAdminClient()
-		if err != nil {
-			logger.Warningf("%s", err)
-			return err
+	if err == nil {
+		if cf == nil {
+			cf, err = InitCmdFactory()
+			if err != nil {
+				return err
+			}
 		}
-
-		_, err = adminClient.RevertLogLevels(context.Background(), &empty.Empty{})
-
+		_, err = cf.AdminClient.RevertLogLevels(context.Background(), &empty.Empty{})
 		if err != nil {
-			logger.Warningf("%s", err)
 			return err
 		}
 		logger.Info("Log levels reverted to the levels at the end of peer startup.")

diff --git a/peer/clilogging/setlevel.go b/peer/clilogging/setlevel.go
@@ -19,41 +19,34 @@ package clilogging
 import (
 	"golang.org/x/net/context"
 
-	"github.com/hyperledger/fabric/peer/common"
 	pb "github.com/hyperledger/fabric/protos/peer"
 
 	"github.com/spf13/cobra"
 )
 
-func setLevelCmd() *cobra.Command {
+func setLevelCmd(cf *LoggingCmdFactory) *cobra.Command {
+	var loggingSetLevelCmd = &cobra.Command{
+		Use:   "setlevel <module> <log level>",
+		Short: "Sets the logging level of the requested module logger.",
+		Long:  `Sets the logging level of the requested module logger`,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return setLevel(cf, cmd, args)
+		},
+	}
 	return loggingSetLevelCmd
 }
 
-var loggingSetLevelCmd = &cobra.Command{
-	Use:   "setlevel <module> <log level>",
-	Short: "Sets the logging level of the requested module logger.",
-	Long:  `Sets the logging level of the requested module logger`,
-	Run: func(cmd *cobra.Command, args []string) {
-		setLevel(cmd, args)
-	},
-}
-
-func setLevel(cmd *cobra.Command, args []string) (err error) {
+func setLevel(cf *LoggingCmdFactory, cmd *cobra.Command, args []string) (err error) {
 	err = checkLoggingCmdParams(cmd, args)
-
-	if err != nil {
-		logger.Warningf("Error: %s", err)
-	} else {
-		adminClient, err := common.GetAdminClient()
-		if err != nil {
-			logger.Warningf("%s", err)
-			return err
+	if err == nil {
+		if cf == nil {
+			cf, err = InitCmdFactory()
+			if err != nil {
+				return err
+			}
 		}
-
-		logResponse, err := adminClient.SetModuleLogLevel(context.Background(), &pb.LogLevelRequest{LogModule: args[0], LogLevel: args[1]})
-
+		logResponse, err := cf.AdminClient.SetModuleLogLevel(context.Background(), &pb.LogLevelRequest{LogModule: args[0], LogLevel: args[1]})
 		if err != nil {
-			logger.Warningf("%s", err)
 			return err
 		}
 		logger.Infof("Log level set for peer module '%s': %s", logResponse.LogModule, logResponse.LogLevel)