fix: Fixes project flaky tests #1669
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -171,17 +171,14 @@ func (d *ProjectsDS) Read(ctx context.Context, req datasource.ReadRequest, resp | |
| } | ||
|
|
||
| func populateProjectsDataSourceModel(ctx context.Context, conn *matlas.Client, connV2 *admin.APIClient, stateModel *tfProjectsDSModel, projectsRes *matlas.Projects) error { | ||
|
|
||
| results := make([]*tfProjectDSModel, 0, len(projectsRes.Results)) | ||
| for _, project := range projectsRes.Results { | ||
| atlasTeams, atlasLimits, atlasProjectSettings, err := getProjectPropsFromAPI(ctx, conn, connV2, project.ID) | ||
| if err == nil { // if the project is still valid, e.g. could have just been deleted | ||
|
There was a problem hiding this comment. should we fail if There was a problem hiding this comment. no, that was the problem before. Imagine we get the list of projects for the org, but then some of them are deleted. It's not really a failure, so we just ignore it if it's deleted after it was in the list. This was the reason for project flaky test. There was a problem hiding this comment. ok noted, but then I guess we're ok to ignore 5xx errors? There was a problem hiding this comment. I've got 500 sometimes when doing something in a project that is being closed, so we could get also a 500 and it doesn't mean a real error but that the project is closing. So I think it's hard in this case to differentiate a "real" error from an error caused by the project being closed. Also chances are that if the project is in the list and it's not closed we'll get the details without any problem There was a problem hiding this comment. Understand this an edge case that is likely only appearing in our acceptance tests execution: fetching and creating/deleting project in parallel. Curious if a potential fix to this problem could be defining the data source test to run sequentially ( There was a problem hiding this comment. that would probably fix the tests, also to use the concurrency facility in GH actions so project acc and migration tests don't run in parallel. But at the end I thought this is a realistic use case that can happen sometimes and we don't want the DS to fail if that happens, that's why I focused on improving the prod. code. |
||
| projectModel := newTFProjectDataSourceModel(ctx, project, atlasTeams, atlasProjectSettings, atlasLimits) | ||
| results = append(results, &projectModel) | ||
| } | ||
| } | ||
| stateModel.Results = results | ||
| stateModel.TotalCount = types.Int64Value(int64(projectsRes.TotalCount)) | ||
| stateModel.ID = types.StringValue(id.UniqueId()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,6 @@ package project_test | |
| import ( | ||
| "fmt" | ||
| "os" | ||
| "testing" | ||
|
|
||
| matlas "go.mongodb.org/atlas/mongodbatlas" | ||
|
|
@@ -14,26 +13,23 @@ import ( | |
| ) | ||
|
|
||
| func TestAccProjectDSProjects_basic(t *testing.T) { | ||
| var ( | ||
| projectName = fmt.Sprintf("test-datasource-project-%s", acctest.RandString(10)) | ||
| orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") | ||
| ) | ||
| resource.Test(t, resource.TestCase{ | ||
| PreCheck: func() { acc.PreCheckBasic(t); acc.PreCheckProjectTeamsIds(t, 2) }, | ||
| ProtoV6ProviderFactories: acc.TestAccProviderV6Factories, | ||
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testAccMongoDBAtlasProjectsConfigWithDS(projectName, orgID, | ||
| []*matlas.ProjectTeam{ | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(0), | ||
| RoleNames: []string{"GROUP_READ_ONLY", "GROUP_DATA_ACCESS_ADMIN"}, | ||
| }, | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(1), | ||
| RoleNames: []string{"GROUP_DATA_ACCESS_ADMIN", "GROUP_OWNER"}, | ||
| }, | ||
| }, | ||
|
|
@@ -52,26 +48,23 @@ func TestAccProjectDSProjects_basic(t *testing.T) { | |
| } | ||
|
|
||
| func TestAccProjectDSProjects_withPagination(t *testing.T) { | ||
| var ( | ||
| projectName = fmt.Sprintf("test-datasource-project-%s", acctest.RandString(10)) | ||
| orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") | ||
| ) | ||
| resource.Test(t, resource.TestCase{ | ||
| PreCheck: func() { acc.PreCheckBasic(t); acc.PreCheckProjectTeamsIds(t, 2) }, | ||
| ProtoV6ProviderFactories: acc.TestAccProviderV6Factories, | ||
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testAccMongoDBAtlasProjectsConfigWithPagination(projectName, orgID, | ||
| []*matlas.ProjectTeam{ | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(0), | ||
| RoleNames: []string{"GROUP_READ_ONLY", "GROUP_DATA_ACCESS_ADMIN"}, | ||
| }, | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(1), | ||
| RoleNames: []string{"GROUP_DATA_ACCESS_ADMIN", "GROUP_OWNER"}, | ||
| }, | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,6 @@ package project_test | |
| import ( | ||
| "os" | ||
| "regexp" | ||
| "testing" | ||
|
|
||
| "github.com/hashicorp/terraform-plugin-testing/helper/acctest" | ||
|
|
@@ -20,26 +19,21 @@ func TestAccProjectRSProject_basic(t *testing.T) { | |
| projectName = acctest.RandomWithPrefix("test-acc") | ||
| orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") | ||
| clusterCount = "0" | ||
| ) | ||
| resource.ParallelTest(t, resource.TestCase{ | ||
| PreCheck: func() { acc.PreCheckBasic(t); acc.PreCheckProjectTeamsIds(t, 3) }, | ||
| ProtoV6ProviderFactories: acc.TestAccProviderV6Factories, | ||
| CheckDestroy: acc.CheckDestroyProject, | ||
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: acc.ConfigProject(projectName, orgID, | ||
| []*matlas.ProjectTeam{ | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(0), | ||
| RoleNames: []string{"GROUP_READ_ONLY", "GROUP_DATA_ACCESS_ADMIN"}, | ||
| }, | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(1), | ||
| RoleNames: []string{"GROUP_DATA_ACCESS_ADMIN", "GROUP_OWNER"}, | ||
| }, | ||
| }, | ||
|
|
@@ -57,15 +51,15 @@ func TestAccProjectRSProject_basic(t *testing.T) { | |
| Config: acc.ConfigProject(projectName, orgID, | ||
| []*matlas.ProjectTeam{ | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(0), | ||
| RoleNames: []string{"GROUP_OWNER"}, | ||
| }, | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(1), | ||
| RoleNames: []string{"GROUP_DATA_ACCESS_READ_WRITE"}, | ||
| }, | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(2), | ||
| RoleNames: []string{"GROUP_READ_ONLY", "GROUP_DATA_ACCESS_ADMIN"}, | ||
| }, | ||
| }, | ||
|
|
@@ -84,11 +78,11 @@ func TestAccProjectRSProject_basic(t *testing.T) { | |
|
|
||
| []*matlas.ProjectTeam{ | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(0), | ||
| RoleNames: []string{"GROUP_READ_ONLY", "GROUP_READ_ONLY"}, | ||
| }, | ||
| { | ||
| TeamID: acc.GetProjectTeamsIds(1), | ||
| RoleNames: []string{"GROUP_OWNER", "GROUP_DATA_ACCESS_ADMIN"}, | ||
| }, | ||
| }, | ||
|
|
@@ -232,24 +226,22 @@ func TestAccProjectRSProject_withUpdatedRole(t *testing.T) { | |
| roleName = "GROUP_DATA_ACCESS_ADMIN" | ||
| roleNameUpdated = "GROUP_READ_ONLY" | ||
| clusterCount = "0" | ||
| ) | ||
| resource.ParallelTest(t, resource.TestCase{ | ||
| PreCheck: func() { acc.PreCheckBasic(t); acc.PreCheckProjectTeamsIds(t, 1) }, | ||
| ProtoV6ProviderFactories: acc.TestAccProviderV6Factories, | ||
| CheckDestroy: acc.CheckDestroyProject, | ||
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: acc.ConfigProjectWithUpdatedRole(projectName, orgID, acc.GetProjectTeamsIds(0), roleName), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| resource.TestCheckResourceAttr(resourceName, "name", projectName), | ||
| resource.TestCheckResourceAttr(resourceName, "org_id", orgID), | ||
| resource.TestCheckResourceAttr(resourceName, "cluster_count", clusterCount), | ||
| ), | ||
| }, | ||
| { | ||
| Config: acc.ConfigProjectWithUpdatedRole(projectName, orgID, acc.GetProjectTeamsIds(0), roleNameUpdated), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| resource.TestCheckResourceAttr(resourceName, "name", projectName), | ||
| resource.TestCheckResourceAttr(resourceName, "org_id", orgID), | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ package acc | |||||
|
|
||||||
| import ( | ||||||
| "os" | ||||||
| "strings" | ||||||
| "testing" | ||||||
| ) | ||||||
|
|
||||||
|
|
@@ -45,10 +46,26 @@ func PreCheckAtlasUsername(tb testing.TB) { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| func PreCheckProjectTeamsIds(tb testing.TB, min int) { | ||||||
|
There was a problem hiding this comment. improve teamsIds checks, and fail tests if pre-check is not met There was a problem hiding this comment. nit: can you please pass the name of the environment variable as input of the function? There was a problem hiding this comment. same, I think it's better to have the env.var in both methods as they're used only for teamsIds, these methods are not intended to be generic for other purporses. There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. I can change it, although I tried to follow "go idiomatic" in the sense the I think "min" in this context is clear, but I agree I can make it more specific here. There was a problem hiding this comment. renamed to minTeamsCount (although it looks a bit redudant to me as Teams is already in function name) |
||||||
| envVar := os.Getenv("MONGODB_ATLAS_TEAMS_IDS") | ||||||
| if envVar == "" { | ||||||
| tb.Fatal("`MONGODB_ATLAS_TEAMS_IDS` must be set for Projects acceptance testing") | ||||||
| return | ||||||
| } | ||||||
| teamsIds := strings.Split(envVar, ",") | ||||||
| if count := len(teamsIds); count < min { | ||||||
| tb.Fatalf("`MONGODB_ATLAS_TEAMS_IDS` must have at least %d team ids for this acceptance testing, has %d", min, count) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func GetProjectTeamsIds(pos int) string { | ||||||
| envVar := os.Getenv("MONGODB_ATLAS_TEAMS_IDS") | ||||||
|
There was a problem hiding this comment. nit: can you please pass the name of the environment variable as input of the function? There was a problem hiding this comment. I think it's better in this way as PreCheckProjectTeamsIds and GetProjectTeamsIds work together for teamsIds and they both use the same env.var There was a problem hiding this comment.
I think this is an assumption that might not be always true, unless you call the There was a problem hiding this comment. if you don't call precheck you won't check that you have the min teams you need but it will still work (returns "" when you don't have that pos). (They have the same name part ProjectTeamsIds and are located together in the file to make it easier to see that they are intended to work together) |
||||||
| teamsIds := strings.Split(envVar, ",") | ||||||
| count := len(teamsIds) | ||||||
| if envVar == "" || pos >= count { | ||||||
| return "" | ||||||
| } | ||||||
| return teamsIds[pos] | ||||||
| } | ||||||
|
|
||||||
| func PreCheckGov(tb testing.TB) { | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used