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

Unknown tests #2054

Merged
merged 5 commits into from
Jun 5, 2024
Merged

Unknown tests #2054

merged 5 commits into from
Jun 5, 2024

Conversation

VenelinMartinov
Copy link
Contributor

This adds GRPC around unknowns for both attributes and collections for both PlanResourceChange and non-PlanResourceChange

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.48%. Comparing base (b5efa0d) to head (f2a07a8).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2054      +/-   ##
==========================================
+ Coverage   61.36%   61.48%   +0.11%     
==========================================
  Files         334      334              
  Lines       44951    44964      +13     
==========================================
+ Hits        27583    27644      +61     
+ Misses      15841    15796      -45     
+ Partials     1527     1524       -3     

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

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.

Thanks for building this out!

To clarify, do these tests assert on that the current behavior doesn't change, or that the current behavior matches TF's behavior?

I know it's a pain, but it would be really nice to have these tests as a table driven test:

tests := []struct{
	input    resource.PropertyValue
	expected *resource.PropertyValue
}{
	{
		input: resource.NewComputed(resource.Property("")),
		expected: nil,
	},
	...
}

Looking through these tests, it looks like only the property value for input and output has changed, but it's hard to verify that for all of them. The rest makes it harder to understand what's going on. I have no idea if there is a relationship between the "normal" and PlanResource tests. It looks like they are the same tests, but with different responses.

By folding PF and "normal" testing into the same table, we would be able to see clearly where PlanResource diverges from expected, which I can't really tell without manually comparing tests now.

 tests := []struct{
 	input                resource.PropertyValue
 	expected             *resource.PropertyValue
+	expectedPlanResource *resource.PropertyValue
 }{

@VenelinMartinov
Copy link
Contributor Author

Both of these assert on the current behaviour for both plan resource change and non-plan resource change.

I built it out to compare behaviour in #1971 and make sure we aren't regressing.

I'd like to merge as is and leave refactoring for later as #1971 depends on this and it has been hanging for quite a while

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Jun 4, 2024

The structure of the tests it that for each of the property it injects an unknown at every possible level and asserts on the response.

@VenelinMartinov VenelinMartinov requested a review from iwahbe June 4, 2024 17:02
@VenelinMartinov
Copy link
Contributor Author

Comparing PlanResourceChange and non-PlanResourceChange is not the point here

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.

Both of these assert on the current behaviour for both plan resource change and non-plan resource change.

I built it out to compare behaviour in #1971 and make sure we aren't regressing.

Can you add a comment explaining that these are regression tests but that we haven't verified that the behavior matches TF for each case then.

I'd like to merge as is and leave refactoring for later as #1971 depends on this and it has been hanging for quite a while

That makes sense.

The structure of the tests it that for each of the property it injects an unknown at every possible level and asserts on the response.

A comment explaining that would be great.

@VenelinMartinov
Copy link
Contributor Author

GH token rate limits ❤️

@t0yv0
Copy link
Member

t0yv0 commented Jun 4, 2024

@iwahbe we need some plumbing work to do to allow cross-tests to express unknowns in the meanwhile we test at this level. I requested a few additional comments/cross-links but it looks good. It was very important to me to have some edge cases covered here with regression tests and some explanation as to why we think it behaves the way it does.

@t0yv0 t0yv0 self-requested a review June 4, 2024 17:39
@VenelinMartinov
Copy link
Contributor Author

I've done some further digging around the results here in #2032 and https://github.com/pulumi/pulumi-terraform-bridge/pull/2060/files and https://github.com/pulumi/pulumi-terraform-bridge/pull/2060/files

I suspect we can do better with unknowns

@VenelinMartinov VenelinMartinov merged commit a9d0c7c into master Jun 5, 2024
11 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/unknown_tests branch June 5, 2024 14:22
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