-
Notifications
You must be signed in to change notification settings - Fork 54
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
Adds acceptance tests for datasources for HVN, Consul and Vault clusters #135
Conversation
tier = "development" | ||
} | ||
|
||
data "hcp_consul_cluster" "test" { |
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.
I'm getting a bit concerned about test time. Since we have to create the resource here, maybe we could take this config and add it in a new test step on the resource tests. We'll have to document the pattern/leave comments, but I think it's worthwhile to save test time, since the creation/teardown of these cloud resources takes at least 10 minutes.
HVNs, Consul clusters, and Vault clusters
ba2c8e8
to
268deea
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.
Nice work on these! I've got two suggestions that shouldn't take too long to fix.
@@ -105,6 +109,33 @@ func TestAccConsulCluster(t *testing.T) { | |||
resource.TestCheckResourceAttrSet(resourceName, "size"), | |||
), | |||
}, | |||
{ |
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.
Two things on each of these tests:
- Since we have a few test steps now, it'd be great to add some guidance comments at the beginning of each step. I.e. first step testsCreate, second step tests Import, third step tests Updates, and fourth step tests the datasource. It might even be worth an intro comment at the top explaining that the datasource test is incorporated in the resource test.
- To have more separation between the test cases, I'm thinking it'd be worth having two TF configs, one with just the resource and the other adding the data source (which we'd use just in this last step).
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.
When I split the configs up the tests errored out, so I ended up just adding comments to each step in the tests
@@ -1,3 +1,5 @@ | |||
// This includes tests against both the resource and the corresponding datasource |
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.
thanks for adding these! last thing: can you bump this down to above L33? Same for the others? I don't usually see comments above the package in go so I think this'll be easy to miss.
π οΈ Description
Adds acceptance tests for datasources for HVN, Consul and Vault clusters.
ποΈ Acceptance tests
Output from acceptance testing: