Skip to content
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

Expose crosstests.Create #2501

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Expose crosstests.Create #2501

merged 1 commit into from
Oct 18, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Oct 18, 2024

Stacked PRs:


Expose crosstests.Create

This begins to turn cross-tests into a library with a defined interface. Existing
cross-tests say that they are a prototype level API, and they feel that way. The
prototyping was successful, and cross-tests have proven their worth. We should begin to
move towards a more discover-able API. The first function of which is:

// crosstests/create.go

func Create(
	t T, resource map[string]*schema.Schema, tfConfig cty.Value, puConfig resource.PropertyMap,
	options ...CreateOption,
)

We can continue to iterate on this API, but we should begin to formalize a boundary
between the implementation of cross-tests and tests written with cross-tests.

This begins to turn cross-tests into a library with a defined interface. Existing
cross-tests say that they are a prototype level API, and they feel that way. The
prototyping was successful, and cross-tests have proven their worth. We should begin to
move towards a more discover-able API. The first function of which is:

```go
// crosstests/create.go

func Create(
	t T, resource map[string]*schema.Schema, tfConfig cty.Value, puConfig resource.PropertyMap,
	options ...CreateOption,
)
```

We can continue to iterate on this API, but we should begin to formalize a boundary
between the implementation of cross-tests and tests written with cross-tests.

stack-info: PR: #2501, branch: iwahbe/stack/2
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 4.47761% with 64 lines in your changes missing coverage. Please review.

Project coverage is 62.50%. Comparing base (23d65d6) to head (563065f).

Files with missing lines Patch % Lines
pkg/internal/tests/cross-tests/create.go 0.00% 62 Missing ⚠️
pkg/tests/pulcheck/pulcheck.go 60.00% 2 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##           iwahbe/stack/1    #2501      +/-   ##
==================================================
- Coverage           62.58%   62.50%   -0.08%     
==================================================
  Files                 380      381       +1     
  Lines               51270    51335      +65     
==================================================
+ Hits                32087    32088       +1     
- Misses              17368    17432      +64     
  Partials             1815     1815              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iwahbe iwahbe requested review from VenelinMartinov and a team October 18, 2024 11:30
@iwahbe iwahbe self-assigned this Oct 18, 2024
@iwahbe iwahbe changed the base branch from iwahbe/stack/1 to master October 18, 2024 12:20
@iwahbe iwahbe changed the base branch from master to iwahbe/stack/1 October 18, 2024 12:20
@iwahbe iwahbe changed the base branch from iwahbe/stack/1 to master October 18, 2024 12:21
@iwahbe iwahbe changed the base branch from master to iwahbe/stack/1 October 18, 2024 12:22
//
// Create *does not* verify the outputs of the resource, only that the provider witnessed the same inputs.
func Create(
t T, resource map[string]*schema.Schema, tfConfig cty.Value, puConfig resource.PropertyMap,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to expose both the pulumi and TF inputs in the test framework? I don't think it is the common use case for these to differ - should we instead aim to make it simpler to use this by having one input and handle the conversion in the framework, as before?

I don't think we have that many use cases for the inputs differing outside of type conversions as it this defeats the purpose of the test

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in #2500 (comment):

We need to test:

Another example: If you want to verify that set order doesn't matter, you would need to do something like:

for _, setPermutation := range permutationsOf(setValues) {
	crosstests.Create(t, schema, tfConfig, resource.PropertyMap{
		"set": resource.NewArrayProperty(setPermutation),
	})
}

I don't think you could reliably do that in tftypes.Value space.

From Create's API, it would be very easy to build in automatic pulumi value inference:

crosstests.Create(t, schema, tfConfig, crosstests.InferValue)

Just as easily, we could make accepting the Pulumi value an option, defaulting to inference:

crosstests.Create(t, schema, tfConfig) // Pulumi value is inferred
crosstests.Create(t, schema, tfConfig, // Pulumi value is explicit
	crosstests.CreatePulumiValue(explicitPulumiValue))

As long as the internals support explicit values (#2500), it's easy to convert to an implicit value. From there, there are many ways we can support implicit values.

iwahbe added a commit that referenced this pull request Oct 18, 2024
This begins to turn cross-tests into a library with a defined interface. Existing
cross-tests say that they are a prototype level API, and they feel that way. The
prototyping was successful, and cross-tests have proven their worth. We should begin to
move towards a more discover-able API. The first function of which is:

```go
// crosstests/create.go

func Create(
	t T, resource map[string]*schema.Schema, tfConfig cty.Value, puConfig resource.PropertyMap,
	options ...CreateOption,
)
```

We can continue to iterate on this API, but we should begin to formalize a boundary
between the implementation of cross-tests and tests written with cross-tests.

stack-info: PR: #2501, branch: iwahbe/stack/2
@iwahbe iwahbe changed the base branch from iwahbe/stack/1 to master October 18, 2024 16:29
@iwahbe iwahbe force-pushed the iwahbe/stack/2 branch 2 times, most recently from 1c635d9 to 06c8af7 Compare October 18, 2024 16:29
@iwahbe iwahbe changed the base branch from master to iwahbe/stack/1 October 18, 2024 16:29
iwahbe added a commit that referenced this pull request Oct 18, 2024
This begins to turn cross-tests into a library with a defined interface. Existing
cross-tests say that they are a prototype level API, and they feel that way. The
prototyping was successful, and cross-tests have proven their worth. We should begin to
move towards a more discover-able API. The first function of which is:

```go
// crosstests/create.go

func Create(
	t T, resource map[string]*schema.Schema, tfConfig cty.Value, puConfig resource.PropertyMap,
	options ...CreateOption,
)
```

We can continue to iterate on this API, but we should begin to formalize a boundary
between the implementation of cross-tests and tests written with cross-tests.

stack-info: PR: #2501, branch: iwahbe/stack/2
@iwahbe iwahbe changed the base branch from iwahbe/stack/1 to master October 18, 2024 16:38
@iwahbe iwahbe changed the base branch from master to iwahbe/stack/1 October 18, 2024 16:38
@t0yv0
Copy link
Member

t0yv0 commented Oct 18, 2024

This is reasonable, thanks:

func Create(
	t T, resource map[string]*schema.Schema, tfConfig cty.Value, puConfig resource.PropertyMap,
	options ...CreateOption,
)

Might need a way to set overrides (info.Schema aka SchemaInfo).

As Venelin points out, some tests may want to omit specifying both tfConfig and puConfig if one can be unambiguously computed from the other.

}

// An option that can be used to customize [Create].
type CreateOption func(*createOpts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For internal code I wonder sometimes if plain struct options are just easier to work with and discover than functional options. That's a stylistic nit though. It's all good.

Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks like it's getting a little nicer. Always welcome.

iwahbe added a commit that referenced this pull request Oct 18, 2024
This begins to turn cross-tests into a library with a defined interface. Existing
cross-tests say that they are a prototype level API, and they feel that way. The
prototyping was successful, and cross-tests have proven their worth. We should begin to
move towards a more discover-able API. The first function of which is:

```go
// crosstests/create.go

func Create(
	t T, resource map[string]*schema.Schema, tfConfig cty.Value, puConfig resource.PropertyMap,
	options ...CreateOption,
)
```

We can continue to iterate on this API, but we should begin to formalize a boundary
between the implementation of cross-tests and tests written with cross-tests.

stack-info: PR: #2501, branch: iwahbe/stack/2
@iwahbe iwahbe changed the base branch from iwahbe/stack/1 to master October 18, 2024 17:16
@iwahbe iwahbe changed the base branch from master to iwahbe/stack/1 October 18, 2024 17:16
Base automatically changed from iwahbe/stack/1 to master October 18, 2024 18:49
@iwahbe iwahbe enabled auto-merge (rebase) October 18, 2024 18:49
@iwahbe iwahbe disabled auto-merge October 18, 2024 19:01
@iwahbe iwahbe enabled auto-merge (rebase) October 18, 2024 19:01
@iwahbe iwahbe disabled auto-merge October 18, 2024 19:14
@iwahbe iwahbe merged commit 0e6ed77 into master Oct 18, 2024
17 checks passed
@iwahbe iwahbe deleted the iwahbe/stack/2 branch October 18, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants