Add long-running test cases & implement slice merge for ReplicationSpecs #138
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package compat | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "reflect" | ||
| ) | ||
|
|
||
| // JSONSliceMerge will merge two slices using JSONCopy according to these rules: | ||
| // | ||
| // 1. If `dst` and `src` are the same length, all elements are merged | ||
| // | ||
| // 2. If `dst` is longer, only the first `len(src)` elements are merged | ||
| // | ||
| // 3. If `src` is longer, first `len(dst)` elements are merged, then remaining elements are appended to `dst` | ||
| func JSONSliceMerge(dst, src interface{}) error { | ||
| dstVal := reflect.ValueOf(dst) | ||
| srcVal := reflect.ValueOf(src) | ||
|
|
||
| if dstVal.Kind() != reflect.Ptr { | ||
| return errors.New("dst must be a pointer to slice") | ||
| } | ||
|
|
||
| dstVal = reflect.Indirect(dstVal) | ||
| srcVal = reflect.Indirect(srcVal) | ||
|
|
||
| if dstVal.Kind() != reflect.Slice { | ||
| return errors.New("dst must be pointing to a slice") | ||
| } | ||
|
|
||
| if srcVal.Kind() != reflect.Slice { | ||
| return errors.New("src must be a slice or a pointer to slice") | ||
| } | ||
|
|
||
| minLen := dstVal.Len() | ||
| if srcVal.Len() < minLen { | ||
| minLen = srcVal.Len() | ||
| } | ||
|
|
||
| // merge common elements | ||
| for i := 0; i < minLen; i++ { | ||
| dstX := dstVal.Index(i).Addr().Interface() | ||
|
Contributor
There was a problem hiding this comment. [q] am I right that this will work only if the items of the slice are pointers as well? (looking at the tests below seems only the destination slice must have pointers?)
Contributor
Author
There was a problem hiding this comment. They don't have to be pointers - I used pointers in the test case just to check that pointers work too |
||
| if err := JSONCopy(dstX, srcVal.Index(i).Interface()); err != nil { | ||
| return fmt.Errorf("cannot copy value at index %d: %w", i, err) | ||
| } | ||
| } | ||
|
|
||
| // append extra elements (if any) | ||
| dstType := reflect.TypeOf(dst).Elem().Elem() | ||
| for i := minLen; i < srcVal.Len(); i++ { | ||
| newVal := reflect.New(dstType).Interface() | ||
| if err := JSONCopy(&newVal, srcVal.Index(i).Interface()); err != nil { | ||
| return fmt.Errorf("cannot copy value at index %d: %w", i, err) | ||
| } | ||
| dstVal.Set(reflect.Append(dstVal, reflect.ValueOf(newVal).Elem())) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| package compat_test | ||
|
|
||
| import ( | ||
| "errors" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| . "github.com/mongodb/mongodb-atlas-kubernetes/pkg/util/compat" | ||
| ) | ||
|
|
||
| func TestJSONSliceMerge(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Great to have the unit test!
|
||
| type Item struct { | ||
| ID string `json:"id,omitempty"` | ||
| Name string `json:"name,omitempty"` | ||
| } | ||
|
|
||
| type OtherItem struct { | ||
| OtherID string `json:"id,omitempty"` | ||
| OtherName string `json:"name,omitempty"` | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| dst, src, expected interface{} | ||
| expectedError error | ||
| }{ | ||
| { | ||
| name: "src is longer", | ||
| dst: &[]*Item{ | ||
| {"00001", "dst1"}, | ||
| {"00002", "dst2"}, | ||
| {"00003", "dst3"}, | ||
| }, | ||
| src: []OtherItem{ // copying from different element type | ||
| {"99999", "src1"}, // different key, different value | ||
| {"", "src2"}, // no key, different value | ||
| {"", ""}, // no key, no value | ||
| {"12345", "extra"}, // extra value | ||
| }, | ||
| expected: &[]*Item{ // kept dst element type | ||
| {"99999", "src1"}, // key & value replaced by src | ||
| {"00002", "src2"}, // only value replaced by src | ||
| {"00003", "dst3"}, // untouched | ||
| {"12345", "extra"}, // appended from src | ||
| }, | ||
| }, | ||
| { | ||
| name: "dst is longer", | ||
| dst: &[]*Item{ | ||
| {"00001", "dst1"}, | ||
| {"00002", "dst2"}, | ||
| {"00003", "dst3"}, | ||
| }, | ||
| src: []OtherItem{ | ||
| {"99999", "src1"}, | ||
| }, | ||
| expected: &[]*Item{ | ||
| {"99999", "src1"}, // key & value replaced by src | ||
| {"00002", "dst2"}, // untouched | ||
| {"00003", "dst3"}, // untouched | ||
| }, | ||
| }, | ||
| { | ||
| name: "src is nil", | ||
| dst: &[]*Item{ | ||
| {"00001", "dst1"}, | ||
| {"00002", "dst2"}, | ||
| {"00003", "dst3"}, | ||
| }, | ||
| src: nil, | ||
| expectedError: errors.New("src must be a slice or a pointer to slice"), | ||
| expected: &[]*Item{ | ||
| {"00001", "dst1"}, // untouched | ||
| {"00002", "dst2"}, // untouched | ||
| {"00003", "dst3"}, // untouched | ||
| }, | ||
| }, | ||
| { | ||
| name: "dst is nil", | ||
| dst: nil, | ||
| expectedError: errors.New("dst must be a pointer to slice"), | ||
| src: []OtherItem{ | ||
| {"99999", "src1"}, | ||
| }, | ||
| expected: nil, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| require := require.New(t) | ||
| err := JSONSliceMerge(tt.dst, tt.src) | ||
| require.Equal(tt.expectedError, err) | ||
| require.Equal(tt.expected, tt.dst) | ||
| }) | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] can we add some comment about why we need to clean these fields?