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.

Sign up for GitHub

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

Jump to bottom

chore: Corrects order of checks in data_source_federated_settings_identity_providers_test #2064

Merged
merged 1 commit into from Mar 23, 2024
Merged

chore: Corrects order of checks in data_source_federated_settings_identity_providers_test #2064

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions ...deratedsettingsidentityprovider/data_source_federated_settings_identity_providers_test.go
View file
Open in desktop
Expand Up @@ -28,12 +28,12 @@ func TestAccFederatedSettingsIdentityProvidersDS_basic(t *testing.T) {

resource.TestCheckResourceAttrSet(resourceName, "federation_settings_id"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why this did not fail with the original PR that changed the request behaviour

Copy link
Member

Choose a reason for hiding this comment

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

i suppose it's not defined the API response order

resource.TestCheckResourceAttrSet(resourceName, "results.#"),
resource.TestCheckResourceAttrSet(resourceName, "results.0.acs_url"),
resource.TestCheckResourceAttrSet(resourceName, "results.1.audience_claim.0"),
resource.TestCheckResourceAttrSet(resourceName, "results.1.client_id"),
resource.TestCheckResourceAttrSet(resourceName, "results.1.groups_claim"),
resource.TestCheckResourceAttrSet(resourceName, "results.1.requested_scopes.0"),
resource.TestCheckResourceAttrSet(resourceName, "results.1.user_claim"),
resource.TestCheckResourceAttrSet(resourceName, "results.1.acs_url"),
resource.TestCheckResourceAttrSet(resourceName, "results.0.audience_claim.0"),
resource.TestCheckResourceAttrSet(resourceName, "results.0.client_id"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure of this order? We should look into using TestCheckTypeSetElemAttr more for lists/sets.
An example where we use this already:
https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/internal/service/orginvitation/resource_org_invitation_test.go#L50

Copy link
Member

Choose a reason for hiding this comment

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

similar to @maastha , how about having tests that don't depend on the order using TestCheckTypeSetElemNestedAttrs? I don't know if it's possible in this case,
e.g.: https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/internal/service/cluster/resource_cluster_test.go#L1339

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Running the test now I consistently get this same order.
As per TestCheckTypeSetElemNestedAttrs and TestCheckTypeSetElemAttr I understand that this also checks a specific value has been set in at least one of the elements of the list/set. I don't like this for a couple of reasons:

  • Checks the specific value. This increases the coupling between the test and the current state of testing environment resources, which could change. This happens because this resources does not have create operation through terraform yet. Once create operation is available I would agree that checking value can be useful and valuable.
  • Majority of attributes being checked are only returned with a specific protocol(OIDC or SAML), so with these methods we could actually be missing edge cases where for some protocols the attributes are not being set when they should. But because it could be set in the other element of the list the check would be successful.

resource.TestCheckResourceAttrSet(resourceName, "results.0.groups_claim"),
resource.TestCheckResourceAttrSet(resourceName, "results.0.requested_scopes.0"),
resource.TestCheckResourceAttrSet(resourceName, "results.0.user_claim"),
),
},
},
Expand Down