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

PF extract inputs from outputs tests #2225

Merged
merged 7 commits into from
Jul 23, 2024

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Jul 22, 2024

This adds adds missing test coverage for ExtractInputsFromOutputs for various PF schemas. ExtractInputsFromOutputs is a shared function so we should have coverage on both sides to make sure we don't cause regressions when fixing one.

Similar to #2224 but on the PF side.

This is in preparation for #2181 for #2180.

Seeing some more evidence of #2218 here.

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.65%. Comparing base (7ccdd03) to head (4427b72).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2225      +/-   ##
==========================================
- Coverage   60.66%   60.65%   -0.01%     
==========================================
  Files         356      356              
  Lines       46447    46447              
==========================================
- Hits        28176    28174       -2     
- Misses      16711    16712       +1     
- Partials     1560     1561       +1     

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

Copy link
Contributor

@guineveresaenger guineveresaenger 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 really good to have, and a nice springboard for doing #2218 right!

Can the individual test names describe the expected behavior under test? e.g. StringWithDefaultExtractsToPropertyAsString` or whatever we're asserting. 🙏

pf/tests/extract_inputs_test.go Show resolved Hide resolved
@VenelinMartinov VenelinMartinov requested a review from a team July 23, 2024 08:44
@VenelinMartinov VenelinMartinov changed the base branch from master to vvm/add_pf_shim_tests1 July 23, 2024 10:27
@VenelinMartinov VenelinMartinov changed the base branch from vvm/add_pf_shim_tests1 to master July 23, 2024 10:27
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 adding test coverage before addressing #2218.

@VenelinMartinov VenelinMartinov merged commit bf90025 into master Jul 23, 2024
11 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/pf_extract_inputs_from_outputs branch July 23, 2024 16:01
VenelinMartinov added a commit that referenced this pull request Jul 23, 2024
Fixes an issue with the SDKv2 and PF input extraction. During import we
wrongly return values for properties which are fully computed which
causes invalid programs to be generated.

~The object check in `CastToTypeObject` was wrong for SDKV2 objects:
https://github.com/pulumi/pulumi-terraform-bridge/blob/48fd41bd16e2f7f933bb9390ab7619d52faabb6d/pkg/tfshim/util/types.go#L36~

The `CastToTypeObject` object check is wrong for both SDKv2 and PF. For
the SDKV2 only sets and lists can have a `Resource` `.Elem`. For PF it
list-nested blocks are represented with a `Resource` `.Elem` in the shim
layer, so the check did not catch these. PF details here:
#2231
This fixes the check and adds regression tests for the broken imports
which resulted from 2180.

Test coverage added in
#2224 for sdkv2
and #2225 for pf.
fixes #2180
depends on pulumi/providertest#91
stacked on #2225
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v3.88.0.

@mjeffryes mjeffryes added this to the 0.108 milestone Aug 16, 2024
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.

7 participants