Repository: knative/pkg
Pull request #1861 (2020-10-28 00:05:06)

Add check variants that return errors for more extensive testing.

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1861#" title="Author self-approved">mattmoor</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

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

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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/1861?src=pr&el=h1) Report
> Merging [#1861](https://codecov.io/gh/knative/pkg/pull/1861?src=pr&el=desc) into [master](https://codecov.io/gh/knative/pkg/commit/e39bfc4544a86ccaa8f6682c7af1fe130600bd82?el=desc) will **increase** coverage by `0.15%`.
> The diff coverage is `66.66%`.

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

```diff
@@            Coverage Diff             @@
##           master    #1861      +/-   ##
==========================================
+ Coverage   68.09%   68.25%   +0.15%     
==========================================
  Files         223      223              
  Lines        9467     9479      +12     
==========================================
+ Hits         6447     6470      +23     
+ Misses       2694     2682      -12     
- Partials      326      327       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/1861?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [depcheck/depcheck.go](https://codecov.io/gh/knative/pkg/pull/1861/diff?src=pr&el=tree#diff-ZGVwY2hlY2svZGVwY2hlY2suZ28=) | `82.75% <66.66%> (+32.75%)` | :arrow_up: |
| [test/gcs/mock/mock.go](https://codecov.io/gh/knative/pkg/pull/1861/diff?src=pr&el=tree#diff-dGVzdC9nY3MvbW9jay9tb2NrLmdv) | `90.32% <0.00%> (-1.08%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/1861?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/1861?src=pr&el=footer). Last update [e39bfc4...5719154](https://codecov.io/gh/knative/pkg/pull/1861?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/depcheck/depcheck.go b/depcheck/depcheck.go
index 08d0bff48..aaa88762f 100644
--- a/depcheck/depcheck.go
+++ b/depcheck/depcheck.go
@@ -39,6 +39,15 @@ func (g graph) contains(name string) bool {
 	return ok
 }
 
+func (g graph) order() []string {
+	order := make(sort.StringSlice, 0, len(g))
+	for k := range g {
+		order = append(order, k)
+	}
+	order.Sort()
+	return order
+}
+
 // path constructs an examplary path that looks something like:
 //    knative.dev/pkg/apis/duck
 //    knative.dev/pkg/apis  # Also: [knative.dev/pkg/kmeta knative.dev/pkg/tracker]
@@ -90,26 +99,57 @@ func buildGraph(importpaths ...string) (graph, error) {
 	return g, nil
 }
 
+// CheckNoDependency checks that the given import paths (ip) does not
+// depend (transitively) on certain banned imports.
+func CheckNoDependency(t *testing.T, ip string, banned []string) error {
+	t.Helper()
+	g, err := buildGraph(ip)
+	if err != nil {
+		return fmt.Errorf("buildGraph(queue) = %v", err)
+	}
+	for _, dip := range banned {
+		if g.contains(dip) {
+			return fmt.Errorf("%s depends on banned dependency %s\n%s", ip, dip,
+				strings.Join(g.path(dip), "\n"))
+		}
+	}
+	return nil
+}
+
 // AssertNoDependency checks that the given import paths (the keys) do not
 // depend (transitively) on certain banned imports (the values)
 func AssertNoDependency(t *testing.T, banned map[string][]string) {
 	t.Helper()
 	for ip, banned := range banned {
 		t.Run(ip, func(t *testing.T) {
-			g, err := buildGraph(ip)
-			if err != nil {
-				t.Fatal("buildGraph(queue) =", err)
-			}
-			for _, dip := range banned {
-				if g.contains(dip) {
-					t.Errorf("%s depends on banned dependency %s\n%s", ip, dip,
-						strings.Join(g.path(dip), "\n"))
-				}
+			if err := CheckNoDependency(t, ip, banned); err != nil {
+				t.Error("CheckNoDependency() =", err)
 			}
 		})
 	}
 }
 
+// CheckOnlyDependencies checks that the given import path only
+// depends (transitively) on certain allowed imports.
+// Note: while perhaps counterintuitive we allow the value to be a superset
+// of the actual imports to that folks can use a constant that holds blessed
+// import paths.
+func CheckOnlyDependencies(t *testing.T, ip string, allowed map[string]struct{}) error {
+	t.Helper()
+
+	g, err := buildGraph(ip)
+	if err != nil {
+		return fmt.Errorf("buildGraph(queue) = %v", err)
+	}
+	for _, name := range g.order() {
+		if _, ok := allowed[name]; !ok {
+			return fmt.Errorf("Dependency %s of %s is not explicitly allowed\n%s", name, ip,
+				strings.Join(g.path(name), "\n"))
+		}
+	}
+	return nil
+}
+
 // AssertOnlyDependencies checks that the given import paths (the keys) only
 // depend (transitively) on certain allowed imports (the values).
 // Note: while perhaps counterintuitive we allow the value to be a superset
@@ -117,7 +157,6 @@ func AssertNoDependency(t *testing.T, banned map[string][]string) {
 // import paths.
 func AssertOnlyDependencies(t *testing.T, allowed map[string][]string) {
 	t.Helper()
-
 	for ip, allow := range allowed {
 		// Always include our own package in the set of allowed dependencies.
 		allowed := make(map[string]struct{}, len(allow)+1)
@@ -125,15 +164,8 @@ func AssertOnlyDependencies(t *testing.T, allowed map[string][]string) {
 			allowed[x] = struct{}{}
 		}
 		t.Run(ip, func(t *testing.T) {
-			g, err := buildGraph(ip)
-			if err != nil {
-				t.Fatal("buildGraph(queue) =", err)
-			}
-			for name := range g {
-				if _, ok := allowed[name]; !ok {
-					t.Errorf("Dependency %s of %s is not explicitly allowed\n%s", name, ip,
-						strings.Join(g.path(name), "\n"))
-				}
+			if err := CheckOnlyDependencies(t, ip, allowed); err != nil {
+				t.Error("CheckOnlyDependencies() =", err)
 			}
 		})
 	}


Test patch:
diff --git a/depcheck/depcheck_test.go b/depcheck/depcheck_test.go
index cfc0bdbbf..5954f1166 100644
--- a/depcheck/depcheck_test.go
+++ b/depcheck/depcheck_test.go
@@ -20,6 +20,7 @@ limitations under the License.
 package depcheck_test
 
 import (
+	"strings"
 	"testing"
 
 	"knative.dev/pkg/depcheck"
@@ -44,13 +45,17 @@ func TestExample(t *testing.T) {
 		"knative.dev/pkg/depcheck": {
 			"k8s.io/apimachinery/pkg/util/sets",
 		},
-
-		// BAD Example used to illustrate path error message.
-		// "knative.dev/pkg/apis/duck": {
-		// 	"k8s.io/api/core/v1",
-		// },
 	})
 
+	// Sample failure case, duck clearly relies on corev1 for all assortment of things.
+	if err := depcheck.CheckNoDependency(t, "knative.dev/pkg/apis/duck", []string{"k8s.io/api/core/v1"}); err == nil {
+		t.Error("CheckNoDependency() = nil, wanted error")
+	} else if !strings.Contains(err.Error(), "knative.dev/pkg/tracker") {
+		t.Errorf("CheckNoDependency() = %v, expected to contain: %v", err, "knative.dev/pkg/tracker")
+	} else {
+		t.Log("CheckNoDependency() =", err)
+	}
+
 	// For small packages, it can make the most sense to curate
 	// the external dependencies very carefully.
 	depcheck.AssertOnlyDependencies(t, map[string][]string{
@@ -64,4 +69,17 @@ func TestExample(t *testing.T) {
 			"time",
 		},
 	})
+
+	// Sample failure case, doesn't include transitive dependencies!
+	if err := depcheck.CheckOnlyDependencies(t, "knative.dev/pkg/depcheck", map[string]struct{}{
+		"fmt":                            {},
+		"sort":                           {},
+		"strings":                        {},
+		"testing":                        {},
+		"golang.org/x/tools/go/packages": {},
+	}); err == nil {
+		t.Error("CheckOnlyDependencies() = nil, wanted error")
+	} else if !strings.Contains(err.Error(), "golang.org/x/tools/go/gcexportdata") {
+		t.Errorf("CheckOnlyDependencies() = %v, expected to contain: %v", err, "golang.org/x/tools/go/gcexportdata")
+	}
 }