Skip to content

Commit

Permalink
add unit tests, fix some issues
Browse files Browse the repository at this point in the history
  • Loading branch information
@VenelinMartinov
VenelinMartinov committed Dec 18, 2024
1 parent 18b303a commit d078619
Show file tree
Hide file tree
Showing 5 changed files with 628 additions and 57 deletions.
77 changes: 44 additions & 33 deletions pkg/tfbridge/detailed_diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ func (differ detailedDiffer) makePropDiff(
case shim.TypeList:
return differ.makeListDiff(path, old, new)
case shim.TypeSet:
return differ.makeSetDiffAlt(path, old, new)
return differ.makeSetDiff(path, old, new)
case shim.TypeMap:
// Note that TF objects are represented as maps when returned by LookupSchemas
return differ.makeMapDiff(path, old, new)
Expand Down Expand Up @@ -395,8 +395,8 @@ func (differ detailedDiffer) matchPlanElementsToInputs(
newInputsList = newInputs.ArrayValue()
}

if len(newInputsList) != len(plannedState) {
// If the number of inputs doesn't match the number of planned state elements,
if len(newInputsList) < len(plannedState) {
// If the number of inputs is less than the number of planned state elements,
// we can't match the elements to the inputs.
return nil
}
Expand Down Expand Up @@ -426,47 +426,25 @@ func (differ detailedDiffer) matchPlanElementsToInputs(
return matched
}

func (differ detailedDiffer) makeSetDiffAlt(
path propertyPath, old, new resource.PropertyValue,
func makeSetDiffResult(
path propertyPath,
removed, newInputAdded []arrayIndex,
isSetForceNew bool,
) map[detailedDiffKey]*pulumirpc.PropertyDiff {
// We need to build a map of what changed for each index in order to present the
// correct diff to the engine since it always uses new inputs and old state
// to display previews.
diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff)
oldList := old.ArrayValue()
newList := new.ArrayValue()

oldIdentities := differ.calculateSetHashIndexMap(path, oldList)
newIdentities := differ.calculateSetHashIndexMap(path, newList)

removed, added := computeSetHashChanges(oldIdentities, newIdentities)

isSetForceNew := differ.isForceNew(path)

// We need to match the new indices to the inputs to ensure that the identity of the
// elements is preserved - this is necessary since the planning process can reorder
// the elements.
matchedIndices := differ.matchPlanElementsToInputs(path, added, newList)
if matchedIndices == nil || len(matchedIndices) != len(added) {
// If we can't match the elements to the inputs, we will return a diff against the whole set.
if isSetForceNew {
diff[path.Key()] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE}
} else {
diff[path.Key()] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE}
}
return diff
}

type setChange struct {
oldChanged bool
newChanged bool
}

// We need to build a map of what changed for each index in order to present the
// correct diff to the engine since it always uses new inputs and old state
// to display previews.
changes := map[arrayIndex]setChange{}
for _, index := range removed {
changes[index] = setChange{oldChanged: true, newChanged: false}
}
for _, index := range added {
for _, index := range newInputAdded {
if _, ok := changes[index]; !ok {
changes[index] = setChange{oldChanged: false, newChanged: true}
} else {
Expand Down Expand Up @@ -501,6 +479,39 @@ func (differ detailedDiffer) makeSetDiffAlt(
return diff
}

func (differ detailedDiffer) makeSetDiff(
path propertyPath, old, new resource.PropertyValue,
) map[detailedDiffKey]*pulumirpc.PropertyDiff {
diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff)
oldList := old.ArrayValue()
newList := new.ArrayValue()

oldIdentities := differ.calculateSetHashIndexMap(path, oldList)
newIdentities := differ.calculateSetHashIndexMap(path, newList)

removed, added := computeSetHashChanges(oldIdentities, newIdentities)

isSetForceNew := differ.isForceNew(path)

// We need to match the new indices to the inputs to ensure that the identity of the
// elements is preserved - this is necessary since the planning process can reorder
// the elements.
matchedIndices := differ.matchPlanElementsToInputs(path, added, newList)
if matchedIndices == nil || len(matchedIndices) != len(added) {
// If we can't match the elements to the inputs, we will return a diff against the whole set.
if isSetForceNew {
diff[path.Key()] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE}
} else {
diff[path.Key()] = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE}
}
return diff
}

diff = makeSetDiffResult(path, removed, matchedIndices, isSetForceNew)

return diff
}

func (differ detailedDiffer) makeMapDiff(
path propertyPath, old, new resource.PropertyValue,
) map[detailedDiffKey]*pulumirpc.PropertyDiff {
Expand Down
233 changes: 226 additions & 7 deletions pkg/tfbridge/detailed_diff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"fmt"
"reflect"
"testing"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand Down Expand Up @@ -2214,7 +2215,7 @@ func TestDetailedDiffSetBlock(t *testing.T) {
runDetailedDiffTest(t,
propertyMapElems("val1"),
propertyMapElems("val2"), tfs, ps, map[string]*pulumirpc.PropertyDiff{
"foo[0].bar": {Kind: update},
"foo[0]": {Kind: update},
},
)
})
Expand Down Expand Up @@ -2296,7 +2297,7 @@ func TestDetailedDiffSetBlock(t *testing.T) {
propertyMapElems("val1", "val2", "val3"),
propertyMapElems("val1", "val4", "val3"), tfs, ps,
map[string]*pulumirpc.PropertyDiff{
"foo[1].bar": {Kind: update},
"foo[1]": {Kind: update},
},
)
})
Expand Down Expand Up @@ -2616,18 +2617,18 @@ func TestDetailedDiffMismatchedSchemas(t *testing.T) {

func TestDetailedDiffSetHashChanges(t *testing.T) {
t.Parallel()
runTest := func(old, new hashIndexMap, expectedRemoved, expectedAdded hashIndexMap) {
runTest := func(old, new hashIndexMap, expectedRemoved, expectedAdded []arrayIndex) {
t.Helper()
removed, added := computeSetHashChanges(old, new)

require.Equal(t, removed, expectedRemoved)
require.Equal(t, added, expectedAdded)
}

runTest(hashIndexMap{}, hashIndexMap{}, hashIndexMap{}, hashIndexMap{})
runTest(hashIndexMap{1: 1}, hashIndexMap{1: 1}, hashIndexMap{}, hashIndexMap{})
runTest(hashIndexMap{1: 1}, hashIndexMap{}, hashIndexMap{1: 1}, hashIndexMap{})
runTest(hashIndexMap{1: 1}, hashIndexMap{2: 2}, hashIndexMap{1: 1}, hashIndexMap{2: 2})
runTest(hashIndexMap{}, hashIndexMap{}, []arrayIndex{}, []arrayIndex{})
runTest(hashIndexMap{1: 1}, hashIndexMap{1: 1}, []arrayIndex{}, []arrayIndex{})
runTest(hashIndexMap{1: 1}, hashIndexMap{}, []arrayIndex{1}, []arrayIndex{})
runTest(hashIndexMap{1: 1}, hashIndexMap{2: 2}, []arrayIndex{1}, []arrayIndex{2})
}

func TestDetailedDiffSetHashPanicCaught(t *testing.T) {
Expand Down Expand Up @@ -2754,3 +2755,221 @@ func TestContainsReplace(t *testing.T) {

require.False(t, containsReplace(map[string]*pulumirpc.PropertyDiff{}))
}

func TestMakeSetDiffResult(t *testing.T) {
t.Parallel()

tests := []struct {
name string
path propertyPath
removed []arrayIndex
added []arrayIndex
isForceNew bool
expectedDiff map[detailedDiffKey]*pulumirpc.PropertyDiff
}{
{
name: "empty changes",
path: newPropertyPath("test"),
removed: []arrayIndex{},
added: []arrayIndex{},
isForceNew: false,
expectedDiff: map[detailedDiffKey]*pulumirpc.PropertyDiff{},
},
{
name: "single addition",
path: newPropertyPath("test"),
removed: []arrayIndex{},
added: []arrayIndex{0},
isForceNew: false,
expectedDiff: map[detailedDiffKey]*pulumirpc.PropertyDiff{
"test[0]": {Kind: pulumirpc.PropertyDiff_ADD},
},
},
{
name: "single deletion",
path: newPropertyPath("test"),
removed: []arrayIndex{0},
added: []arrayIndex{},
isForceNew: false,
expectedDiff: map[detailedDiffKey]*pulumirpc.PropertyDiff{
"test[0]": {Kind: pulumirpc.PropertyDiff_DELETE},
},
},
{
name: "single update (same index)",
path: newPropertyPath("test"),
removed: []arrayIndex{0},
added: []arrayIndex{0},
isForceNew: false,
expectedDiff: map[detailedDiffKey]*pulumirpc.PropertyDiff{
"test[0]": {Kind: pulumirpc.PropertyDiff_UPDATE},
},
},
{
name: "multiple changes",
path: newPropertyPath("test"),
removed: []arrayIndex{0, 2},
added: []arrayIndex{1, 3},
isForceNew: false,
expectedDiff: map[detailedDiffKey]*pulumirpc.PropertyDiff{
"test[0]": {Kind: pulumirpc.PropertyDiff_DELETE},
"test[1]": {Kind: pulumirpc.PropertyDiff_ADD},
"test[2]": {Kind: pulumirpc.PropertyDiff_DELETE},
"test[3]": {Kind: pulumirpc.PropertyDiff_ADD},
},
},
{
name: "force new - single addition",
path: newPropertyPath("test"),
removed: []arrayIndex{},
added: []arrayIndex{0},
isForceNew: true,
expectedDiff: map[detailedDiffKey]*pulumirpc.PropertyDiff{
"test[0]": {Kind: pulumirpc.PropertyDiff_ADD_REPLACE},
},
},
{
name: "force new - single deletion",
path: newPropertyPath("test"),
removed: []arrayIndex{0},
added: []arrayIndex{},
isForceNew: true,
expectedDiff: map[detailedDiffKey]*pulumirpc.PropertyDiff{
"test[0]": {Kind: pulumirpc.PropertyDiff_DELETE_REPLACE},
},
},
{
name: "force new - single update",
path: newPropertyPath("test"),
removed: []arrayIndex{0},
added: []arrayIndex{0},
isForceNew: true,
expectedDiff: map[detailedDiffKey]*pulumirpc.PropertyDiff{
"test[0]": {Kind: pulumirpc.PropertyDiff_UPDATE_REPLACE},
},
},
{
name: "nested path",
path: newPropertyPath("parent").Subpath("child"),
removed: []arrayIndex{0},
added: []arrayIndex{1},
isForceNew: false,
expectedDiff: map[detailedDiffKey]*pulumirpc.PropertyDiff{
"parent.child[0]": {Kind: pulumirpc.PropertyDiff_DELETE},
"parent.child[1]": {Kind: pulumirpc.PropertyDiff_ADD},
},
},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := makeSetDiffResult(tt.path, tt.removed, tt.added, tt.isForceNew)
require.Equal(t, tt.expectedDiff, result)
})
}
}

func TestMatchPlanElementsToInputs(t *testing.T) {
t.Parallel()
tfs := shimv2.NewSchemaMap(map[string]*schema.Schema{
"my_list": {
Type: schema.TypeList,
Optional: true,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
})

ps := map[string]*SchemaInfo{}
tests := []struct {
name string
path propertyPath
changedIndices []arrayIndex
plannedState []resource.PropertyValue
newInputs resource.PropertyMap
expectedMatches []arrayIndex
}{
{
name: "basic matching",
path: newPropertyPath("myList"),
changedIndices: []arrayIndex{0, 1},
plannedState: []resource.PropertyValue{
resource.NewStringProperty("foo"),
resource.NewStringProperty("bar"),
},
newInputs: resource.PropertyMap{
"myList": resource.NewArrayProperty([]resource.PropertyValue{
resource.NewStringProperty("foo"),
resource.NewStringProperty("bar"),
}),
},
expectedMatches: []arrayIndex{0, 1},
},
{
name: "length mismatch returns nil",
path: newPropertyPath("myList"),
changedIndices: []arrayIndex{0},
plannedState: []resource.PropertyValue{
resource.NewStringProperty("foo"),
resource.NewStringProperty("bar"),
},
newInputs: resource.PropertyMap{
"myList": resource.NewArrayProperty([]resource.PropertyValue{
resource.NewStringProperty("foo"),
}),
},
expectedMatches: nil,
},
{
name: "no matches returns empty slice",
path: newPropertyPath("myList"),
changedIndices: []arrayIndex{0},
plannedState: []resource.PropertyValue{
resource.NewStringProperty("foo"),
resource.NewStringProperty("bar"),
},
newInputs: resource.PropertyMap{
"myList": resource.NewArrayProperty([]resource.PropertyValue{
resource.NewStringProperty("baz"),
resource.NewStringProperty("qux"),
}),
},
expectedMatches: []arrayIndex{},
},
{
name: "missing input path returns empty slice",
path: newPropertyPath("nonexistentList"),
changedIndices: []arrayIndex{0},
plannedState: []resource.PropertyValue{
resource.NewStringProperty("foo"),
},
newInputs: resource.PropertyMap{},
expectedMatches: nil,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
differ := detailedDiffer{
ctx: context.Background(),
tfs: tfs,
ps: ps,
newInputs: tt.newInputs,
}

matches := differ.matchPlanElementsToInputs(tt.path, tt.changedIndices, tt.plannedState)

if tt.expectedMatches == nil && matches != nil {
t.Errorf("expected nil matches, got %v", matches)
return
}

if !reflect.DeepEqual(matches, tt.expectedMatches) {
t.Errorf("expected matches %v, got %v", tt.expectedMatches, matches)
}
})
}
}
Loading

0 comments on commit d078619

Please sign in to comment.