Skip to content

[Major Release 5.0.0] Update test to reflect empty string validation on provider config added in 5.0.0#9120

Merged
SarahFrench merged 1 commit into
mainfrom
remove-outdated-test
Oct 2, 2023
Merged

[Major Release 5.0.0] Update test to reflect empty string validation on provider config added in 5.0.0#9120
SarahFrench merged 1 commit into
mainfrom
remove-outdated-test

Conversation

@SarahFrench

@SarahFrench SarahFrench commented Sep 28, 2023

Copy link
Copy Markdown
Contributor

Part of hashicorp/terraform-provider-google#16035

This PR removes an acceptance test that's outdated in 5.0.0 due to the addition of validation on provider configuration values.

Release Note Template for Downstream PRs (will be copied)


@SarahFrench SarahFrench changed the title Remove test superseded by empty string validation on provider [Major Release 5.0.0] Remove test superseded by empty string validation on provider Sep 28, 2023
@modular-magician

Copy link
Copy Markdown
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.

Terraform GA: Diff ( 1 file changed, 43 deletions(-))
Terraform Beta: Diff ( 1 file changed, 43 deletions(-))

@modular-magician

Copy link
Copy Markdown
Collaborator

Tests analytics

Total tests: 3098
Passed tests 2784
Skipped tests: 312
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccBigQueryDataTable_bigtable|TestAccContainerCluster_withAddons

Get to know how VCR tests work

@shuyama1 shuyama1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just want to make sure, this test can/ should be removed due to the change in #9014?

@modular-magician

Copy link
Copy Markdown
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryDataTable_bigtable[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerCluster_withAddons[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@SarahFrench

Copy link
Copy Markdown
Contributor Author

Just want to make sure, this test can/ should be removed due to the change in #9014?

I realised that I can update the test to still work - I thought that the the validation added in #9050 made this test not work anymore, but I can update the test to be plan only and that allows ExpectError to handle errors coming from provider config validation again.

So this PR is now to update the test, not remove it. Sorry for being a bit chaotic!

@SarahFrench SarahFrench changed the title [Major Release 5.0.0] Remove test superseded by empty string validation on provider [Major Release 5.0.0] Update test to reflect empty string validation on provider config added in 5.0.0 Sep 29, 2023
@modular-magician

Copy link
Copy Markdown
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.

Terraform GA: Diff ( 1 file changed, 28 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 28 insertions(+), 2 deletions(-))

@modular-magician

Copy link
Copy Markdown
Collaborator

Tests analytics

Total tests: 3104
Passed tests 2787
Skipped tests: 314
Affected tests: 3

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
TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withKubeletConfig|TestAccContainerCluster_withAddons

Get to know how VCR tests work

@modular-magician

Copy link
Copy Markdown
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerNodePool_withUpgradeSettings[Error message] [Debug log]
TestAccContainerNodePool_withKubeletConfig[Error message] [Debug log]
TestAccContainerCluster_withAddons[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@shuyama1

shuyama1 commented Sep 29, 2023

Copy link
Copy Markdown
Member

Just want to make sure, this test can/ should be removed due to the change in #9014?

I realised that I can update the test to still work - I thought that the the validation added in #9050 made this test not work anymore, but I can update the test to be plan only and that allows ExpectError to handle errors coming from provider config validation again.

So this PR is now to update the test, not remove it. Sorry for being a bit chaotic!

No worries. Thanks for updating the test. The tests are skipped in VCR. Are you able to get them passed locally or kick off an upstream run on TC?

@shuyama1 shuyama1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change looks good. Assuming tests passed locally.

@SarahFrench

Copy link
Copy Markdown
Contributor Author

Upstream run on TC shows the updated test passes - merging now

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