Adding experimental status and example usage for ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath plan and state checks
#276
Conversation
* Configuring when state checks are executed. * Testing that state checks are executed.
…or checking of null values (#266)
…CheckResourceAttrPtr
… TestCheckResourceAttrPtr
…e attributes rather than using output values
…h plan check with versions of Terraform <1.3.0
There was a problem hiding this comment.
Lots of great changes here, great work @bendbennett 🥳
Left a couple question/nits 
🚀
helper/resource/testing.go
Outdated
| // The following is an example of using [ExpectKnownValue] with [knownvalue.NotNull] | ||
| // to replicate the behaviour of TestCheckResourceAttrSet. |
There was a problem hiding this comment.
nit: Should this sentence have a little clarifier of why we're including the example? Something along the lines of "TestCheckResourceAttrSet is legacy", "will be deprecated" , or whatever context we think is appropriate.
Same feedback applies to other checks we aren't deprecating, but providing alternative examples for.
There was a problem hiding this comment.
Also these examples look great in VSCode with full syntax highlighting 🥳
There was a problem hiding this comment.
I was thinking that the examples might serve for "organic" adoption of, or experimentation with, the new state checks rather than having an overt declaration that the TestCheckFunc implementations are legacy, or will be deprecated until such time as we're happy to move the new plan and state checks out of experimental status and have all of the alternative implementations for replicating built-in TestCheckFunc(s) in place. But I'm happy to add some additional verbiage if that's the consensus.
There was a problem hiding this comment.
I lean a little towards Austin's clarity request here, it does feel like there needs to be something to describe why the example is there. Maybe something like "An experimental interface exists to to potentially replace this functionality in the future and feedback would be appreciated. This example performs the same check with that experimental interface:"
There was a problem hiding this comment.
Fair enough. Thanks for the suggested wording, have incorporated throughout.
| // NotNull returns a Check for asserting the value passed | ||
| // to the CheckValue method is not nil. |
There was a problem hiding this comment.
Curious q: Does this require any sort of mentioning SDKv2 behavior of storing "" vs nil and how knownvalue.NotNull won't detect that?
I don't fully understand the old SDKv2 behavior so this question may be off-base
There was a problem hiding this comment.
I'm a little reluctant to mention any SDK/v2-specific behaviour within the knownvalue package itself. Of course if the consensus is that we should make an explicit statement here regarding SDK/v2 behaviour then I'm happy to do that, but I'd be interested in hearing a little more detail around exactly what should appear in the Go docs under these circumstances. In terms of the behaviour of knownvalue.NotNull then it should be the same irrespective of whether SDK/v2 or the framework have been used to write the provider, as the check is just looking to see whether the value is not nil/null in the state or plan.
The built-in state checks for TestCheckResourceAttrSet, and TestCheckNoResourceAttr are different in that null values are removed from the state that is retrieved and used for evaluating TestCheckFunc(s). For example, the random provider, uses, TestCheckNoResourceAttr to verify that an optional attribute (override_special) does not appear in state. This is a little peculiar, given that optional attributes that are not defined in configuration are stored in state as null. Investigation as to why this usage of TestCheckNoResourceAttr executes successfully reveals that immediately prior to the TestStep.Check being run, the state is retrieved by calling getState(). This calls shimStateFromJson(), and ultimately shimmedFlatmap.AddEntry() which omits adding an entry for the attribute in the returned terraform.State if the attribute is nil, which is the case if the attribute has been omitted from the configuration, or explicitly set to null in the configuration.
Hope I'm not off the mark with this. Let me know if we need to discuss this further.
There was a problem hiding this comment.
Yeah that all makes sense to me. Probably best to avoid and if confusion arises, any documentation about knownvalue usage with SDKv2 would be best served in the SDKv2 section itself 😄
knownvalue/string_regexp.go
Outdated
|
|
||
| // StringRegularExpression returns a Check for asserting equality between the | ||
| // supplied regular expression and a value passed to the CheckValue method. | ||
| func StringRegularExpression(regex *regexp.Regexp) stringRegularExpression { |
There was a problem hiding this comment.
super nit 🦸🏻: Can we shorten this to StringRegex/StringRegexp?
The Go standard library already uses regexp and Terraform uses regex so it seems reasonable to assume this wouldn't cause too much confusion.
There was a problem hiding this comment.
Good point. Updated to use StringRegexp.
website/docs/plugin/testing/acceptance-tests/known-value-checks/index.mdx
Show resolved
Hide resolved
| } | ||
|
|
||
| if !v.regex.MatchString(otherVal) { | ||
| return fmt.Errorf("expected regex match %s for StringRegexp check, got: %s", v.regex.String(), otherVal) |
There was a problem hiding this comment.
Thank you for including the regex string in the error ❤️
|
|
||
| ## `NotNull` Check | ||
|
|
||
| The [NotNull](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing/knownvalue#NotNull) check tests that a resource attribute, or output value is not null. |
There was a problem hiding this comment.
Nit: I wonder if its worth mentioning here or anywhere else that "not null" also means "any known value"?
There was a problem hiding this comment.
I've updated the docs here. I didn't modify the Go docs for knownvalue.NotNull, as it seems explicit that the check is evaluating whether the passed value is nil or not.
bendbennett
changed the title
ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath plan and state checks
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
The original intent of this PR has been altered. Rather than deprecating the pre-existing built-in TestCheckFunc implementations, examples of how to use the newer
ExpectKnownValue,ExpectKnownOutputValue, andExpectKnownOutputValueAtPathplan and state checks, with theknownvaluepackage have been added.References: #266
This PR adds predefined/built-in state checks that provide equivalent functionality to that provided by the predefined TestCheckFunc functions (e.g., TestCheckNoResourceAttr).
The following implementations have been included:
ExpectNoValueExistsExpectKnownOutputValue,ExpectKnownOutputValueAtPathExpectKnownValueExpectMatchingValuesExpectValueExistsCustomknown value type check can be usedSetExactandSetPartialknown value type checks can be usedExpectContainsSetExactandSetPartialknown value type checks can be usedExpectKnownOutputValue, andExpectKnownOutputValueAtPathwithCustomknown value type check can be usedExpectKnownValuewithCustomknown value type check can be usedSetorListknown value type checks with nestedCustomknown value type checks can be used