Adding plan checks for `ExpectKnownValue`, `ExpectKnownOutputValue`, and `ExpectKnownOutputValueAtPath` #248

bendbennett · 2023-12-18T11:34:32Z

Closes: #243

Further Considerations

There are two related PRs open on terraform-json, and terraform-exec which, if they proceed, would allow for checking of numerical values in the plan using json.Number. The current PR will be amended if the PRs on terraform-json, and terraform-exec are merged and released.
There has been some discussion of adding assertions for plancheck.ExpectStringOutputValue(name string, value string), and similar. Do we want to add those to this PR, add an issue and implement later, or wait for an indication of community interest/utility?

…pe and value, and known value type and value (#243)

…ath (#243)

plancheck/expect_known_output_value.go

…on.Plan (#243)

bflad

Overall super excited about this as its a really nice enhancement for the testing logic. Please let me know if you want to chat about any of this.

internal/plugintest/working_dir.go

bflad · 2023-12-20T18:47:15Z

knownvalue/known_value.go

+	// Equal should perform equality testing.
+	Equal(other any) bool


Open question: Do we want to call this "check", "match", or another name? While exact equality is one use case with these, there are other use cases as well, e.g.

Regular expressions for strings

Minimum/maximum for numbers

Partial matching for collections or objects

Returning true for any value

I guess I'm just trying to ensure folks aren't confused or turned away for the not-exactly-equal use cases.

Seeing everything written out now, I almost wonder if this interface should be something like:

type Check interface { CheckValue(value any) error String() string }

There's at least two benefits I see here beyond the potentially clearer naming:

They can set their own error messaging.

They can raise implementation errors rather than potentially hiding those types of errors based on a specific boolean answer.

Provider developers can implement their own implementations without needing to reimplement something like ExpectKnownValue

I hesitate to say going down the fully-type-named interfaces, e.g.

type BoolCheck interface { CheckBool(value bool) error String() string }

Unless we'd consider passing json.Number directly for number checks. We could still have nice "int64", etc implementations even if we did that though.

I like the idea of returning an error and the additional flexibility that this yields.

In terms of the fully-type-named interfaces, I'm wondering about the internals. For instance, implementation of CheckBool() would be straightforward:

func (v BoolValue) CheckBool(other bool) error { if other != v.value { return fmt.Errorf("%t does not equal %t", other, v.value) } return nil }

With the call to CheckBool() from expectKnownValue.CheckPlan() looking something like the following:

switch reflect.TypeOf(result).Kind() { case reflect.Bool: v, ok := e.knownValue.(knownvalue.BoolValue) if !ok { resp.Error = fmt.Errorf("wrong type: attribute value is bool, known value type is %T", e.knownValue) return } err := v.CheckBool(result.(bool)) if err != nil { resp.Error = fmt.Errorf("attribute value: %v does not equal expected value: %s", result, v) return }

However, the internals for all of the KnownValue types that deal with maps, or slices, would, I believe, require some internal type assertion to determine whether the fully-type-named interface methods could be called. For example:

func (v ObjectValue) CheckObject(other map[string]any) error { if len(other) != len(v.value) { return fmt.Errorf("%v does not equal %v", other, v.value) } for k, v := range v.value { otherItem, ok := other[k] if !ok { return fmt.Errorf("%s is missing from %v", k, other) } switch reflect.TypeOf(otherItem).Kind() { case reflect.Bool: boolCheck, ok := v.(BoolValue) if !ok { return fmt.Errorf("wrong type: %T, known value type is %T", otherItem, v) } if err := boolCheck.CheckBool(otherItem.(bool)); err != nil { return fmt.Errorf("%v does not equal %v", otherItem, v) } // Reflection logic for reflect.Map, reflect.Slice, reflect.String default: return fmt.Errorf("unrecognised type: %T, known value type is %T", otherItem, v) } } return nil }

I'll go ahead and implement the Check interface, and perhaps we can go from there?

In that situation, yes, I would expect the collection/object checks to need to figure out what their element or per-object-attribute check type is. Another option would be to not support that functionality and leave it up to developer implementations to figure out what to do with making more generic collection/object checks that support elements/object attributes, but I think there's a lot of value in being able to offer that out of the box.

Not sure if it will make the logic easier/harder, but another way of doing this is performing a type switch on the v.value (presumably the per-object-attribute checks in the given example code) and raising an error if the otherItem doesn't conform to the given check type. This can help make sure the error messaging is tailored for the what the developer put into their object check definition rather than tailored for the data returned from Terraform, which without additional context can be harder to reason about in my opinion. e.g.

// note also including the object attribute name (k from example) and not assuming // it was an equality check error in error messaging (the checks can say so // themselves, amongst other possible errors :) ) switch check := v.(type) { case BoolValue: otherValue, ok := otherItem.(bool) if !ok { return fmt.Errorf("%s object attribute: expected bool value for BoolValue check, got: %T", k, otherItem) } if err := check.CheckBool(otherValue); err != nil { return fmt.Errorf("%s object attribute: %s", k, err) } // ... default: }

I guess a potential wrinkle here is, what if there is a dynamically typed value is being checked? Hmm. I would normally presume the value should be statically typed based on the given Terraform configuration. If not, we would be fine as long as there is some sort of escape hatch for a "I don't know the value type and I'll do all the work to figure it out" type of check, e.g. CheckDynamic(value any) error.

I've refactored to use the Check interface:

type Check interface { CheckValue(value any) error String() string }

Before proceeding down the route of using fully-type-named interface, thought it might be worth considering how the ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath would handle these fully-type-named interfaces.

Currently, taking ExpectKnownValue for instance, the type and constructor look as follows:

var _ PlanCheck = expectKnownValue{} type expectKnownValue struct { resourceAddress string attributePath tfjsonpath.Path knownValue knownvalue.Check } func (e expectKnownValue) CheckPlan(ctx context.Context, req CheckPlanRequest, resp *CheckPlanResponse) { /* ... */ } func ExpectKnownValue(resourceAddress string, attributePath tfjsonpath.Path, knownValue knownvalue.Check) PlanCheck { return expectKnownValue{ resourceAddress: resourceAddress, attributePath: attributePath, knownValue: knownValue, } }

As an example, BoolValue currently implements the Check interface:

var _ Check = BoolValue{} type BoolValue struct { value bool } func (v BoolValue) CheckValue(other any) error { /* ... */ } /* ... */ func BoolValueExact(value bool) BoolValue { return BoolValue{ value: value, } }

If we refactor BoolValue to implement the BoolCheck interface:

type BoolCheck interface { CheckBool(value bool) error String() string } var _ BoolCheck = BoolValue{} type BoolValue struct { value bool } func (v BoolValue) CheckBool(value bool) error { /* ... */ } /* ... */ func BoolValueExact(value bool) BoolValue { return BoolValue{ value: value, } }

Using fully-type-named interfaces will require that we reconsider how the constructor and struct field holding the checks will be handled for ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath, as we'll no longer be able to construct an ExpectKnownValue plan check, for instance, using the following:

plancheck.ExpectKnownValue( "test_resource.one", tfjsonpath.New("bool_attribute"), knownvalue.BoolValueExact(true), ),

knownvalue/known_value.go

knownvalue/bool.go

knownvalue/bool_test.go

website/docs/plugin/testing/acceptance-tests/known-values.mdx

bflad · 2023-12-20T20:06:41Z

knownvalue/object_partial.go

+// NewObjectValuePartial returns a KnownValue for asserting partial equality between the
+// supplied map[string]KnownValue and the value passed to the Equal method.


Maybe its obvious, but in case it might not be, it might be good to talk about the "partial equality" a little bit more for the "Partial" implementations, e.g. in this case that the map keys are intended to be object attribute names and that only those attribute values are checked.

Have added some further clarification around "partial equality". Let me know if you think we need more here.

website/docs/plugin/testing/acceptance-tests/known-values.mdx

bflad · 2023-12-20T20:26:13Z

website/docs/plugin/testing/acceptance-tests/known-values.mdx

+})
+```
+
+## Known Value Types


I love these tables but I wonder if we should consider laying this out a little differently based first on the type system types, then each type's available checks, instead of being based on the kind of check (e.g. exact vs partial vs whatever). I don't think we should be shy about making separate pages since there is a lot of good/relevant content.

One potential way to do this would be move this page to known-value(-checks)/index.mdx and treat it as an overview page where the tables then link to checks available for each type system type, e.g. known-value(-checks)/list.mdx. That page could then walk through the available list type checks, e.g. exact, partial, number of elements, etc. A string page could walk through the available string type checks, e.g. exact, regular expression, etc. A new custom page could walk through how developers can implement their own.

Have refactored, splitting out by Known Value Check type.

Also added a Custom Known Value Check page. Thought I'd hold off expanding on this until we finalise the interface(s) we're going to use.

A consequence of allowing custom known value checks is that the internal logic within ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath should be able to handle any known value checks that fill the Check interface. The internals of the the Check function for these three types has therefore been refactored to provide this flexibility.

knownvalue/num_elements.go

…tKnownOutputValueAtPaath (#243)

…merical value representation in the plan (#243)

…#243)

…ove references to KnownValue interface (#243)

…ue, ExpectKnownOutputValue and ExpectKnownOutputValueAtPath (#243)

bflad

Excellent work! Other than some minor quibbles over exporting less things upfront and the forever naming bikeshedding, this looks good to me 🚀 Excited to see what developers build with the interface.

bflad · 2024-01-10T15:14:14Z

knownvalue/bool.go

+
+// BoolValue is a Check for asserting equality between the value supplied
+// to BoolValueExact and the value passed to the CheckValue method.
+type BoolValue struct {


Two questions:

Should we leave these Check implementations as unexported except for the functions (e.g. boolValue here)? It'll clean up the public Go package documentation nicely and I'm not sure if there is anything that should need to reference/extend the implementation directly since its value field is unexported.

If doing that, maybe these names can then match the exported functions? e.g. boolValueExact for some additional clarity while reading the code

We can always export additional things in the future if we need! 👍

+1 for reducing the amount of exported types 👍🏻

Sounds good to me.

The known value check types have all be modified so that they are camel-cased versions of the exported functions, for example:

boolValueExact => BoolValueExact

listElementsExact => ListElementsExact`

listValueExact => ListValueExact

listValuePartial => ListValuePartial (note that the constructor for the know value partial types has changed from <List|Map|Object|Set>ValuePartialMatch to <List|Map|Object|Set>ValuePartial)

bflad · 2024-01-10T15:17:00Z

knownvalue/bool.go

+	otherVal, ok := other.(bool)
+
+	if !ok {
+		return fmt.Errorf("expected bool value for BoolValue check, got: %T", other)


Followup to above: If the only exported thing is the function, then these errors here and in others can reference the function name (BoolValueExact) which is super nice since that's how developers will be familiar with them 😄

bflad · 2024-01-10T15:21:43Z

knownvalue/bool_test.go

+
+// equateErrorMessage reports errors to be equal if both are nil
+// or both have the same message.
+var equateErrorMessage = cmp.Comparer(func(x, y error) bool {


Sometimes I wish this was natively in go-cmp 😂

knownvalue/known_value.go

bflad · 2024-01-10T15:35:27Z

knownvalue/list_elements.go

+
+// ListElementsExact returns a Check for asserting that
+// a list num elements.
+func ListElementsExact(num int) ListElements {


❤️ having room for things like ListElementsAtLeast, etc. which is the important part here. Not sure if folks might be more familiar with "size" over "elements" as a synonymous with "element count" though, since for example, we name these validators SizeAtLeast, etc. Just wanted to mention, not necessarily say it has to be that way or anything.

Totally happy to refactor to <List|Map|Set>SizeExact. Having some level of parity with the validators seems beneficial to me.

Only question I have if we go this route is the naming for the object equivalent - ObjectNumAttributesExact, ObjectLenAttributesExact, ObjectAttributesCountExact.........

Ah, for objects I would argue that its a little awkward to count their attributes since that's part of their static type definition. This differs from map elements where there could be an arbitrary amount from an API.

Have renamed to <List|Map|Set>SizeExact, and removed ObjectAttributesExact.

knownvalue/map_elements.go

bflad · 2024-01-10T15:44:40Z

knownvalue/number.go

+// NumberValueExact returns a Check for asserting equality between the
+// supplied *big.Float and the value passed to the CheckValue method.


I wonder if it might be good to call out the 512-bit precision here too 😄

bflad · 2024-01-10T15:57:00Z

plancheck/expect_known_output_value_at_path.go

+		reflect.Slice,
+		reflect.String:
+		if err := e.knownValue.CheckValue(result); err != nil {
+			resp.Error = err


Should these errors include the path string as well for developers?

I've added a String() method to tfjson.Path for this purpose.

Awesome, thanks!

bflad · 2024-01-10T15:57:53Z

plancheck/expect_known_value.go

+		reflect.Slice,
+		reflect.String:
+		if err := e.knownValue.CheckValue(result); err != nil {
+			resp.Error = err


Similarly here regarding the path string

bflad · 2024-01-10T16:01:10Z

website/docs/plugin/testing/acceptance-tests/known-value-checks/bool.mdx

+func TestExpectKnownValue_CheckPlan_Bool(t *testing.T) {
+	t.Parallel()
+
+	r.Test(t, r.TestCase{


I'd suggest using resource written out in these examples to match the import without aliasing.

austinvalle

I had a question about the naming scheme below, but not a hard stance by any means. All of this code looks awesome!

austinvalle · 2024-01-10T20:41:22Z

knownvalue/known_value.go

super nit 🦸🏻 - Should this file be named check.go now?

austinvalle · 2024-01-10T20:45:53Z

knownvalue/bool.go

+
+// BoolValue is a Check for asserting equality between the value supplied
+// to BoolValueExact and the value passed to the CheckValue method.
+type BoolValue struct {


+1 for reducing the amount of exported types 👍🏻

austinvalle · 2024-01-10T20:52:30Z

knownvalue/bool.go

+func BoolValueExact(value bool) BoolValue {
+	return BoolValue{
+		value: value,
+	}
+}


Wanted to bring up the first thing I thought of with the naming. Thinking there may be some context that I'm missing that's important to the explicitness of the naming.

Is there a benefit in duplicating the Value in some of these implementations? Since it's already in the package name as knownvalue.BoolValueExact() vs. knownvalue.BoolExact(). Does this naming scheme break down/not make sense in certain use-cases?

Some examples:

knownvalue.Null()

knownvalue.ObjectPartial()

knownvalue.ObjectExact()

knownvalue.ListExact()

knownvalue.ListPartialMatch()

knownvalue.MapExact()

knownvalue.MapPartialMatch()

knownvalue.StringExact()

Leaving room for others that become clearer with the additional words, like:

knownvalue.ListElementsExact()

knownvalue.MapElementsExact()

I'm on board with this naming change, and potentially the change in names for the "Size" checks too.

Happy to go with consensus here. What are your thoughts @bflad ?

I'm +1 for removing repeated words for when folks need to use something, so I'm +1 for removing Value out of the naming in the knownvalue package.

Have refactored. Thanks for the suggestion.

Co-authored-by: Brian Flad <bflad417@gmail.com>

…266)

…243' into bendbennett/issues-243

…eExact (#243)

github-actions · 2024-02-15T02:00:12Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

bendbennett added 6 commits December 15, 2023 11:19

Add KnownValue interface and types (#243)

34cee4a

Add ExpectKnownValue plan check (#243)

6915f0e

Handling different permutations for equality checking of interface ty…

f503594

…pe and value, and known value type and value (#243)

Adding tests for missing resource, and attribute value null (#243)

4f8eed9

Adding plan checks for known output value and known output value at p…

a1f35f6

…ath (#243)

Adding documentation (#243)

853a3e4

bendbennett added the enhancement New feature or request label Dec 18, 2023

bendbennett added this to the v1.7.0 milestone Dec 18, 2023

bendbennett added 3 commits December 18, 2023 11:34

Merge branch 'main' into bendbennett/issues-243

f88c44b

Adding changelog entries (#243)

0ccf758

Adding TerraformVersionChecks (#243)

f161203

bendbennett commented Dec 18, 2023

View reviewed changes

plancheck/expect_known_output_value.go Show resolved Hide resolved

This was referenced Dec 20, 2023

build(deps): Bump github.com/hashicorp/terraform-exec from 0.19.0 to 0.20.0 #252

Closed

build(deps): Bump github.com/hashicorp/terraform-json from 0.18.0 to 0.19.0 #253

Closed

bendbennett added 2 commits December 20, 2023 14:16

Modifying to handle numerical values returned as json.Number for tfjs…

e13bdaa

…on.Plan (#243)

Merge remote-tracking branch 'origin/main' into bendbennett/issues-243

916674e

bendbennett marked this pull request as ready for review December 20, 2023 14:21

bendbennett requested a review from a team as a code owner December 20, 2023 14:21

bflad reviewed Dec 20, 2023

View reviewed changes

bendbennett added 11 commits January 2, 2024 11:20

Renaming known value constructors (#243)

450339d

Refactoring to Check interface (#243)

31cbf09

Merge remote-tracking branch 'origin/main' into bendbennett/issues-243

73a5300

Linting (#243)

3f14259

Modifying known value check error messages and tests (#243)

5e61524

Updating tests for ExpectKnownValue, ExpectKnownOutputValue and Expec…

41eb4de

…tKnownOutputValueAtPaath (#243)

Adding changelog entry to note the switch to using json.Number for nu…

94bdfa7

…merical value representation in the plan (#243)

Remove reference to state checks (#243)

f09f7e9

Moving concepts under title and removing reference to Framework types (…

7c5c177

…#243)

Updating Go doc comments to clarify usage of partial equality and rem…

1638808

…ove references to KnownValue interface (#243)

Modifying known-values.mdx page description (#243)

213b81a

bendbennett added 7 commits January 4, 2024 09:49

Restructuring and updating references to knownvalue.Check (#243)

fa8f125

Adding individual docs pages for each type of known value check (#243)

91417e1

Removing references to num elements (#243)

463c0e3

Removing references to state (#243)

a661490

Adding docs page for custom known value checks (#243)

6b626f4

Fixing error message (#243)

ad071ae

Refactoring to accomodate custom known value checks in ExpectKnownVal…

efb0b98

…ue, ExpectKnownOutputValue and ExpectKnownOutputValueAtPath (#243)

bflad approved these changes Jan 10, 2024

View reviewed changes

austinvalle approved these changes Jan 10, 2024

View reviewed changes

bendbennett and others added 10 commits January 11, 2024 10:09

Apply suggestions from code review

9e5cde1

Co-authored-by: Brian Flad <bflad417@gmail.com>

Unexporting types that implement known value check (#266)

d14429e

Document usage of 512-bit precision in the number known value check (#…

f1b9656

…266)

Adding attribute or output path to error message (#266)

ed9d236

Replacing alias in example code (#266)

9bdf445

Rename file (#266)

0849854

Merge remote-tracking branch 'refs/remotes/origin/bendbennett/issues-…

4f283cf

…243' into bendbennett/issues-243

Renamed list, map, and set element length checks to <List|Map|Set>Siz…

edcddb6

…eExact (#243)

Removing ObjectAttributesExact (#243)

5f4c952

Renaming known value check types (#243)

aab63ef

bendbennett merged commit 198c751 into main Jan 15, 2024
27 checks passed

bendbennett deleted the bendbennett/issues-243 branch January 15, 2024 12:28

github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding plan checks for `ExpectKnownValue`, `ExpectKnownOutputValue`, and `ExpectKnownOutputValueAtPath` #248

Adding plan checks for `ExpectKnownValue`, `ExpectKnownOutputValue`, and `ExpectKnownOutputValueAtPath` #248

bendbennett commented Dec 18, 2023 •

edited

Loading

bflad left a comment

bflad Dec 20, 2023

bendbennett Jan 2, 2024

bflad Jan 2, 2024

bflad Jan 2, 2024

bendbennett Jan 3, 2024

bflad Dec 20, 2023

bendbennett Jan 4, 2024

bflad Dec 20, 2023

bendbennett Jan 4, 2024 •

edited

Loading

bflad left a comment

bflad Jan 10, 2024

austinvalle Jan 10, 2024

bendbennett Jan 11, 2024

bflad Jan 10, 2024

bflad Jan 10, 2024

bflad Jan 10, 2024

bendbennett Jan 11, 2024

bflad Jan 12, 2024

bendbennett Jan 15, 2024

bflad Jan 10, 2024

bflad Jan 10, 2024

bendbennett Jan 11, 2024

bflad Jan 12, 2024

bflad Jan 10, 2024

bflad Jan 10, 2024

austinvalle left a comment

austinvalle Jan 10, 2024

austinvalle Jan 10, 2024

austinvalle Jan 10, 2024

bendbennett Jan 11, 2024 •

edited

Loading

bflad Jan 12, 2024

bendbennett Jan 15, 2024

github-actions bot commented Feb 15, 2024

		// Equal should perform equality testing.
		Equal(other any) bool

		// NewObjectValuePartial returns a KnownValue for asserting partial equality between the
		// supplied map[string]KnownValue and the value passed to the Equal method.

		// NumberValueExact returns a Check for asserting equality between the
		// supplied *big.Float and the value passed to the CheckValue method.

Adding plan checks for ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath #248

Adding plan checks for ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath #248

Conversation

bendbennett commented Dec 18, 2023 • edited Loading

Further Considerations

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvalle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 15, 2024

Adding plan checks for `ExpectKnownValue`, `ExpectKnownOutputValue`, and `ExpectKnownOutputValueAtPath` #248

Adding plan checks for `ExpectKnownValue`, `ExpectKnownOutputValue`, and `ExpectKnownOutputValueAtPath` #248

bendbennett commented Dec 18, 2023 •

edited

Loading

bendbennett Jan 4, 2024 •

edited

Loading

bendbennett Jan 11, 2024 •

edited

Loading