test: Fixes tests after updating terraform-plugin-testing from 1.5.1 to 1.6.0 #1792

AgustinBettati · 2023-12-27T10:34:04Z

Description

Link to any related issue(s): CLOUDP-219141

This PR uses the dependabot PR as the base branch which is updating terraform-plugin-testing from 1.5.1 to 1.6.0.
Due to some changes introduced in this new version some of our tests where impacted (seen in the following CI execution that was triggered manually):

(caused by 1) TestAccClusterRSCluster_withDefaultBiConnectorAndAdvancedConfiguration_maintainsBackwardCompatibility
(caused by 2) TestAccFederatedDatabaseInstance_basic
(caused by 2) TestAccConfigRSCustomDBRoles_importBasic

The failing test are due to 2 causes.

1. Changes in the behaviour of `PlanOnly`

I raised an issue to HashiCorp with the details of the failing test and guidance was provided: hashicorp/terraform-plugin-testing#256.
In summary, the failing case defines a migration test from an old provider version to the latest, and during these version one attribute is removed from the schema. After terraform-plugin-testing v1.6.0 when using PlanOnly the state is not automatically updated with the current provider schema before the destroy begins, leading to the error.
The suggested way moving forward is to avoid using PlanOnly: true, and instead use plancheck.ExpectEmptyPlan(). This is only needed when the underlying provider schema changes between TestSteps.

2. Changes in the behaviour of `ImportState`

Examples of the failing test:

--- FAIL: TestAccConfigRSCustomDBRoles_importBasic (18.63s)
    /Users/agustin.bettati/mongodb/terraform-provider-mongodbatlas/internal/service/customdbrole/resource_custom_db_role_test.go:442: Step 2/3 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.
        
          map[string]string{
        + 	"actions.0.resources.0.cluster": "false",
          }

As seen in the message, after terraform-plugin-testing v1.6.0 the ImportStateVerify: true, config verifies if there are attributes that are present in the state, but not in the config. Before this version this check only verified missing attributes in the state. I verified that in both cases these additional attributes do not cause a non-empty plan (and also added in test cases), and add these additional attribute in the ImportStateVerifyIgnore array.

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contribution guidelines
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.

Further comments

Bumps [github.com/hashicorp/terraform-plugin-testing](https://github.com/hashicorp/terraform-plugin-testing) from 1.5.1 to 1.6.0. - [Release notes](https://github.com/hashicorp/terraform-plugin-testing/releases) - [Changelog](https://github.com/hashicorp/terraform-plugin-testing/blob/main/CHANGELOG.md) - [Commits](hashicorp/terraform-plugin-testing@v1.5.1...v1.6.0) --- updated-dependencies: - dependency-name: github.com/hashicorp/terraform-plugin-testing dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

…ate and not in the config

…rror during execution of test

AgustinBettati · 2023-12-27T12:49:05Z

failing test in cluster group (TestAccClusterRSCluster_tenant) is not related, will followup with a CLOUDP ticket for that issue as I am also seeing it in QA executions (flaky scenario). Also ran all acceptance tests/migrations test to verify no other failing test was left out after the fix.

andreaangiolillo

LGTM

andreaangiolillo · 2023-12-28T10:23:50Z

internal/service/cluster/resource_cluster_test.go

 					},
 				},
-				PlanOnly: true,


Should we do a quick search/replace of PlanOnly in the repo? 👀

sorry I did not understand this point

just updated the message. I am wondering if we should search for PlanOnly in the repo and remove it

yeah, was waiting for confirmation here as the migration test documentation still mentions PlanOnly usage. Can do this in a followup PR.

andreaangiolillo · 2023-12-28T10:25:02Z

internal/service/cluster/resource_cluster_test.go

 					},
 				},
-				PlanOnly: true,


[not-blocking] Should we document these errors somewhere so that we do not forget them in the future? I am not sure about an ideal location 🤔 leave this here to think about it

Our terraform wiki can be good starting point, helpful for referencing in the future if it is encountered again.

maastha · 2023-12-28T11:52:02Z

internal/service/cluster/resource_cluster_test.go

@@ -1451,9 +1451,9 @@ func TestAccClusterRSCluster_withDefaultBiConnectorAndAdvancedConfiguration_main
 				ConfigPlanChecks: resource.ConfigPlanChecks{
 					PostApplyPreRefresh: []plancheck.PlanCheck{
 						acc.DebugPlan(),


do we still need this as well?

thanks for noting, not necessary as ExpectEmptyPlan() gives details of the plan if it is non empty.

…ptyPlan check

dependabot bot and others added 3 commits December 21, 2023 16:32

fix import test cases to ignore computed attributes present in the st…

dbf0069

…ate and not in the config

use plancheck.ExpectEmptyPlan() over PlanOnly in test step to avoid e…

ffe1e8b

…rror during execution of test

AgustinBettati changed the base branch from master to dependabot/go_modules/github.com/hashicorp/terraform-plugin-testing-1.6.0 December 27, 2023 10:34

AgustinBettati changed the title ~~test: make adjustment in tests after updating terraform-plugin-testing from 1.5.1 to 1.6.0~~ test: Fixes tests after updating terraform-plugin-testing from 1.5.1 to 1.6.0 Dec 27, 2023

ensure empty plan after import has been made

be7c4b2

AgustinBettati force-pushed the CLOUDP-219141 branch from 5cfcbf4 to be7c4b2 Compare December 27, 2023 11:38

andreaangiolillo marked this pull request as ready for review December 28, 2023 10:23

andreaangiolillo requested a review from a team as a code owner December 28, 2023 10:23

andreaangiolillo approved these changes Dec 28, 2023

View reviewed changes

maastha reviewed Dec 28, 2023

View reviewed changes

remove redundant DebugPlan as non empty plan is displayed by ExpectEm…

a6d691b

…ptyPlan check

AgustinBettati changed the base branch from dependabot/go_modules/github.com/hashicorp/terraform-plugin-testing-1.6.0 to master December 28, 2023 13:36

AgustinBettati merged commit 4203246 into master Dec 28, 2023
33 checks passed

AgustinBettati deleted the CLOUDP-219141 branch December 28, 2023 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Fixes tests after updating terraform-plugin-testing from 1.5.1 to 1.6.0 #1792

test: Fixes tests after updating terraform-plugin-testing from 1.5.1 to 1.6.0 #1792

AgustinBettati commented Dec 27, 2023 •

edited

AgustinBettati commented Dec 27, 2023 •

edited

andreaangiolillo left a comment

andreaangiolillo Dec 28, 2023 •

edited

AgustinBettati Dec 28, 2023

andreaangiolillo Dec 28, 2023

AgustinBettati Dec 28, 2023

andreaangiolillo Dec 28, 2023

AgustinBettati Dec 28, 2023

maastha Dec 28, 2023

AgustinBettati Dec 28, 2023

test: Fixes tests after updating terraform-plugin-testing from 1.5.1 to 1.6.0 #1792

test: Fixes tests after updating terraform-plugin-testing from 1.5.1 to 1.6.0 #1792

Conversation

AgustinBettati commented Dec 27, 2023 • edited

Description

1. Changes in the behaviour of PlanOnly

2. Changes in the behaviour of ImportState

Type of change:

Required Checklist:

Further comments

AgustinBettati commented Dec 27, 2023 • edited

andreaangiolillo left a comment

Choose a reason for hiding this comment

andreaangiolillo Dec 28, 2023 • edited

Choose a reason for hiding this comment

AgustinBettati Dec 28, 2023

Choose a reason for hiding this comment

andreaangiolillo Dec 28, 2023

Choose a reason for hiding this comment

AgustinBettati Dec 28, 2023

Choose a reason for hiding this comment

andreaangiolillo Dec 28, 2023

Choose a reason for hiding this comment

AgustinBettati Dec 28, 2023

Choose a reason for hiding this comment

maastha Dec 28, 2023

Choose a reason for hiding this comment

AgustinBettati Dec 28, 2023

Choose a reason for hiding this comment

AgustinBettati commented Dec 27, 2023 •

edited

1. Changes in the behaviour of `PlanOnly`

2. Changes in the behaviour of `ImportState`

AgustinBettati commented Dec 27, 2023 •

edited

andreaangiolillo Dec 28, 2023 •

edited