(WIP) Add tests for DynamicPseudoType behavior #208

austinvalle · 2024-01-19T16:04:02Z

Ref: hashicorp/terraform-plugin-framework#147

In preparation for introducing dynamic schema attributes and type handling in terraform-plugin-framework, this PR adds tests to verify the behavior of tftypes.DynamicPseudoType, which the plugin framework dynamic code will be abstracting.

I tried to add comments in places where the test isn't self-explanatory ™️, but please ask questions if something looks off or an explanation doesn't describe enough.

The tests currently implemented in this PR covers resource schemas and built-in functions for Protocol v6, some follow-up tests that still need to be introduced:

Data source schemas
Provider schemas
Duplicate as many tests as possible for Protocol v5
- This will include some implementation for the testsdk for v5 support
Provider-defined functions (will probably wait until Terraform 1.8 release?)

PR Notes

This PR includes a shameless copy/pasta of the useful declarative plugin-go SDK created in: https://github.com/hashicorp/terraform-plugin-testing/tree/main/internal/testing 😄. Currently, none of it has been modified, but I will eventually add protocol v5 support, which should be purely additive.
I'm utilizing the latest branch of terraform-plugin-testing, but will switch to the newest release when available

bflad

Overall super excited to get this level of end-to-end testing! There is a ton of great tests and assertions in here. My comments probably fall more under pedantic than anything.

bflad · 2024-01-31T14:22:20Z

.github/workflows/test.yml

@@ -96,6 +97,7 @@ jobs:
      - run: go mod download
      - run: go test -v -cover ./internal/framework6provider/
      - run: go test -v -cover ./internal/protocolv6provider/
+      - run: go test -v -cover ./internal/dynamic6provider/


It'd be nice to not expand our packages here and use protocol(v6)provider because it means updating the terraform-plugin-go CI and I'm not sure it offers much benefit over having all the terraform-plugin-go tests in one package per protocol version. Maybe we can introduce the testsdk and update the existing provider in a separate PR, then the dynamic tests are additive to the existing provider?

That's a good shout and will make even more sense once framework implements dynamic types and we add tests for that.

I think I can wait for @bendbennett's work on #210 to merge, then will add all my tests to the protocolv6provider + protocolprovider and switch them all to using the testsdk.

#210 is almost complete with the exception of dealing with hashicorp/terraform-plugin-framework#914. I'll be looking into that imminently, but don't let me hold you up in terms of merging this PR.

I don't plan on opening this PR for review until the dynamic type work in terraform-plugin-framework is nearing completion (which I just started), so I think we still have some time, whoever gets to it first 😆

After that work is done, I want to make sure we also have a discussion with the core team to ensure we aren't exposing dynamics in any way that could be invalid/confusing.

internal/dynamic6provider/dynamic_functions_test.go

bflad · 2024-01-31T14:44:00Z

internal/dynamic6provider/dynamic_import_test.go

+
+func Test_Dynamic_ImportState(t *testing.T) {
+	r.UnitTest(t, r.TestCase{
+		Steps: []r.TestStep{


One thing to potentially consider with this testing is whether we should make it two TestStep so it can enable ImportStateVerify: true and verify that there are no state differences between an originally applied state and imported state (or create a separate test like that). This is typically how import testing is done for regular providers and I think its a nice benefit even in this case since it will check the imported state contents beyond core's "schema correctness" of the import response.

I wasn't 100% sure how to write an import test, but I took a look at the AWS provider and I think I fixed it in 95d52c2

bflad · 2024-01-31T14:47:43Z

internal/dynamic6provider/dynamic_literals_test.go

+								},
+							},
+						},
+						PlanChangeFunc: verifyDynamicTypeByPath(


Verifying the type is awesome! Love this. While we're already so close to it, should we just verify the value as well? It could catch bizarre conversion bugs in tftypes, e.g. if the element values got errantly re-arranged for example.

Sure! I adjusted the verify method to just accept a tftypes.Value and use the type and value of that to verify. Seemed more ergonomic 😆

I broke them into two separate assertions for different messaging (even though I think (tftypes.Value).Equal also verifies the type.) Does that make sense or should we just stick to one assert?

0fd1b90

internal/dynamic6provider/dynamic_nesting_modes_test.go

austinvalle · 2024-01-31T22:30:25Z

internal/dynamic6provider/dynamic_element_type_test.go

+						// The type is stored as a tuple, but we can still use `ListExact` to verify state
+						knownvalue.ListExact([]knownvalue.Check{
+							knownvalue.StringExact("hey"),
+							knownvalue.ObjectExact(map[string]knownvalue.Check{
+								"number": knownvalue.Int64Exact(12345),
+							}),
+							knownvalue.ListExact([]knownvalue.Check{
+								knownvalue.StringExact("there"),
+								knownvalue.StringExact("tuple"),
+							}),
+						}),


@bflad @bendbennett I hesitate to suggest we add a TupleExact, but just wanted to point out that this works fine

Interesting. If there are legitimate use-cases for TupleExact then perhaps we can add this in going forwards.

bendbennett

This is marvellous!
Great job.

As discussed out-of-band, perhaps we can think about documenting some of the procedures for extracting and examining msgpack data to aid provider developers and practitioners.

bendbennett · 2024-02-01T13:19:25Z

.github/workflows/test.yml

@@ -96,6 +97,7 @@ jobs:
      - run: go mod download
      - run: go test -v -cover ./internal/framework6provider/
      - run: go test -v -cover ./internal/protocolv6provider/
+      - run: go test -v -cover ./internal/dynamic6provider/


#210 is almost complete with the exception of dealing with hashicorp/terraform-plugin-framework#914. I'll be looking into that imminently, but don't let me hold you up in terms of merging this PR.

bendbennett · 2024-02-01T13:21:25Z

go.mod

@@ -9,7 +9,7 @@ require (
 	github.com/hashicorp/terraform-plugin-go v0.21.0
 	github.com/hashicorp/terraform-plugin-mux v0.14.0
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.32.0
-	github.com/hashicorp/terraform-plugin-testing v1.6.0
+	github.com/hashicorp/terraform-plugin-testing v1.6.1-0.20240130155516-51777dda582e


I'll tag a release once we've finalised and merged hashicorp/terraform-plugin-testing#276

bendbennett · 2024-02-01T13:29:18Z

internal/dynamic6provider/dynamic_element_type_test.go

+					"corner_dynamic_thing": {
+						SchemaResponse: &resource.SchemaResponse{
+							Schema: &tfprotov6.Schema{
+								Block: &tfprotov6.SchemaBlock{


Because I spend far more time looking at a schema at the "Framework-level", scanning Block: ... always makes me think, hang on a minute, given the config that's being used this isn't a block, it's an attribute. Always makes me think of Plugin Protocol v6: What is a block, even?.

bendbennett · 2024-02-01T13:41:20Z

internal/dynamic6provider/dynamic_element_type_test.go

+						"corner_dynamic_thing.foo",
+						tfjsonpath.New("attribute_with_dpt"),
+						knownvalue.ListExact([]knownvalue.Check{
+							knownvalue.StringExact("hey"),


Might not be appropriate for this PR, or this repo even, but if it's not documented somewhere, is it worth providing some information about the rules that Terraform applies when there is ambiguity in the types specified in configuration, in this instance a string and an integer, and how that is handled in the context of a attributes that use tftypes.DynamicPseudoType? Does Terraform take the first value (i.e., "hey"), determine that it is a string, then determine if all of the subsequent values in the list can be converted to a string?

I'm not sure it's documented in the context you're describing, but it is an interesting thought I also share.

Just observing the behavior of Terraform/cty it looks potentially arbitrary, but digging more into cty documentation, I believe it may just be following the only safe conversion route possible: https://github.com/zclconf/go-cty/blob/main/docs/convert.md#conversion-charts

The literal representation of this specific test config is known to Terraform as: tuple[string, number]

The schema we defined tells Terraform/cty that it needs to be converted to a list, which has a single element type that is not determined (DynamicPseudoType)

Terraform needs to determine the element type, so it needs to find a safe route to converge [string, number]. Which the only safe option according to that table is string

If we switch the config around to:

resource "corner_dynamic_thing" "foo" { attribute_with_dpt = [12345, "hey"] }

We receive the same element type as string from the msgpack config:

{ "attribute_with_dpt": [ [ "\"string\"", "12345" ], [ "\"string\"", "hey" ] ] }

It would be nice to get some confirmation on this behavior, so I will note this down as something to ask the core team so we can understand a little deeper 👍🏻

bendbennett · 2024-02-01T14:05:59Z

internal/dynamic6provider/dynamic_element_type_test.go

+				// This may eventually be considered an invalid schema. It currently behaves as expected but may not be exposed to provider developers
+				// to avoid confusion. While the element type is dynamic, all elements must still have the exact same type, which Terraform will achieve
+				// by either performing a conversion (like below, converting 12345 to "12345"), or throw an error if conversion is impossible, for example:
+				//
+				// 	Error: Incorrect attribute value type
+				//
+				// 		on terraform_plugin_test.tf line 12, in resource "corner_dynamic_thing" "foo":
+				// 		 					attribute_with_dpt = {
+				// 		 						"key1" = "hey"
+				// 		 						"key2" = {
+				// 		 							number = 12345
+				// 		 						}
+				// 		 					}
+				//
+				//   Inappropriate value for attribute "attribute_with_dpt": all map elements must have the same type.
+				//
+				// Related issue: https://github.com/hashicorp/terraform/issues/34574


❤️ the extensive notes/documentation.

bendbennett · 2024-02-01T15:07:53Z

internal/dynamic6provider/dynamic_element_type_test.go

+						// The type is stored as a tuple, but we can still use `ListExact` to verify state
+						knownvalue.ListExact([]knownvalue.Check{
+							knownvalue.StringExact("hey"),
+							knownvalue.ObjectExact(map[string]knownvalue.Check{
+								"number": knownvalue.Int64Exact(12345),
+							}),
+							knownvalue.ListExact([]knownvalue.Check{
+								knownvalue.StringExact("there"),
+								knownvalue.StringExact("tuple"),
+							}),
+						}),


Interesting. If there are legitimate use-cases for TupleExact then perhaps we can add this in going forwards.

bendbennett · 2024-02-01T15:18:47Z

internal/dynamic6provider/dynamic_element_type_test.go

+													tftypes.DynamicPseudoType,
+													tftypes.DynamicPseudoType,


Not directly related to this PR, but given that within terraform-plugin-go, tftypes.DynamicPseudoType is defined as a primitive, it seems a little odd that it can be used for any Terraform type.

Right, I think the documentation and wording will be important for framework developers to understand. As described by cty, it's a placeholder for a type that will eventually be determined, "pseudo-type": https://github.com/zclconf/go-cty/blob/main/docs/types.md#the-dynamic-pseudo-type.

One consequence of this being a "pseudo-type" is that there is no known, non-null value of this type

bendbennett · 2024-02-01T15:29:38Z

internal/dynamic6provider/dynamic_literals_test.go

+	})
+}
+
+func verifyDynamicAttributeByPath(path *tftypes.AttributePath, expectedVal tftypes.Value) func(context.Context, resource.PlanChangeRequest, *resource.PlanChangeResponse) {


❤️ this is really great!

bendbennett · 2024-02-01T15:51:00Z

internal/dynamic6provider/dynamic_nesting_modes_test.go

+				// - https://github.com/hashicorp/terraform-plugin-go/issues/267
+				// - https://github.com/hashicorp/terraform/issues/34574
+				//
+				// In this test, the dynamic `bar` attribute will cause the entire block to be sent over as a DynamicPseudoType, instead of just the bar attribute.


Wondering if it's worth noting that the indication that the entire block is a DynamicPseudoType is provided by the "type annotation" that appears prior to the values. Perhaps this is obvious, and maybe we should document this elsewhere, but it could be contrasted with an example where the DynamicPseudoTypes are constrained to, for instance elements, such as the Test_Dynamic_Attribute_ListType you documented:

{ "attribute_with_dpt": [ [ "\"string\"", "hey" ], [ "\"string\"", "12345" ] ] }

bflad · 2024-04-19T21:44:34Z

@austinvalle do we still need this now that #228 is merged?

austinvalle · 2024-04-22T13:15:02Z

@austinvalle do we still need this now that #228 is merged?

I'm comfortable saying that the important parts of this PR are covered by #228, so I'll close this.

The only tests that aren't covered in #228 are:

Testing import
Testing Terraform core's type conversion
Testing a feature we don't natively support in terraform-plugin-framework (collections w/ dynamic element type)

github-actions · 2024-05-22T19:06:48Z

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.

austinvalle added 30 commits January 19, 2024 10:31

add testsdk and testprovider from plugin-testing

a470dab

add test for dynamic tuple literal

4dbe03a

finished literal tests

f9d4c72

license headers

19980cf

add dynamic6provider tests

6f43a9e

dynamic in workflow

9666b03

implement dynamic state type checking

e312501

update type change in state test

190ffa9

comment out config plan check

972e82a

tf 1.0 issues

366bb5b

add skip for terraform version 1.0

93a3959

skip below

b23bcdc

add comment on why test fails

a35ccd9

add plan/state changing type negative tests

9fdb4b9

doc

8f0ea23

add bug recreation of 267

f54d8ac

add copywrite license

d31232b

add set test showing invalid usage

c6d8c74

rename file

45044a7

update tests and add comments

8afcae8

ignore set validation tests on 1.1.x and 1.0.x

39fd893

change name of file

fd4a3a3

name again

0a4b8ee

fix skips and add tests for types

0f7507e

add compliance header

98dfbbe

rename file

8db9c77

add tests for object types

8e75137

add tuple type example

2639111

add null to new type test

fb62de7

add function tests

39c75a6

austinvalle added 3 commits January 30, 2024 16:53

add import test

3bd7fd9

Merge branch 'main' into av/dpt-testing

80296bd

update to latest branch

10f3778

austinvalle changed the title ~~(WIP) DynamicPseudoType testing~~ (WIP) Add tests for DynamicPseudoType behavior Jan 30, 2024

bflad reviewed Jan 31, 2024

View reviewed changes

austinvalle added 6 commits January 31, 2024 11:43

fix import test

95d52c2

fix test names

612e588

verify the value of literals match as well

0fd1b90

remove dynamic built-in function test

5c36673

add state checks to all successful test cases

9c5c8d0

add multiple elements to list, set, and map tests

beaf6db

austinvalle commented Jan 31, 2024

View reviewed changes

bendbennett reviewed Feb 1, 2024

View reviewed changes

austinvalle mentioned this pull request Feb 27, 2024

all: Add dynamic type, attribute, and function support hashicorp/terraform-plugin-framework#931

Merged

austinvalle closed this Apr 22, 2024

austinvalle deleted the av/dpt-testing branch April 22, 2024 13:15

github-actions bot locked as resolved and limited conversation to collaborators May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(WIP) Add tests for DynamicPseudoType behavior #208

(WIP) Add tests for DynamicPseudoType behavior #208

austinvalle commented Jan 19, 2024 •

edited

Loading

bflad left a comment

bflad Jan 31, 2024

austinvalle Jan 31, 2024 •

edited

Loading

bendbennett Feb 1, 2024

austinvalle Feb 1, 2024

bflad Jan 31, 2024

austinvalle Jan 31, 2024

bflad Jan 31, 2024

austinvalle Jan 31, 2024 •

edited

Loading

austinvalle Jan 31, 2024

bendbennett Feb 1, 2024

bendbennett left a comment

bendbennett Feb 1, 2024

bendbennett Feb 1, 2024

bendbennett Feb 1, 2024

bendbennett Feb 1, 2024

austinvalle Feb 1, 2024 •

edited

Loading

bendbennett Feb 1, 2024

bendbennett Feb 1, 2024

bendbennett Feb 1, 2024

austinvalle Feb 1, 2024

bendbennett Feb 1, 2024

bendbennett Feb 1, 2024

bflad commented Apr 19, 2024

austinvalle commented Apr 22, 2024

github-actions bot commented May 22, 2024

(WIP) Add tests for DynamicPseudoType behavior #208

(WIP) Add tests for DynamicPseudoType behavior #208

Conversation

austinvalle commented Jan 19, 2024 • edited Loading

PR Notes

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvalle Jan 31, 2024 • edited Loading

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 Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett 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

austinvalle Feb 1, 2024 • edited Loading

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

bflad commented Apr 19, 2024

austinvalle commented Apr 22, 2024

github-actions bot commented May 22, 2024

austinvalle commented Jan 19, 2024 •

edited

Loading

austinvalle Jan 31, 2024 •

edited

Loading

austinvalle Jan 31, 2024 •

edited

Loading

austinvalle Feb 1, 2024 •

edited

Loading