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

Full fidelity SDKv2 crosstest.Create equality #2840

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Jan 16, 2025

Supply tfprotov5.ApplyResourceChangeRequest.ProviderMeta when we call ApplyResourceChange in SDKv2 providers. This gets us to a byte for byte identical result for SDKv2 crosstest.Create. This allows us to simplify (and strengthen) the comparison to general equality.

Related to #2521

@iwahbe iwahbe self-assigned this Jan 16, 2025
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 67.48%. Comparing base (8bb538e) to head (b4300c5).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/tfshim/sdk-v2/provider2.go 33.33% 6 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2840      +/-   ##
==========================================
- Coverage   67.49%   67.48%   -0.01%     
==========================================
  Files         325      325              
  Lines       41621    41633      +12     
==========================================
+ Hits        28093    28097       +4     
- Misses      11951    11957       +6     
- Partials     1577     1579       +2     

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

@iwahbe iwahbe force-pushed the iwahbe/full-fidelity-create-cross-tests branch from b38a215 to 34c02db Compare January 17, 2025 12:12
Supply `tfprotov5.ApplyResourceChangeRequest.ProviderMeta` when we call
`ApplyResourceChange` in SDKv2 providers. This gets us to a byte for byte identical result
for SDKv2 `crosstest.Create`. This allows us to simplify (and strengthen) the comparison
to general equality.

Related to #2521
@iwahbe iwahbe force-pushed the iwahbe/full-fidelity-create-cross-tests branch from 34c02db to b4300c5 Compare January 17, 2025 12:28
@iwahbe iwahbe marked this pull request as ready for review January 17, 2025 13:15
@iwahbe iwahbe requested review from VenelinMartinov and a team January 17, 2025 13:15
// `assert`'s default `reflect.DeepEqual` because cmp treats identical
// function pointers as equal, but `reflect.DeepEqual` does not.
opts := []cmp.Option{
cmp.Exporter(func(reflect.Type) bool { return true }),
Copy link
Member Author

Choose a reason for hiding this comment

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

This has cmp compare all un-exported fields.

Comment on lines +130 to +132
cmp.Comparer(func(x, y schema.SchemaStateFunc) bool {
return reflect.ValueOf(x).Pointer() == reflect.ValueOf(y).Pointer()
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

This implements pointer equality for schema.SchemaStateFunc.

Comment on lines +641 to +652
var providerMetaVal []byte
if providerMeta != nil {
providerMetaVal, err = msgpack.Marshal(*providerMeta, providerMeta.Type())
if err != nil {
return nil, err
}
} else {
providerMetaVal, err = msgpack.Marshal(cty.NullVal(cty.EmptyObject), cty.EmptyObject)
if err != nil {
return nil, err
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The previous compare function wasn't catching this, but the new comparison does. This now more accurately imitates TF.

iwahbe added a commit that referenced this pull request Jan 17, 2025
Stacked on top of #2840.

This switches `crosstest.Configure` over to using a stronger comparison function.

Fixes #2521
iwahbe added a commit that referenced this pull request Jan 17, 2025
Stacked on top of #2840.

This switches `crosstest.Configure` over to using a stronger comparison function.

Fixes #2521
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.

lgtm

@iwahbe iwahbe merged commit 7cc82df into master Jan 17, 2025
71 checks passed
@iwahbe iwahbe deleted the iwahbe/full-fidelity-create-cross-tests branch January 17, 2025 15:27
iwahbe added a commit that referenced this pull request Jan 17, 2025
Stacked on top of #2840.

This switches `crosstest.Configure` over to using a stronger comparison function.

Fixes #2521
iwahbe added a commit that referenced this pull request Jan 17, 2025
Stacked on top of #2840.

This switches `crosstest.Configure` over to using a stronger comparison function.

Fixes #2521
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.

2 participants