hashicorp / terraform-provider-google

Terraform Provider for Google Cloud Platform
https://registry.terraform.io/providers/hashicorp/google/latest/docs
Mozilla Public License 2.0
2.33k stars 1.73k forks source link

Document what means that we must skip VCR for a given test #20059

Open SarahFrench opened 1 week ago

SarahFrench commented 1 week ago

What kind of contribution is this issue about?

Other (specify in details)

Details

There are various things we can do to make valid acceptance tests for resources & data sources that are just not compatible with the VCR system currently. In those cases it may be unclear to a contributor why a test works when run locally but is failing in VCR, and some familiarity with the VCR system is needed either on the contributor or reviewer's part to unpick what is happening.

Maybe in some cases the VCR test could be updated to accommodate these specific use cases, but even if that's possible we are unlikely to make large edits to the VCR system soon. For now we should document what things are valid to include in a VCR test but mean that test should 100% be skipped in VCR.

References

wyardley commented 6 days ago

Agree with all this, having dealt with some of these quirks recently. Explaining how the seed is used with random names would be a useful thing to explain - took me a little to see where that actually happens.

Not sure if this would work, but one idea in addition to docs: .gitkeep an empty directory for VCR recordings, and .gitignore its contents, and default the vcr dir to that in the Makefile - usually these files are not too large, I think? At that point, it would be easier for contributors to switch modes, or to have a make target that runs in recording mode, then, upon success, again in replaying… getting it right also could save time and money.

Some public bucket of recordings would also be very useful (for cheaply and quickly running a lot of tests locally to see which ones might need to be checked in recording mode), though I can understand the security risks of doing that (e.g., if a header or token that should be scrubbed isn’t, leaking internal project names, or whatever).

Could there be an option to ignore a missing call? For example, an immutable resource like a KMS keyring being created on the first pass is one reason a test couldn’t use VCR (see #20039 and the PR linked to it)

Maybe instead of a comment above the line skipping the test, SkipVCR() could take the reason for the skip as a parameter?

Side note that’s only partially related, but would be nice if detection of which tests are affected by a PR could be a little better at ignoring unrelated failures, though I guess that could also make it easier to ignore failing tests.

wyardley commented 4 days ago

Would also be nice if the "Get to know how VCR tests work" link in the comments that get posted actually pointed to some information on "how VCR tests work" 😅