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

Plugin Framework bridge cross tests #2592

Merged
merged 30 commits into from
Nov 14, 2024
Merged

Plugin Framework bridge cross tests #2592

merged 30 commits into from
Nov 14, 2024

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Nov 5, 2024

This is a test-only PR - it adds cross-tests for Diff for the plugin framework for the existing behaviour.

Each test defines a set of schemas and a set of scenarios and the full matrix of schemas X scenarios is run in cross-tests. For each of these test cases both pulumi and TF are run with the initial input then a preview is run with the second set of inputs and the outcome of the preview is compared.

We also record the stdout for both pulumi and TF for manual comparison of the preview output. The recordings also contain the inputs in order to make spot-checking them easier.

The full test plan is available in #2597

fixes #2297
fixes #2597

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Nov 5, 2024

This change is part of the following stack:

Change managed by git-spice.

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.81%. Comparing base (e0fa71e) to head (6fda624).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2592      +/-   ##
==========================================
+ Coverage   62.79%   62.81%   +0.01%     
==========================================
  Files         389      389              
  Lines       51938    51939       +1     
==========================================
+ Hits        32615    32624       +9     
+ Misses      17515    17506       -9     
- Partials     1808     1809       +1     

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

@VenelinMartinov VenelinMartinov force-pushed the vvm/pf_diff_tests branch 3 times, most recently from 6a336a1 to a98d243 Compare November 6, 2024 13:01
@VenelinMartinov VenelinMartinov force-pushed the vvm/pf_diff_tests branch 3 times, most recently from 20d3be3 to 93314c6 Compare November 6, 2024 16:37
Base automatically changed from vvm/refactor_hclwrite to master November 6, 2024 16:56
@VenelinMartinov VenelinMartinov changed the title PF diff cross tests Plugin Framework bridge cross tests Nov 6, 2024
@VenelinMartinov VenelinMartinov marked this pull request as ready for review November 6, 2024 20:41
@VenelinMartinov
Copy link
Contributor Author

I'll follow-up with issues for the TODOs

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.

This is a massive PR, with 27 commits. Can you give some advice on how to review?

I'm assuming the Pu stdout is collected for humans to read? Are we supposed to read it as part of review? Does it exist only so we notice if we change it? Has it been verified as correct?

pkg/pf/tests/diff_test.go Show resolved Hide resolved
pkg/pf/tests/diff_set_test.go Outdated Show resolved Hide resolved
pkg/pf/tests/diff_set_test.go Show resolved Hide resolved
pkg/pf/tests/diff_set_test.go Show resolved Hide resolved
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.

Should address one comment Ian had on List vs Set, otherwise looks valuable and something we can build on.

@VenelinMartinov VenelinMartinov enabled auto-merge (squash) November 14, 2024 11:25
@VenelinMartinov VenelinMartinov merged commit 317d4b8 into master Nov 14, 2024
17 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/pf_diff_tests branch November 14, 2024 12:07
VenelinMartinov added a commit that referenced this pull request Nov 20, 2024
This change adds additional testing for Diff in PF for sets involving
Computed.

The following schemas are covered:
- Set block with a Computed attribute
- Set block with a Computed attribute and RequiresReplace on the
non-computed attribute
- Set block with a Computed attribute and RequiresReplace on the
computed attribute
- Set block with a Computed attribute and RequiresReplace on the block
- Set block with a Computed attribute and no UseStateForUnknown on the
Computed attribute

For each of these we test both when the Computed attribute is specified
in the program and when it is not.

For each of these schemas we test the full set of scenarios added in
#2592


This should give us some confidence that the changes we are making by
adding Detailed Diff for PF are good for Computed properties.

I will categorize all the issues here as a follow-up before merging
#2629
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v3.96.0.

VenelinMartinov added a commit that referenced this pull request Dec 13, 2024
This is a refactor of the SDKv2 detailed diff tests for sets. Instead of
recording them inline, we now run them as cross-tests and record them in
separate files along with the name of the test and the input values.
This mimics what we did for the PF detailed diff tests in
#2592

I've gone through the recordings and believe all of them are correct.
VenelinMartinov added a commit that referenced this pull request Dec 13, 2024
This is a refactors the rest of the SDKv2 detailed diff tests. Instead
of recording them inline, we now run them as cross-tests and record them
in separate files along with the name of the test and the input values.
This mimics what we did for the PF detailed diff tests in
#2592

I've gone through the recordings and believe all of them are correct.
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.

Plugin Framework bridge Diff test plan Test: TF diff decision and pulumi diff decision match (PF)
4 participants