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

Add acceptance tests for how provider handles billing_project arguments #11610

Conversation

SarahFrench
Copy link
Contributor

@SarahFrench SarahFrench commented Sep 2, 2024

This PR adds acceptance tests for usage of billing_project that demonstrate:

  • how the provider behaves when provider configuration arguments come from different sources ( config vs ENVs)
  • schema-level validation that's in place, e.g. handling of empty arrays
  • use cases: how does this argument impact the providers behaviour in plan/apply
    • I've tested the use of billing_project + user_project_override=false to show that billing_project has no impact then
    • I've tested the use of billing_project + user_project_override=true to show that billing_project does have impact then

NOTE: I added some usage tests for the PF provider, but they should be replaced in future because being limited to Firebase data sources to test the PF provider makes the test very awkward

Release Note Template for Downstream PRs (will be copied)


@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@SarahFrench SarahFrench force-pushed the mux-refactor-7-replace-billing-project-tests branch from 05841af to e7c2dd8 Compare September 4, 2024 17:47
@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@SarahFrench SarahFrench force-pushed the mux-refactor-7-replace-billing-project-tests branch from 6a74dea to e78cd7b Compare September 5, 2024 11:21
@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@SarahFrench SarahFrench force-pushed the mux-refactor-7-replace-billing-project-tests branch from e78cd7b to 5071cbd Compare September 24, 2024 18:32
@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

1 similar comment
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 2 files changed, 316 insertions(+))
google-beta provider: Diff ( 2 files changed, 316 insertions(+))

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

@modular-magician

This comment has been minimized.

1 similar comment
@modular-magician

This comment was marked as outdated.

@modular-magician

This comment has been minimized.

@modular-magician

This comment was marked as outdated.

@SarahFrench
Copy link
Contributor Author

Hi @NickElliot - I've currently stuck with something in this PR that seems related to the VCR test project, or specifically to tests running on PRs. It's odd!

The TestAccSdkProvider_billing_project test that's failing here is failing because the test expects errors but doesn't experience any.

However there should be an error, as the test is structured to cause one. When the SDK test is run in TeamCity (using the VCR project) the test passes as expected.

Any idea what could be causing this? I'm stuck... the solution could just be to skip in VCR?

Copy link

github-actions bot commented Oct 7, 2024

@NickElliot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link

github-actions bot commented Oct 9, 2024

@GoogleCloudPlatform/terraform-team @NickElliot This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link

@GoogleCloudPlatform/terraform-team @NickElliot This PR has been waiting for review for 2 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

@SarahFrench
Copy link
Contributor Author

SarahFrench commented Oct 17, 2024

@NickElliot , could you please take a look at this PR? If you're at capacity with work I can reassign, just let me know!

Similar for #11686

@SarahFrench SarahFrench requested review from c2thorn and removed request for NickElliot October 22, 2024 17:10
@SarahFrench
Copy link
Contributor Author

Hey @c2thorn, here's the PR where I need a bit of help figuring out why tests behave differently when I run them locally vs on this PR. There's a message summarising the issue here : #11610 (comment)

Copy link
Member

@c2thorn c2thorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tentative lgtm depending on how you want to address https://github.com/GoogleCloudPlatform/magic-modules/pull/11610/files#r1811341541

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 2 files changed, 503 insertions(+))
google-beta provider: Diff ( 2 files changed, 635 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 4209
Passed tests: 3786
Skipped tests: 420
Affected tests: 3

Click here to see the affected service packages

All service packages are affected

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccBackupDRBackupVault_fullUpdate
  • TestAccSdkProvider_billing_project
  • TestAccSqlDatabaseInstance_Edition_Downgrade

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccBackupDRBackupVault_fullUpdate [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🔴 Tests failed during RECORDING mode:
TestAccSdkProvider_billing_project [Error message] [Debug log]
TestAccSqlDatabaseInstance_Edition_Downgrade [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

@SarahFrench
Copy link
Contributor Author

SarahFrench commented Oct 23, 2024

provider_billing_project_test.go:200: Step 2/3, expected an error but got none

I'd missed that 2 tests are failing in TestAccSdkProvider_billing_project. I'll confirm that it passes ok in the nightlies and then make it skip in VCR too. Edit: confirmed.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 2 files changed, 507 insertions(+))
google-beta provider: Diff ( 2 files changed, 639 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 4209
Passed tests: 3788
Skipped tests: 420
Affected tests: 1

Click here to see the affected service packages

All service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccSqlDatabaseInstance_Edition_Downgrade

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🔴 Tests failed during RECORDING mode:
TestAccSqlDatabaseInstance_Edition_Downgrade [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

@SarahFrench
Copy link
Contributor Author

Going ahead with merge!

@SarahFrench
Copy link
Contributor Author

@SarahFrench
Copy link
Contributor Author

For completeness, noting here:

The tests failed in VCR due to hashicorp/terraform-provider-google#20019
When the test runs in VCR mode the unaliased provider is the first to be configured, so its Config struct is cached. Later when the aliased provider is used in the test the provider arguments are effectively ignored because the aliased provider pulls in the Config of the un-aliased provider (which was configured with different arguments).

This is why the test passes when run outside of VCR mode and failed in VCR mode.

The failure identified here is just bad luck and unrelated to the VCR-related confusion!

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