Skip to content

Commit

Permalink
Add comment + fix tests
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
iwahbe committed Oct 26, 2022
1 parent ab39b0d commit d77d3b4
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 11 deletions.
12 changes: 5 additions & 7 deletions pkg/resource/edit/operations_test.go
Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions pkg/resource/graph/dependency_graph.go
Expand Up @@ -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
Expand Down
6 changes: 2 additions & 4 deletions pkg/resource/graph/dependency_graph_rapid_test.go
Expand Up @@ -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
}
Expand All @@ -140,7 +138,7 @@ func expectedDependingOn(universe []*resource.State, includeChildren bool) R {
}
}
return false
}
})
}

// Verify `DependingOn` against `expectedDependingOn`. Note that
Expand Down

0 comments on commit d77d3b4

Please sign in to comment.