-
Notifications
You must be signed in to change notification settings - Fork 12
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 tests #6
Adds tests #6
Conversation
@@ -181,6 +181,35 @@ func TestViewBasic(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestViewBasicUpdate(t *testing.T) { |
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.
So in these tests, I don't see any expected failure tests—explicit failure cases such as passing a boolean instead of a string or an invalid value instead of a valid one for values that are constrained to a set. I'm not comfortable approving a PR without those, personally, as those failure cases need to be handled gracefully to ensure a strong developer experience.
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.
@nimbinatus all the expected failure tests can be found here:
func TestView_expectServiceKeyError(t *testing.T) { |
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 see. I don't see a failure test for a view, however. Maybe this isn't a thing in Go, but I suspect it is. I'm expecting something like this, but in actual Go code and not my poor pseudocode:
func TestAssertErrorViewBulkEmails(t *testing.T err error) {
name := "test"
query := "test"
category := "DEMO"
tags1 := "host1"
tags2 := 123
resource.Test(t, resource.TestCase{
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: testViewConfigBulkEmails(name, query, category, tags1, tags2),
Check: resource.ComposeTestCheckFunc(
testViewExists("logdna_view.new"),
resource.TestCheckResourceAttr("logdna_view.new", "name", name),
resource.TestCheckResourceAttr("logdna_view.new", "query", query),
resource.TestCheckResourceAttr("logdna_view.new", "category.#", "1"),
resource.TestCheckResourceAttr("logdna_view.new", "category.0", category),
resource.TestCheckResourceAttr("logdna_view.new", "tags.#", "2"),
resource.TestCheckResourceAttr("logdna_view.new", "tags.0", tags1),
resource.TestCheckResourceAttr("logdna_view.new", "tags.1", tags2),
resource.TestCheckResourceAttr("logdna_view.new", "webhook_channels.#", "0"),
),
},
},
})
if assert.Error(t, err) {
assert.Equal(t, expectedError, err)
}
else {
t.Fail()
t.Logf("View created with invalid values %s", tags2) # Though I'd also expect the single string for `categories` to fail as it's not an array.
}
}
Personally, I'd also add tests to validate each string for the key-value pairs that have set values as options. For example, what if someone put pagerduty_channel.0.immediate
as "no"
instead of "false"
—or if they mistyped and entered "fasle"
, which I've done myself many times? How do you test for a graceful failure or error here? We'd rather see that kind of mistake just error out with a reasonable message for the user versus possibly crashing or giving the user no feedback at all.
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.
@nimbinatus am following the guidelines provided here on how to test error handling with the terraform sdk: https://www.terraform.io/docs/extend/best-practices/testing.html#expecting-errors-or-non-empty-plans:
You can see in the test here that if the the user doesn't provide a servicekey the provider will give the following error:
ExpectError: regexp.MustCompile("The argument \"servicekey\" is required, but no definition was found."),
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.
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.
Yeah I'm not sure how to make assertions on errors returned from the config-api with the terraform sdk and the ExpectError function. I've also created a post here: https://discuss.hashicorp.com/t/testing-errors-when-building-a-terraform-provider/16770
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 configure terraform error levels but even the lowest level still has that debug message at the beginning: https://www.terraform.io/docs/internals/debugging.html
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.
After talking with some folks at Hashicorp, the ExpectError
is the right thing to use. For the most part, people test for the happy case, but it isn't necessarily uncommon to test for errors as I noted. They sent along samples like https://github.com/terraform-providers/terraform-provider-aws/search?q=expecterror and https://github.com/hashicorp/terraform-provider-google/search?q=expecterror.
I think that, in this case, the ExpectError
is the assert, so it is the way to go. Some similar ideas to look at:
- https://github.com/hashicorp/terraform-provider-google/blob/b0e3ae2a9e7235af180fc1c06380659d0b3a474f/google/resource_bigtable_instance_test.go#L27
- A bit more involved, using the
TestCheckFunc
(docs) here called here
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 also okay with dropping this for a later PR if @mdeltito is good with that.
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.
yeah I think this is good - we have test coverage for error cases on custom validators (among other things) which I think is the most important.
@dm36 we'll need to sync up on what the issue was with using ExpectError
for other error cases at a later point, I'm not sure I really understand based on your screenshots after testing these things locally.
this is looking good to me - I'm not finding any info on testing http-related errors via the builtin |
cea4681
to
9035ef4
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.
A couple of comments, I think this is looking good though. I still think we could use some tests around the error cases in client.go
Providers: testAccProviders, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testViewConfigBulkEmails(name, query, app1, app2, levels1, levels2, host1, host2, category, category2, tags1, tags2), |
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.
This pattern of adding infinite function parameters to a templating seems pretty unmaintainable. The test config is very static, even for the fields that are using the values passed in - there are always 2 apps, 2 hosts, etc. So the added indirection of a function wrapper around fmt.Sprintf
doesn't really do much for us, aside from providing a place to store the template string. I don't think we're gaining much here by having this as a function vs just defining the template string as a top-level variable and calling fmt.Sprintf
directly.
I don't think it's necessary to change in this PR, just a pattern I think we should find an alternative for in future work.
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.
how best to change this in the future? passing along the values in the sprintf also makes sense in the context that I can assert that the values match the variables in the TestCheckResourcesAttr but I also do agree that the # of variables is a lot for a couple of the tests
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 could not sprintf or pass anything to the config functions (testViewConfigBulkEmails for example) but still have the variables as reference and assert on them in the TestCheckResourcesAttr
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.
yeah you could have the template string as a top-level variable and use fmt.Sprintf
where you need it. just removing a layer:
var testConfigBulkEmails = `provider "logdna" {
}
resource "logdna_view" "new" {
name = "%s"
query = "%s"
}`
func TestViewBulkEmailsUpdate(t *testing.T) {
foo := "foo"
bar := "bar"
config := fmt.Sprintf(testConfigBulkEmails, foo, bar)
...
there might be other ways to manage all of this too, I'm just thinking of the simplest form. I still think it's fine as-is for now.
@@ -181,6 +181,35 @@ func TestViewBasic(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestViewBasicUpdate(t *testing.T) { |
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.
@dm36 RE that screenshot, it looks like you'd just need to adjust the regex to match on the error you're looking for? It looks like it should work to use ExpectError
to test that the user gets the expected error output - the extra debug log output in the error text might be throwing you off. Can that debug output be disabled under test?
you also likely need to escape (at minimum) the bracket characters in the regexp.MustCompile
Error: "channels[0].immediate" must be...
Makes updates to the README and docs such as pluralizing category. Additionally makes code changes to comply with the categories update Ref: LOG-7687 Semver: Patch
Improve coveralls coverage by testing updates and error-handling. Ref: LOG-7721 Semver: Patch
9035ef4
to
4f0e963
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.
I'm good with it as long as Mike is, but I'd wait for further approvals from the other engineers before merging.
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.
looks good. LOG-7779 made for followup task error handling support.
nimbinatus said that she's good as long as Mike is and deferred to other engineers for further approval (and Yash picked this up).
Improve coveralls coverage by testing updates and error-handling.
Ref: LOG-7721
Semver: Patch