From 67e5906bd1d61d762938e2094b6d35a490b5faa9 Mon Sep 17 00:00:00 2001 From: tongjingran Date: Thu, 16 Jul 2020 20:58:58 +0800 Subject: [PATCH 1/2] optimize statement and encapsulate logic --- cmd/profile.go | 8 +-- pkg/cover/client.go | 2 +- pkg/cover/client_test.go | 2 +- pkg/cover/server.go | 122 ++++++++++++++++++++------------------- pkg/cover/server_test.go | 39 +++++++++---- 5 files changed, 97 insertions(+), 76 deletions(-) diff --git a/cmd/profile.go b/cmd/profile.go index 1050928..4d9fa9d 100644 --- a/cmd/profile.go +++ b/cmd/profile.go @@ -38,10 +38,10 @@ goc profile # Get coverage counter from specified register center, the result output to specified file. goc profile --center=http://192.168.1.1:8080 --output=./coverage.cov -# Get coverage counter of several specified services. You can get all available service names from command 'goc list'. Use 'service' and 'address' flag at the same time is illegal. +# Get coverage counter of several specified services. You can get all available service names from command 'goc list'. Use 'service' and 'address' flag at the same time may cause ambiguity, please use them separately. goc profile --service=service1,service2,service3 -# Get coverage counter of several specified addresses. You can get all available addresses from command 'goc list'. Use 'service' and 'address' flag at the same time is illegal. +# Get coverage counter of several specified addresses. You can get all available addresses from command 'goc list'. Use 'service' and 'address' flag at the same time may cause ambiguity, please use them separately. goc profile --address=address1,address2,address3 # Force to get the coverage counter of all the available services you want. @@ -81,8 +81,8 @@ var addrList []string func init() { profileCmd.Flags().StringVarP(&output, "output", "o", "", "download cover profile") - profileCmd.Flags().StringSliceVarP(&svrList, "service", "", nil, "get the cover profile of these services, you can get all available service names from command `goc list`, use this flag and 'address' flag at the same time is illegal.") - profileCmd.Flags().StringSliceVarP(&addrList, "address", "", nil, "get the cover profile of these addresses, you can get all available addresses from command `goc list`, use this flag and 'service' flag at the same time is illegal.") + profileCmd.Flags().StringSliceVarP(&svrList, "service", "", nil, "get the cover profile of these services, you can get all available service names from command `goc list`, use this flag and 'address' flag at the same time may cause ambiguity, please use them separately.") + profileCmd.Flags().StringSliceVarP(&addrList, "address", "", nil, "get the cover profile of these addresses, you can get all available addresses from command `goc list`, use this flag and 'service' flag at the same time may cause ambiguity, please use them separately.") profileCmd.Flags().BoolVarP(&force, "force", "f", false, "force to get the coverage counter of all the available services you want") addBasicFlags(profileCmd.Flags()) rootCmd.AddCommand(profileCmd) diff --git a/pkg/cover/client.go b/pkg/cover/client.go index 2c8f030..7bdbb44 100644 --- a/pkg/cover/client.go +++ b/pkg/cover/client.go @@ -92,7 +92,7 @@ func (c *client) ListServices() ([]byte, error) { func (c *client) Profile(param ProfileParam) ([]byte, error) { u := fmt.Sprintf("%s%s?force=%s", c.Host, CoverProfileAPI, strconv.FormatBool(param.Force)) if len(param.Service) != 0 && len(param.Address) != 0 { - return nil, fmt.Errorf("use 'service' and 'address' flag at the same time is illegal") + return nil, fmt.Errorf("use 'service' flag and 'address' flag at the same time may cause ambiguity, please use them separately") } for _, svr := range param.Service { diff --git a/pkg/cover/client_test.go b/pkg/cover/client_test.go index 9bff493..f097593 100644 --- a/pkg/cover/client_test.go +++ b/pkg/cover/client_test.go @@ -68,7 +68,7 @@ func TestClientAction(t *testing.T) { { service: Service{Name: "serviceOK", Address: profileSuccessMockSvr.URL}, param: ProfileParam{Force: false, Service: []string{"serviceOK"}, Address: []string{profileSuccessMockSvr.URL}}, - res: "use 'service' and 'address' flag at the same time is illegal", + res: "use 'service' flag and 'address' flag at the same time may cause ambiguity, please use them separately", }, { service: Service{Name: "serviceOK", Address: profileSuccessMockSvr.URL}, diff --git a/pkg/cover/server.go b/pkg/cover/server.go index 10da691..96b2421 100644 --- a/pkg/cover/server.go +++ b/pkg/cover/server.go @@ -138,35 +138,33 @@ func profile(c *gin.Context) { c.JSON(http.StatusExpectationFailed, gin.H{"error": "invalid param"}) return } - svrList := c.QueryArray("service") - addrList := c.QueryArray("address") + svrList := removeDuplicateElement(c.QueryArray("service")) + addrList := removeDuplicateElement(c.QueryArray("address")) svrsAll := DefaultStore.GetAll() - svrsUnderTest, err := getSvrUnderTest(svrList, addrList, force, svrsAll) + filterAddrList, err := filterAddrs(svrList, addrList, force, svrsAll) if err != nil { c.JSON(http.StatusExpectationFailed, gin.H{"error": err.Error()}) } var mergedProfiles = make([][]*cover.Profile, 0) - for _, svrs := range svrsUnderTest { - for _, addr := range svrs { - pp, err := NewWorker(addr).Profile(ProfileParam{}) - if err != nil { - if force { - continue - } - c.JSON(http.StatusExpectationFailed, gin.H{"error": err.Error()}) - return + for _, addr := range filterAddrList { + pp, err := NewWorker(addr).Profile(ProfileParam{}) + if err != nil { + if force { + continue } - profile, err := convertProfile(pp) - if err != nil { - if force { - continue - } - c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) - return - } - mergedProfiles = append(mergedProfiles, profile) + c.JSON(http.StatusExpectationFailed, gin.H{"error": err.Error()}) + return } + profile, err := convertProfile(pp) + if err != nil { + if force { + continue + } + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + mergedProfiles = append(mergedProfiles, profile) } if len(mergedProfiles) == 0 { @@ -235,48 +233,52 @@ func contains(arr []string, str string) bool { return false } -// getSvrUnderTest get service map by service and address list -func getSvrUnderTest(svrList, addrList []string, force bool, svrsAll map[string][]string) (svrsUnderTest map[string][]string, err error) { - svrsUnderTest = map[string][]string{} +// filterAddrs filter address list by given service and address list +func filterAddrs(svrList, addrList []string, force bool, svrsAll map[string][]string) (addrs []string, err error) { + addrs = []string{} + addrAll := []string{} + for _, addr := range svrsAll { + addrAll = append(addrAll, addr...) + } if len(svrList) != 0 && len(addrList) != 0 { - return nil, fmt.Errorf("use this flag and 'address' flag at the same time is illegal") + return nil, fmt.Errorf("use 'service' flag and 'address' flag at the same time may cause ambiguity, please use them separately") + } + // Add matched services to map + for _, name := range svrList { + if addr, ok := svrsAll[name]; ok { + addrs = append(addrs, addr...) + continue // jump to match the next service + } + if !force { + return nil, fmt.Errorf("service [%s] not found", name) + } + } + // Add matched addresses to map + for _, addr := range addrList { + if contains(addrAll, addr) { + addrs = append(addrs, addr) + continue + } + if !force { + return nil, fmt.Errorf("address [%s] not found", addr) + } + } + if len(addrList) == 0 && len(svrList) == 0 { + addrs = addrAll } // Return all servers when all param is nil - if len(svrList) == 0 && len(addrList) == 0 { - return svrsAll, nil - } else { - // Add matched services to map - if len(svrList) != 0 { - for _, name := range svrList { - if addr, ok := svrsAll[name]; ok { - svrsUnderTest[name] = addr - continue // jump to match the next service - } - if !force { - return nil, fmt.Errorf("service [%s] not found", name) - } - } - } - // Add matched addresses to map - if len(addrList) != 0 { - I: - for _, addr := range addrList { - for svr, addrs := range svrsAll { - if contains(svrsUnderTest[svr], addr) { - continue I // The address is duplicate, jump over - } - for _, a := range addrs { - if a == addr { - svrsUnderTest[svr] = append(svrsUnderTest[svr], a) - continue I // jump to match the next address - } - } - } - if !force { - return nil, fmt.Errorf("address [%s] not found", addr) - } - } + return addrs, nil +} + +// removeDuplicateElement remove duplicate element in slice +func removeDuplicateElement(addrs []string) []string { + result := make([]string, 0, len(addrs)) + temp := map[string]struct{}{} + for _, item := range addrs { + if _, ok := temp[item]; !ok { + temp[item] = struct{}{} + result = append(result, item) } } - return svrsUnderTest, nil + return result } diff --git a/pkg/cover/server_test.go b/pkg/cover/server_test.go index 1e6d928..0ffd283 100644 --- a/pkg/cover/server_test.go +++ b/pkg/cover/server_test.go @@ -10,51 +10,70 @@ func TestContains(t *testing.T) { assert.Equal(t, contains([]string{"a", "b"}, "c"), false) } -func TestGetSvrUnderTest(t *testing.T) { +func TestFilterAddrs(t *testing.T) { svrAll := map[string][]string{ "service1": {"http://127.0.0.1:7777", "http://127.0.0.1:8888"}, "service2": {"http://127.0.0.1:9999"}, } + addrAll := []string{} + for _, addr := range svrAll { + addrAll = append(addrAll, addr...) + } items := []struct { svrList []string addrList []string force bool err string - svrRes map[string][]string + addrRes []string }{ { svrList: []string{"service1"}, addrList: []string{"http://127.0.0.1:7777"}, - err: "use this flag and 'address' flag at the same time is illegal", + err: "use 'service' flag and 'address' flag at the same time may cause ambiguity, please use them separately", }, { - svrRes: svrAll, + addrRes: addrAll, }, { svrList: []string{"service1", "unknown"}, err: "service [unknown] not found", }, { - svrList: []string{"service1", "service1", "service2", "unknown"}, + svrList: []string{"service1", "service2", "unknown"}, force: true, - svrRes: svrAll, + addrRes: addrAll, + }, + { + svrList: []string{"unknown"}, + force: true, + addrRes: []string{}, }, { addrList: []string{"http://127.0.0.1:7777", "http://127.0.0.2:7777"}, err: "address [http://127.0.0.2:7777] not found", }, { - addrList: []string{"http://127.0.0.1:7777", "http://127.0.0.1:7777", "http://127.0.0.1:9999", "http://127.0.0.2:7777"}, + addrList: []string{"http://127.0.0.1:7777", "http://127.0.0.1:9999", "http://127.0.0.2:7777"}, force: true, - svrRes: map[string][]string{"service1": {"http://127.0.0.1:7777"}, "service2": {"http://127.0.0.1:9999"}}, + addrRes: []string{"http://127.0.0.1:7777", "http://127.0.0.1:9999"}, }, } for _, item := range items { - svrs, err := getSvrUnderTest(item.svrList, item.addrList, item.force, svrAll) + addrs, err := filterAddrs(item.svrList, item.addrList, item.force, svrAll) if err != nil { assert.Equal(t, err.Error(), item.err) } else { - assert.Equal(t, svrs, item.svrRes) + if len(addrs) == 0 { + assert.Equal(t, addrs, item.addrRes) + } + for _, a := range addrs { + assert.Contains(t, item.addrRes, a) + } } } } + +func TestRemoveDuplicateElement(t *testing.T) { + strArr := []string{"a", "a", "b"} + assert.Equal(t, removeDuplicateElement(strArr), []string{"a", "b"}) +} From 627d7e14eeb79d5857a75ccb7ae6ec5312db1cf5 Mon Sep 17 00:00:00 2001 From: tongjingran Date: Fri, 17 Jul 2020 14:50:03 +0800 Subject: [PATCH 2/2] standardize naming --- pkg/cover/client_test.go | 5 ----- pkg/cover/server.go | 44 ++++++++++++++++++++-------------------- pkg/cover/server_test.go | 1 - 3 files changed, 22 insertions(+), 28 deletions(-) diff --git a/pkg/cover/client_test.go b/pkg/cover/client_test.go index f097593..687eb1c 100644 --- a/pkg/cover/client_test.go +++ b/pkg/cover/client_test.go @@ -94,11 +94,6 @@ func TestClientAction(t *testing.T) { service: Service{Name: "serviceErr", Address: profileErrMockSvr.URL}, res: "bad mode line: error", }, - { - service: Service{Name: "serviceErr", Address: profileErrMockSvr.URL}, - param: ProfileParam{Force: true}, - res: "no profiles", - }, { service: Service{Name: "serviceNotExist", Address: "http://172.0.0.2:7777"}, res: "connection refused", diff --git a/pkg/cover/server.go b/pkg/cover/server.go index 96b2421..3b36122 100644 --- a/pkg/cover/server.go +++ b/pkg/cover/server.go @@ -138,12 +138,13 @@ func profile(c *gin.Context) { c.JSON(http.StatusExpectationFailed, gin.H{"error": "invalid param"}) return } - svrList := removeDuplicateElement(c.QueryArray("service")) - addrList := removeDuplicateElement(c.QueryArray("address")) - svrsAll := DefaultStore.GetAll() - filterAddrList, err := filterAddrs(svrList, addrList, force, svrsAll) + serviceList := removeDuplicateElement(c.QueryArray("service")) + addressList := removeDuplicateElement(c.QueryArray("address")) + allInfos := DefaultStore.GetAll() + filterAddrList, err := filterAddrs(serviceList, addressList, force, allInfos) if err != nil { c.JSON(http.StatusExpectationFailed, gin.H{"error": err.Error()}) + return } var mergedProfiles = make([][]*cover.Profile, 0) @@ -151,6 +152,7 @@ func profile(c *gin.Context) { pp, err := NewWorker(addr).Profile(ProfileParam{}) if err != nil { if force { + log.Warnf("get profile from [%s] failed, error: %s", addr, err.Error()) continue } c.JSON(http.StatusExpectationFailed, gin.H{"error": err.Error()}) @@ -158,9 +160,6 @@ func profile(c *gin.Context) { } profile, err := convertProfile(pp) if err != nil { - if force { - continue - } c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -234,40 +233,41 @@ func contains(arr []string, str string) bool { } // filterAddrs filter address list by given service and address list -func filterAddrs(svrList, addrList []string, force bool, svrsAll map[string][]string) (addrs []string, err error) { - addrs = []string{} - addrAll := []string{} - for _, addr := range svrsAll { - addrAll = append(addrAll, addr...) +func filterAddrs(serviceList, addressList []string, force bool, allInfos map[string][]string) (filterAddrList []string, err error) { + addressAll := []string{} + for _, addr := range allInfos { + addressAll = append(addressAll, addr...) } - if len(svrList) != 0 && len(addrList) != 0 { + if len(serviceList) != 0 && len(addressList) != 0 { return nil, fmt.Errorf("use 'service' flag and 'address' flag at the same time may cause ambiguity, please use them separately") } // Add matched services to map - for _, name := range svrList { - if addr, ok := svrsAll[name]; ok { - addrs = append(addrs, addr...) + for _, name := range serviceList { + if addr, ok := allInfos[name]; ok { + filterAddrList = append(filterAddrList, addr...) continue // jump to match the next service } if !force { return nil, fmt.Errorf("service [%s] not found", name) } + log.Warnf("service [%s] not found", name) } // Add matched addresses to map - for _, addr := range addrList { - if contains(addrAll, addr) { - addrs = append(addrs, addr) + for _, addr := range addressList { + if contains(addressAll, addr) { + filterAddrList = append(filterAddrList, addr) continue } if !force { return nil, fmt.Errorf("address [%s] not found", addr) } + log.Warnf("address [%s] not found", addr) } - if len(addrList) == 0 && len(svrList) == 0 { - addrs = addrAll + if len(addressList) == 0 && len(serviceList) == 0 { + filterAddrList = addressAll } // Return all servers when all param is nil - return addrs, nil + return filterAddrList, nil } // removeDuplicateElement remove duplicate element in slice diff --git a/pkg/cover/server_test.go b/pkg/cover/server_test.go index 0ffd283..61f65ed 100644 --- a/pkg/cover/server_test.go +++ b/pkg/cover/server_test.go @@ -46,7 +46,6 @@ func TestFilterAddrs(t *testing.T) { { svrList: []string{"unknown"}, force: true, - addrRes: []string{}, }, { addrList: []string{"http://127.0.0.1:7777", "http://127.0.0.2:7777"},