From f1437a8203ea80641ed401a43b308edaba16c7bf Mon Sep 17 00:00:00 2001 From: Will Lahti Date: Thu, 27 Apr 2017 16:34:21 -0400 Subject: [PATCH] [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 --- peer/clilogging/common.go | 21 +++++ peer/clilogging/getlevel.go | 42 ++++----- peer/clilogging/logging.go | 8 +- peer/clilogging/logging_test.go | 153 +++++++++++++------------------- peer/clilogging/revertlevels.go | 41 ++++----- peer/clilogging/setlevel.go | 41 ++++----- peer/common/mockclient.go | 38 ++++++++ peer/main.go | 2 +- 8 files changed, 180 insertions(+), 166 deletions(-) diff --git a/peer/clilogging/common.go b/peer/clilogging/common.go index 468d8030e0b..94481e6f3a7 100644 --- 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 index 4dcc7d0b633..3906ddde8d8 100644 --- 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 ", + 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 ", - 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 index a69eac3eaf6..4066bfa9acc 100644 --- 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 index f490bf49c1c..d74144ba502 100644 --- 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 index 301f720e7b9..3af8dc67e7e 100644 --- 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 index d2a4a2f5dff..a6e701e5573 100644 --- 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 ", + 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 ", - 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) diff --git a/peer/common/mockclient.go b/peer/common/mockclient.go index 2f71017d0d6..6936135dee0 100644 --- a/peer/common/mockclient.go +++ b/peer/common/mockclient.go @@ -17,6 +17,7 @@ limitations under the License. package common import ( + "github.com/golang/protobuf/ptypes/empty" cb "github.com/hyperledger/fabric/protos/common" pb "github.com/hyperledger/fabric/protos/peer" context "golang.org/x/net/context" @@ -56,3 +57,40 @@ func (m *mockBroadcastClient) Send(env *cb.Envelope) error { func (m *mockBroadcastClient) Close() error { return nil } + +func GetMockAdminClient(err error) pb.AdminClient { + return &mockAdminClient{err: err} +} + +type mockAdminClient struct { + status *pb.ServerStatus + err error +} + +func (m *mockAdminClient) GetStatus(ctx context.Context, in *empty.Empty, opts ...grpc.CallOption) (*pb.ServerStatus, error) { + return m.status, m.err +} + +func (m *mockAdminClient) StartServer(ctx context.Context, in *empty.Empty, opts ...grpc.CallOption) (*pb.ServerStatus, error) { + m.status = &pb.ServerStatus{Status: pb.ServerStatus_STARTED} + return m.status, m.err +} + +func (m *mockAdminClient) StopServer(ctx context.Context, in *empty.Empty, opts ...grpc.CallOption) (*pb.ServerStatus, error) { + m.status = &pb.ServerStatus{Status: pb.ServerStatus_STOPPED} + return m.status, m.err +} + +func (m *mockAdminClient) GetModuleLogLevel(ctx context.Context, in *pb.LogLevelRequest, opts ...grpc.CallOption) (*pb.LogLevelResponse, error) { + response := &pb.LogLevelResponse{LogModule: in.LogModule, LogLevel: "INFO"} + return response, m.err +} + +func (m *mockAdminClient) SetModuleLogLevel(ctx context.Context, in *pb.LogLevelRequest, opts ...grpc.CallOption) (*pb.LogLevelResponse, error) { + response := &pb.LogLevelResponse{LogModule: in.LogModule, LogLevel: in.LogLevel} + return response, m.err +} + +func (m *mockAdminClient) RevertLogLevels(ctx context.Context, in *empty.Empty, opts ...grpc.CallOption) (*empty.Empty, error) { + return &empty.Empty{}, m.err +} diff --git a/peer/main.go b/peer/main.go index abbdebfbf83..659a1abb7e8 100644 --- a/peer/main.go +++ b/peer/main.go @@ -98,7 +98,7 @@ func main() { mainCmd.AddCommand(version.Cmd()) mainCmd.AddCommand(node.Cmd()) mainCmd.AddCommand(chaincode.Cmd(nil)) - mainCmd.AddCommand(clilogging.Cmd()) + mainCmd.AddCommand(clilogging.Cmd(nil)) mainCmd.AddCommand(channel.Cmd(nil)) runtime.GOMAXPROCS(viper.GetInt("peer.gomaxprocs"))