-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Modifies stream_connection and stream_instance tests to use shared cluster and project #2096
Conversation
resource "mongodbatlas_project" "test" { | ||
org_id = %[1]q | ||
name = %[2]q | ||
} | ||
|
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.
projects are not longer present as part of the terraform config
func clusterStreamConnectionConfig(projectIDStr, instanceName, clusterNameStr, clusterTerraformStr string) string { | ||
return clusterTerraformStr + fmt.Sprintf(` | ||
|
||
func clusterStreamConnectionConfig(projectID, instanceName, clusterName string) string { | ||
return fmt.Sprintf(` |
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.
cluster definition is no longer part of the terraform config
internal/service/streaminstance/data_source_stream_instances_test.go
Outdated
Show resolved
Hide resolved
Config: streamInstancesDataSourceConfig(orgID, projectName, instanceName, region, cloudProvider), | ||
Check: streamInstancesAttributeChecks(dataSourceName, nil, nil, 1), | ||
Config: streamInstancesDataSourceConfig(projectID, instanceName, region, cloudProvider), | ||
Check: streamInstancesAttributeChecks(dataSourceName, nil, nil, acc.IntGreatThan(0)), // at least on instance is present in the project |
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.
can we change this and check something that uniquely identifies the instance?
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.
good idea, apart from checking result is larger than 0 added a TestCheckTypeSetElemNestedAttrs
to verify it contains an element with the stream instance name that was created in the test.
@@ -44,40 +41,40 @@ func TestAccStreamDSStreamInstances_withPageConfig(t *testing.T) { | |||
CheckDestroy: acc.CheckDestroyStreamInstance, | |||
Steps: []resource.TestStep{ | |||
{ | |||
Config: streamInstancesWithPageAttrDataSourceConfig(orgID, projectName, instanceName, region, cloudProvider), | |||
Check: streamInstancesAttributeChecks(dataSourceName, admin.PtrInt(2), admin.PtrInt(1), 0), | |||
Config: streamInstancesWithPageAttrDataSourceConfig(projectID, instanceName, region, cloudProvider, 1000), |
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.
could we give that 1000 a variable name?
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.
yes, extracted
Config: streamInstancesWithPageAttrDataSourceConfig(orgID, projectName, instanceName, region, cloudProvider), | ||
Check: streamInstancesAttributeChecks(dataSourceName, admin.PtrInt(2), admin.PtrInt(1), 0), | ||
Config: streamInstancesWithPageAttrDataSourceConfig(projectID, instanceName, region, cloudProvider, 1000), | ||
Check: streamInstancesAttributeChecks(dataSourceName, admin.PtrInt(1000), admin.PtrInt(1), acc.IntLowerThan(1)), // expecting no results |
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.
why not leaving 0?
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.
mess up from my end, leaving the check with 0 is just fine.
9b77917
to
b59a73c
Compare
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.
LGTM. Probably out of scope to refactor the datasource tests to be part of the resource tests?
will followup with a new PR for this change 👍 |
Description
Link to any related issue(s): CLOUDP-239134
Type of change:
Required Checklist:
Further comments