chore: Adds mig tests and refactor - searchindex #2065
Conversation
| }, | ||
| }, | ||
| }) | ||
| } | ||
| func commonChecks(indexName, indexType, mappingsDynamic, databaseName string, clusterInfo acc.ClusterInfo) []resource.TestCheckFunc { |
There was a problem hiding this comment.
NOTE: adds AddAttrChecks and AddAttrSetChecks to acc package
These functions are meant to simplify the reusing of checks.
This function is used by almost all of the tests to simplify the checking.
commonChecks defines:
attributesused byAddAttrChecksthe common fields to be checked on datasource and resource for all tests (checking both field and value)AddAttrSetChecksonly checks that the field is defined- any differences between
resourceNameanddatasourceName, e.g.,index_idis also checked on thedatasourceName
In this case, I add two more helpers to simplify adding checks to both datasource and resource: addAttrChecks and addAttrSetChecks
| "github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/mig" | ||
| ) | ||
|
|
||
| func TestMigSearchIndexRS_basic(t *testing.T) { |
There was a problem hiding this comment.
remove RS from test names if we're testing resource and datasource together
| func AddAttrSetChecks(targetName string, checks []resource.TestCheckFunc, attrNames ...string) []resource.TestCheckFunc { | ||
| // avoids accidentally modifying existing slice | ||
| newChecks := []resource.TestCheckFunc{} | ||
| copy(newChecks, checks) |
There was a problem hiding this comment.
not sure this copy is doing anything as destination is empty, e.g: https://yourbasic.org/golang/copy-explained/
" It returns the number of elements copied, which will be the minimum of len(dst) and len(src) "
If destination is empty nothing is copied. you should need:
newChecks := make(resource.TestCheckFunc, len(checks))
There was a problem hiding this comment.
Great. Thank you for the reference as well 🙏
There was a problem hiding this comment.
Note: I had to add the capacity argument too, otherwise ci-lint complains about append to slice newChecks with non-zero initialized length (makezero)
|
LGTM, just minor comments |
| @@ -54,3 +54,23 @@ func JSONEquals(expected string) resource.CheckResourceAttrWithFunc { | |||
| return nil | |||
| } | |||
| } | |||
|
|
|||
| func AddAttrSetChecks(targetName string, checks []resource.TestCheckFunc, attrNames ...string) []resource.TestCheckFunc { | |||
There was a problem hiding this comment.
Name was a little confusing to me at first (thought Set referred to the collection type), does AddAttrIsDefinedChecks sound better?
There was a problem hiding this comment.
Hmm. It comes from the underlying resource.TestCheckResourceAttrSet from the terraform-plugin-testing.
I thought it would be smart to keep the names the same. The relevant calls are:
acc.AddAttrSetChecks(datasourceName, checks, "project_id", "index_id")acc.AddAttrSetChecks(resourceName, checks, "project_id")
WDYT?
There was a problem hiding this comment.
makes sense, agree with that approach 👍
| return acc.AddAttrChecks(datasourceName, checks, mapChecks) | ||
| } | ||
|
|
||
| func addAttrSetChecks(checks []resource.TestCheckFunc, attrNames ...string) []resource.TestCheckFunc { |
There was a problem hiding this comment.
relevant to comment made below regarding naming
Description
Adds mig tests and refactor - searchindex
See also comment below
Link to any related issue(s): CLOUDP-237030
Type of change:
Required Checklist:
Further comments