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

Rapid generator for schema-value pairs #1801

Merged
merged 45 commits into from
Apr 12, 2024
Merged

Rapid generator for schema-value pairs #1801

merged 45 commits into from
Apr 12, 2024

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Mar 28, 2024

Fixes #1790 by building a rapid generator for schemas and associated values.

Large-ish problem 1: I do not have it figured out how to test unknown values. TF literals as unknown values are forbidden and do not make sense. We might need a helper resource so that testing unknown values generates references to an output of the helper resource. This is logged for future work.

Large-ish problem 2: iteration is pretty slow (x-proc). Normal n=100 rapid tests can take up to 10min. Could try batching so several resources are tried in one shot say 100 resources.

Large-ish problem 3: I'm not sure if no-op Update and Create implementations are acceptable. There is something to testing Computed attributes where provider has to set values. Possibly Update also needs to set values? Possibly not.

Small problems:

  • Using TF JSON syntax didn't handle null/empty correctly; that is now discarded, using actual HCL syntax
  • TF representations are difficult to visualize in failing tests and difficult to assert against
  • Lots of lost-in-translation papercuts possible between representations (cty.Value, resource.PropertyValue, tftypes.Value)
  • this requires a change to providertest to abstract from testing.T so we can pass rapid.T
  • it's very hard to disable annoying TF logging, using env vars for now

We are starting to find bugs and discrepancies from this work:

Future work:

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 28, 2024

@VenelinMartinov @iwahbe

@iwahbe
Copy link
Member

iwahbe commented Mar 28, 2024

Building #1790

There's a large number of problems, both small and large, so it's not really finding any real bugs yet.

Large-ish problem 1: I do not have it figured out how to test unknown values. TF literals as unknown values are forbidden and do not make sense. We might need a helper resource so that testing unknown values generates references to an output of the helper resource.

I'm assuming that this isn't relevant:

// makeTerraformUnknown creates an unknown value with the shape indicated by the given schema.
//
// It is important that we use the TF schema (if available) to decide what shape the unknown value should have:
// e.g. TF does not play nicely with unknown lists, instead expecting a list of unknowns.
func makeTerraformUnknown(tfs shim.Schema) interface{} {
if tfs == nil {
return TerraformUnknownVariableValue
}
switch tfs.Type() {
case shim.TypeList, shim.TypeSet:
// TF does not accept unknown lists or sets. Instead, it accepts lists or sets of unknowns.
count := 1
if tfs.MinItems() > 0 {
count = tfs.MinItems()
}
arr := make([]interface{}, count)
for i := range arr {
arr[i] = makeTerraformUnknownElement(tfs.Elem())
}
return arr
default:
return TerraformUnknownVariableValue
}
}

Large-ish problem 2: iteration is pretty slow (x-proc). Normal n=100 rapid tests can take up to 10min. Could try batching so several resources are tried in one shot say 100 resources.

That's painful. Instead of batching, are we missing calls to t.Parallel()? In theory, Go should handle batching for us. Do we know what the bottleneck is. 100 tests in 10min indicate 6 seconds per test, which seems high.

Large-ish problem 3: I'm not sure if no-op Update and Create implementations are acceptable. There is something to testing Computed attributes where provider has to set values. Possibly Update also needs to set values? Possibly not.

This seems like a good problem to address in the next PR.

@t0yv0
Copy link
Member Author

t0yv0 commented Apr 2, 2024

6 seconds per test is about right, why would you expect it to be faster? it's shelling out to tf and pulumi CLI and setting up folders and such.

Looks like rapid does not support parallel exploration of the space. At least I didn't find it. We'd definitely want to batch evaluations over a map of resource instances and shrink it to the failing one.

I've got some simple diff x-tests passing but still iterating to add more of different kinds of generators. Already yielding first very interesting bugs in the tracker.

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Attention: Patch coverage is 48.04805% with 519 lines in your changes are missing coverage. Please review.

Project coverage is 60.18%. Comparing base (8222ffd) to head (8c9933b).
Report is 5 commits behind head on master.

Files Patch % Lines
pkg/tests/cross-tests/rapid_tv_gen.go 0.00% 304 Missing ⚠️
pkg/tests/cross-tests/adapt.go 44.00% 80 Missing and 4 partials ⚠️
pkg/tests/cross-tests/pu_driver.go 58.03% 40 Missing and 7 partials ⚠️
pkg/tests/cross-tests/pretty.go 76.36% 38 Missing and 1 partial ⚠️
pkg/tests/cross-tests/tf_driver.go 82.85% 23 Missing and 1 partial ⚠️
pkg/tests/cross-tests/diff_check.go 81.15% 13 Missing ⚠️
pkg/tests/cross-tests/tfwrite.go 85.00% 5 Missing and 1 partial ⚠️
pkg/tfshim/sdk-v2/cty.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1801      +/-   ##
==========================================
- Coverage   61.04%   60.18%   -0.87%     
==========================================
  Files         303      319      +16     
  Lines       42442    43832    +1390     
==========================================
+ Hits        25910    26380     +470     
- Misses      15061    15964     +903     
- Partials     1471     1488      +17     

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

Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this!


rapid.Check(outerT, func(t *rapid.T) {
outerT.Logf("Iterating..")
tv := tvg.GenBlock(3).Draw(t, "tv")
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking if we should start with depth 1 as potentially easier to fix and then expand from there. Looks like most of the failures so far are at depth >=2.

Would that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, the depth shouldn't be fixed but should enable shrinking. I'll look into it.

Copy link
Member

Choose a reason for hiding this comment

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

I've had success with rapid by interpreting the 3 as a depth cap (but not an explicit depth), setting the depth cap to 5 and then letting rapid optimize from there.

pkg/tests/cross-tests/rapid_tv_gen.go Outdated Show resolved Hide resolved
pkg/tests/cross-tests/tf_driver.go Outdated Show resolved Hide resolved
pkg/tests/cross-tests/diff_check.go Outdated Show resolved Hide resolved
pkg/tests/cross-tests/tfwrite.go Outdated Show resolved Hide resolved
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

I'd love to check this in with an example of how to use the rapid generator. I want to make sure people can discover how to use these.

pkg/tfshim/sdk-v2/cty.go Outdated Show resolved Hide resolved
pkg/tests/cross-tests/diff_check.go Outdated Show resolved Hide resolved
pkg/tests/cross-tests/pu_driver.go Outdated Show resolved Hide resolved

rapid.Check(outerT, func(t *rapid.T) {
outerT.Logf("Iterating..")
tv := tvg.GenBlock(3).Draw(t, "tv")
Copy link
Member

Choose a reason for hiding this comment

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

I've had success with rapid by interpreting the 3 as a depth cap (but not an explicit depth), setting the depth cap to 5 and then letting rapid optimize from there.

pkg/tests/cross-tests/rapid_tv_gen.go Outdated Show resolved Hide resolved
pkg/tests/cross-tests/rapid_tv_gen.go Outdated Show resolved Hide resolved
pkg/tests/cross-tests/rapid_tv_gen.go Show resolved Hide resolved
@t0yv0
Copy link
Member Author

t0yv0 commented Apr 12, 2024

@iwahbe the example is https://github.com/pulumi/pulumi-terraform-bridge/blob/t0yv0%2Ftv-generators/pkg/tests/cross-tests/rapid_test.go#L30 although it does not quite yet pass. Once you have a test that you can parameterize by "any schema and value of that schema" you can hook it up to rapid to start tearing through.

@t0yv0 t0yv0 enabled auto-merge (squash) April 12, 2024 18:49
@t0yv0 t0yv0 merged commit 7aa41da into master Apr 12, 2024
11 checks passed
@t0yv0 t0yv0 deleted the t0yv0/tv-generators branch April 12, 2024 19:20
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.

Build a rapid Generator for SDKV2 Schema/Value pairs
3 participants