Repository: knative/pkg
Pull request #1790 (2020-10-09 03:47:51)

BucketList cleanup
The `BucketList` func is only used in tests. No need to return a random order. Make the return sorted to simplify tests.

/assign @vagababov 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1790#" title="Author self-approved">yanweiguo</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/1790?src=pr&el=h1) Report
> Merging [#1790](https://codecov.io/gh/knative/pkg/pull/1790?src=pr&el=desc) into [master](https://codecov.io/gh/knative/pkg/commit/7bad84346637bdbe5b63d3022b7be1e2d91907e5?el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/1790/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO)](https://codecov.io/gh/knative/pkg/pull/1790?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1790      +/-   ##
==========================================
- Coverage   68.20%   68.19%   -0.02%     
==========================================
  Files         220      220              
  Lines        9320     9317       -3     
==========================================
- Hits         6357     6354       -3     
  Misses       2646     2646              
  Partials      317      317              
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/1790?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [hash/bucketer.go](https://codecov.io/gh/knative/pkg/pull/1790/diff?src=pr&el=tree#diff-aGFzaC9idWNrZXRlci5nbw==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/1790?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/1790?src=pr&el=footer). Last update [7bad843...10a6aa0](https://codecov.io/gh/knative/pkg/pull/1790?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/hash/bucketer.go b/hash/bucketer.go
index 41d26d97c..584b436ee 100644
--- a/hash/bucketer.go
+++ b/hash/bucketer.go
@@ -81,7 +81,7 @@ func (b *Bucket) Has(nn types.NamespacedName) bool {
 // ordered by bucket name.
 func (bs *BucketSet) Buckets() []reconciler.Bucket {
 	bkts := make([]reconciler.Bucket, len(bs.buckets))
-	for i, n := range bs.sortedBucketNames() {
+	for i, n := range bs.BucketList() {
 		bkts[i] = &Bucket{
 			name:    n,
 			buckets: bs,
@@ -90,13 +90,6 @@ func (bs *BucketSet) Buckets() []reconciler.Bucket {
 	return bkts
 }
 
-func (bs *BucketSet) sortedBucketNames() []string {
-	bs.mu.RLock()
-	defer bs.mu.RUnlock()
-
-	return bs.buckets.List()
-}
-
 // Owner returns the owner of the key.
 // Owner will cache the results for faster lookup.
 func (bs *BucketSet) Owner(key string) string {
@@ -116,12 +109,12 @@ func (bs *BucketSet) HasBucket(bkt string) bool {
 	return bs.buckets.Has(bkt)
 }
 
-// BucketList returns the bucket names of this BucketSet in random order.
+// BucketList returns the bucket names of this BucketSet in sorted order.
 func (bs *BucketSet) BucketList() []string {
 	bs.mu.RLock()
 	defer bs.mu.RUnlock()
 
-	return bs.buckets.UnsortedList()
+	return bs.buckets.List()
 }
 
 // Update updates the universe of buckets.


Test patch:
diff --git a/hash/bucketer_test.go b/hash/bucketer_test.go
index 8181175f2..8b42d6cd7 100644
--- a/hash/bucketer_test.go
+++ b/hash/bucketer_test.go
@@ -18,7 +18,6 @@ package hash
 
 import (
 	"reflect"
-	"sort"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -69,7 +68,6 @@ func TestBucketSetList(t *testing.T) {
 	bs := NewBucketSet(buckets)
 
 	got := bs.BucketList()
-	sort.Strings(got)
 	if want := buckets.List(); !reflect.DeepEqual(got, want) {
 		t.Errorf("Name = %q, want: %q, diff(-want,+got):\n%s", got, want, cmp.Diff(want, got))
 	}
diff --git a/leaderelection/context_test.go b/leaderelection/context_test.go
index b2011b83b..bf6dbb487 100644
--- a/leaderelection/context_test.go
+++ b/leaderelection/context_test.go
@@ -23,7 +23,6 @@ import (
 	"context"
 	"fmt"
 	"os"
-	"sort"
 	"testing"
 	"time"
 
@@ -252,7 +251,6 @@ func TestNewStatefulSetBucketAndSet(t *testing.T) {
 	}
 
 	gotNames := bs.BucketList()
-	sort.Strings(gotNames)
 	if !cmp.Equal(gotNames, wantNames) {
 		t.Errorf("BucketSet.BucketList() = %q, want: %q", gotNames, wantNames)
 	}
diff --git a/test/helpers/name.go b/test/helpers/name.go
index ef5ac4dbd..0cd25785b 100644
--- a/test/helpers/name.go
+++ b/test/helpers/name.go
@@ -23,7 +23,6 @@ import (
 	"unicode"
 
 	"knative.dev/pkg/kmeta"
-	"knative.dev/pkg/test"
 )
 
 const (
@@ -42,13 +41,17 @@ func init() {
 	rand.Seed(seed)
 }
 
+type named interface {
+	Name() string
+}
+
 // ObjectPrefixForTest returns the name prefix for this test's random names.
-func ObjectPrefixForTest(t test.T) string {
+func ObjectPrefixForTest(t named) string {
 	return MakeK8sNamePrefix(strings.TrimPrefix(t.Name(), testNamePrefix))
 }
 
 // ObjectNameForTest generates a random object name based on the test name.
-func ObjectNameForTest(t test.T) string {
+func ObjectNameForTest(t named) string {
 	return kmeta.ChildName(ObjectPrefixForTest(t), string(sep)+RandomString())
 }