Skip to content

Conversation

@guineveresaenger
Copy link
Contributor

@guineveresaenger guineveresaenger commented Apr 7, 2025

This pull request fixes up TestTranslateCodeBlocks:

  • Provides a fully assigned Generator with a mock test host to the test case. This:
    • prevents the underlying code from attempting to re-create a Generator for each Convert call, resulting in very quick rate limiting
    • additionally prevents the automatic downloading of the terraform converter plugin
  • Ensures the test provider names in the test files match the test provider name in the test (simple).
  • Removes the required_providers section as unnecessary

As a result of pulumi/pulumi-converter-terraform#319, which is now in use by the bridge, we also no longer download the terraform-provider for a nonexisting provider.

Fixes #2974

@codecov
Copy link

codecov bot commented Apr 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.48%. Comparing base (c7b8ac4) to head (6494399).
Report is 32 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2981      +/-   ##
==========================================
+ Coverage   68.30%   68.48%   +0.18%     
==========================================
  Files         330      334       +4     
  Lines       42108    43212    +1104     
==========================================
+ Hits        28761    29593     +832     
- Misses      11766    11963     +197     
- Partials     1581     1656      +75     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@guineveresaenger guineveresaenger requested a review from a team April 7, 2025 23:15
@t0yv0
Copy link
Member

t0yv0 commented Apr 11, 2025

Feel free to check this in but this does not fix 2974 as in principle we still have test isolation issues that can surface is that correct?

@guineveresaenger
Copy link
Contributor Author

You're right, this PR is mainly a response to changes in the converter, which add additional network calls when encountering a required_provider section, which this test doesn't need.

Going to add my thoughts on #2974 over there - had a few thoughts.

@t0yv0
Copy link
Member

t0yv0 commented Apr 11, 2025

Another source of confusion and problem in this test case is

Configure the OpenStack Provider
provider "simple-provider" {
  user_name   = "admin"
  tenant_name = "admin"
  password    = "pwd"
  auth_url    = "http://myauthurl:5000/v3"
  region      = "RegionOne"
}

Should be probably this:

provider "simple"

As it thinks provider-simple is very different from the one needed for simple_ resources.

@guineveresaenger guineveresaenger changed the title Do not add required_providers to installation docs test file Improve test isolation for TestTranslateCodeBlocks Apr 24, 2025
@guineveresaenger guineveresaenger requested a review from t0yv0 April 24, 2025 23:59
@t0yv0
Copy link
Member

t0yv0 commented Apr 28, 2025

It would be great to understand what this fixes, and possibly take this as an improvement but pulumi/pulumi-converter-terraform#319 remains a problem as of this morning:

anton@anton-mbp-m3> pulumi plugin ls                                                                                                     ~/code/pulumi-terraform-bridge
NAME       KIND       VERSION  SIZE   INSTALLED      LAST USED
terraform  converter  1.1.0    40 MB  8 seconds ago  8 seconds ago

The test is not isolated as it auto-installs the converter at latest version.

Am I perhaps running this wrong?

@guineveresaenger
Copy link
Contributor Author

Hm 🤔 - this test should now be exactly as isolated as TestConvertViaPulumiCLI. I thought this meant the only way it would install the converter was by going through the converter install in Makefile but it appears I was wrong.

I'm not sure that it is a significant problem anymore though - the test will not attempt to install the latest converter, and instead abide by the converter version specified in Make. So if tests are run via Make, which is what CI uses, this should be OK.

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.

If it's isolated in CI that's probably good enough for now.. Sounds like the installation of a concrete version is pinned in Makefile. I'd like to fix someday so that the tests could be invoked directly through Go w/o Makefile.

@t0yv0 t0yv0 merged commit fa76324 into master May 1, 2025
75 checks passed
@t0yv0 t0yv0 deleted the guin/reduce-scope-test-translate-code-blocks branch May 1, 2025 14:45
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.

testing: TestTranslateCodeBlocks insufficiently isolated

2 participants