From d77d3b4bf20eb906ae67132b034b88edfdbc1626 Mon Sep 17 00:00:00 2001
From: Ian Wahbe <ian@wahbe.com>
Date: Wed, 26 Oct 2022 16:51:52 -0700
Subject: [PATCH] Add comment + fix tests

We had two problems here.

1. Our graph rapid tests didn't encode the `includeChildren` flag as
transitive, which it should be. Looking at the call sites of
`DependingOn` confirms this.

2. `TestDeleteProtected` was sharing variables between threads (causing
flakey results). This was fixed.
---
 pkg/resource/edit/operations_test.go              | 12 +++++-------
 pkg/resource/graph/dependency_graph.go            |  2 ++
 pkg/resource/graph/dependency_graph_rapid_test.go |  6 ++----
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/pkg/resource/edit/operations_test.go b/pkg/resource/edit/operations_test.go
index 64e8889d0f86..fc67bc94809e 100644
--- a/pkg/resource/edit/operations_test.go
+++ b/pkg/resource/edit/operations_test.go
@@ -186,8 +186,6 @@ func TestFailedDeletionProtected(t *testing.T) {
 func TestDeleteProtected(t *testing.T) {
 	t.Parallel()
 
-	var pA, a, b, c *resource.State
-	var snap *deploy.Snapshot
 	tests := []struct {
 		name string
 		test func(t *testing.T, pA, a, b, c *resource.State, snap *deploy.Snapshot)
@@ -268,11 +266,11 @@ func TestDeleteProtected(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			pA = NewProviderResource("a", "p1", "0")
-			a = NewResource("a", pA)
-			b = NewResource("b", pA)
-			c = NewResource("c", pA, b.URN)
-			snap = NewSnapshot([]*resource.State{
+			pA := NewProviderResource("a", "p1", "0")
+			a := NewResource("a", pA)
+			b := NewResource("b", pA)
+			c := NewResource("c", pA, b.URN)
+			snap := NewSnapshot([]*resource.State{
 				pA,
 				a,
 				b,
diff --git a/pkg/resource/graph/dependency_graph.go b/pkg/resource/graph/dependency_graph.go
index de0d71ed4be9..4cf0f03656e1 100644
--- a/pkg/resource/graph/dependency_graph.go
+++ b/pkg/resource/graph/dependency_graph.go
@@ -20,6 +20,8 @@ type DependencyGraph struct {
 // order with respect to the snapshot dependency graph.
 //
 // The time complexity of DependingOn is linear with respect to the number of resources.
+//
+// includeChildren adds children as another type of (transitive) dependency.
 func (dg *DependencyGraph) DependingOn(res *resource.State,
 	ignore map[resource.URN]bool, includeChildren bool) []*resource.State {
 	// This implementation relies on the detail that snapshots are stored in a valid
diff --git a/pkg/resource/graph/dependency_graph_rapid_test.go b/pkg/resource/graph/dependency_graph_rapid_test.go
index 85b0e9e39618..80708ff726a3 100644
--- a/pkg/resource/graph/dependency_graph_rapid_test.go
+++ b/pkg/resource/graph/dependency_graph_rapid_test.go
@@ -127,10 +127,8 @@ func expectedDependingOn(universe []*resource.State, includeChildren bool) R {
 		return inverse(transitively(universe)(restrictedDependenciesOf))
 	}
 
-	// TODO this extends base `expectedDependingOn` with
-	// immediate children. Should it be a transitive closure?
 	dependingOn := expectedDependingOn(universe, false)
-	return func(a, b *resource.State) bool {
+	return transitively(universe)(func(a, b *resource.State) bool {
 		if dependingOn(a, b) || isParent(b, a) {
 			return true
 		}
@@ -140,7 +138,7 @@ func expectedDependingOn(universe []*resource.State, includeChildren bool) R {
 			}
 		}
 		return false
-	}
+	})
 }
 
 // Verify `DependingOn` against `expectedDependingOn`. Note that