fix: Fixes project flaky tests #1669

lantoli · 2023-11-27T15:26:13Z

Description

Jira ticket: INTMDB-1341

Fixes project flaky tests

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

This reverts commit f021848.

This reverts commit 571fa00.

This reverts commit 43aae57.

This reverts commit 698adeb.

This reverts commit 1c96572.

This reverts commit 95e0854.

This reverts commit f1f0a82.

This reverts commit 8c546b4.

This reverts commit 1f8e81d.

lantoli · 2023-11-28T20:46:39Z

.github/workflows/acceptance-tests.yml

@@ -335,7 +335,6 @@ jobs:
          MONGODB_ATLAS_ORG_ID: ${{ vars.MONGODB_ATLAS_ORG_ID_CLOUD_DEV }}
          MONGODB_ATLAS_BASE_URL: ${{ vars.MONGODB_ATLAS_BASE_URL }}
          MONGODB_ATLAS_PROJECT_OWNER_ID: ${{ vars.MONGODB_ATLAS_PROJECT_OWNER_ID }}
-          MONGODB_ATLAS_API_KEYS_IDS: ${{ vars.MONGODB_ATLAS_API_KEYS_IDS }}


lantoli · 2023-11-28T20:47:54Z

internal/testutil/acc/pre_check.go

-func PreCheckTeamsIds(tb testing.TB) {
-	if os.Getenv("MONGODB_ATLAS_TEAMS_IDS") == "" {
-		tb.Skip("`MONGODB_ATLAS_TEAMS_IDS` must be set for Projects acceptance testing")
+func PreCheckProjectTeamsIds(tb testing.TB, min int) {


improve teamsIds checks, and fail tests if pre-check is not met

This reverts commit d6d391f.

…er being in the list

lantoli · 2023-11-28T21:24:38Z

internal/service/project/data_source_projects.go

@@ -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 {


the fix is in this func

lantoli · 2023-11-28T21:29:13Z

internal/service/project/data_source_project_test.go

-	orgID := os.Getenv("MONGODB_ATLAS_ORG_ID")
-	teamsIds := strings.Split(os.Getenv("MONGODB_ATLAS_TEAMS_IDS"), ",")
-	if len(teamsIds) < 2 {
-		t.Skip("`MONGODB_ATLAS_TEAMS_IDS` must have 2 team ids for this acceptance testing")


improve pre-checks and fail if they're not met instead of just skipping the test

marcosuma · 2023-11-29T06:17:51Z

internal/service/project/data_source_projects.go

 		atlasTeams, atlasLimits, atlasProjectSettings, err := getProjectPropsFromAPI(ctx, conn, connV2, project.ID)
-		if err != nil {
-			return fmt.Errorf("error while getting project properties for project %s: %v", project.ID, err.Error())
+		if err == nil { // if the project is still valid, e.g. could have just been deleted


should we fail if err != nil ?

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.

ok noted, but then I guess we're ok to ignore 5xx errors?
What I am trying to understand here is: if there is a 5xx error and we ignore that and we move on, will we get a different test behaviour that might hide some problem?

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

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 (resource.Test( over resource.ParallelTest() to reduce the chances of this scenario. (just to keep in mind for other flaky tests that we might have)

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.

marcosuma · 2023-11-29T06:20:13Z

internal/testutil/acc/pre_check.go

+}
+
+func GetProjectTeamsIds(pos int) string {
+	envVar := os.Getenv("MONGODB_ATLAS_TEAMS_IDS")


nit: can you please pass the name of the environment variable as input of the function?

I think it's better in this way as PreCheckProjectTeamsIds and GetProjectTeamsIds work together for teamsIds and they both use the same env.var

work together

I think this is an assumption that might not be always true, unless you call the precheck... in the get... method right?

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).
but if you don't call the precheck method, i don't see how passing the env.var can help there.

(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)

marcosuma · 2023-11-29T06:20:29Z

internal/testutil/acc/pre_check.go

-func PreCheckTeamsIds(tb testing.TB) {
-	if os.Getenv("MONGODB_ATLAS_TEAMS_IDS") == "" {
-		tb.Skip("`MONGODB_ATLAS_TEAMS_IDS` must be set for Projects acceptance testing")
+func PreCheckProjectTeamsIds(tb testing.TB, min int) {


nit: can you please pass the name of the environment variable as input of the function?

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.

marcosuma · 2023-11-29T06:23:48Z

internal/service/project/data_source_project_test.go

 	resource.ParallelTest(t, resource.TestCase{
-		PreCheck:                 func() { acc.PreCheckBasic(t); acc.PreCheckTeamsIds(t) },
+		PreCheck:                 func() { acc.PreCheckBasic(t); acc.PreCheckProjectTeamsIds(t, 2) },


Suggested change

PreCheck: func() { acc.PreCheckBasic(t); acc.PreCheckProjectTeamsIds(t, 2) },

PreCheck: func() { acc.PreCheckBasic(t); acc.PreCheckProjectTeamsIdsWithMinSize(t, 2) },

nit: for clarity I would suggest a more specific method name

I can do that but I normally like to say "with..." when there are multiple related methods, there is only one method in this case

renamed to: func PreCheckProjectTeamsIdsWithMinCount(tb testing.TB, minTeamsCount int)

marcosuma · 2023-11-29T06:24:27Z

internal/testutil/acc/pre_check.go

-func PreCheckTeamsIds(tb testing.TB) {
-	if os.Getenv("MONGODB_ATLAS_TEAMS_IDS") == "" {
-		tb.Skip("`MONGODB_ATLAS_TEAMS_IDS` must be set for Projects acceptance testing")
+func PreCheckProjectTeamsIds(tb testing.TB, min int) {


Suggested change

func PreCheckProjectTeamsIds(tb testing.TB, min int) {

func PreCheckProjectTeamsIds(tb testing.TB, minProjectCount int) {

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.

renamed to minTeamsCount (although it looks a bit redudant to me as Teams is already in function name)

marcosuma

few minor comments, LGTM overall

github-actions · 2023-11-29T06:42:22Z

Package	Line Rate	Health
github.com/mongodb/terraform-provider-mongodbatlas/internal/common/conversion	31%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/common/validate	68%	➖
github.com/mongodb/terraform-provider-mongodbatlas/internal/provider	5%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/advancedcluster	3%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/alertconfiguration	0%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/atlasuser	0%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/cluster	1%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/databaseuser	0%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/encryptionatrest	0%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/project	0%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/projectipaccesslist	0%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/searchdeployment	23%	❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/acc	5%	❌
github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas	2%	❌
Summary	3% (315 / 10714)	❌

test fail if precondition for MONGODB_ATLAS_ORG_ID is not met

37bd3a6

lantoli changed the title ~~bug: Fixes project flaky tests~~ fix: Fixes project flaky tests Nov 27, 2023

github-actions bot added the bug label Nov 27, 2023

lantoli added 13 commits November 27, 2023 17:39

refactor teamsIds

38e9bab

remove unused var MONGODB_ATLAS_API_KEYS_IDS

98d87b6

make an explicit dependency

f021848

Revert "make an explicit dependency"

72539d0

This reverts commit f021848.

enable TF logs

571fa00

Revert "enable TF logs"

260afbb

This reverts commit 571fa00.

check TF versions in GH action

1f8e81d

TEMPORARY - run only flaky test

8c546b4

debug terraform file

f1f0a82

try 2 tests

43aae57

Revert "try 2 tests"

3291160

This reverts commit 43aae57.

debug statements

95e0854

more debug

1c96572

lantoli added run-testacc-project and removed run-testacc-project labels Nov 28, 2023

lantoli added 8 commits November 28, 2023 20:16

debug delete project

698adeb

TEMPORARY - don't run project migration tests

d6d391f

Revert "debug delete project"

a30ccce

This reverts commit 698adeb.

Revert "more debug"

c76b692

This reverts commit 1c96572.

Revert "debug statements"

f2a16ae

This reverts commit 95e0854.

Revert "debug terraform file"

1ee3627

This reverts commit f1f0a82.

Revert "TEMPORARY - run only flaky test"

2b2c8da

This reverts commit 8c546b4.

Revert "check TF versions in GH action"

9266a49

This reverts commit 1f8e81d.

lantoli commented Nov 28, 2023

View reviewed changes

lantoli added 2 commits November 28, 2023 21:48

Revert "TEMPORARY - don't run project migration tests"

89b0ea3

This reverts commit d6d391f.

fix data source plural so it doesn't fail if a project is deleted aft…

fc43b97

…er being in the list

lantoli commented Nov 28, 2023

View reviewed changes

lantoli marked this pull request as ready for review November 28, 2023 21:26

lantoli requested a review from a team as a code owner November 28, 2023 21:26

lantoli commented Nov 28, 2023

View reviewed changes

marcosuma reviewed Nov 29, 2023

View reviewed changes

marcosuma approved these changes Nov 29, 2023

View reviewed changes

lantoli added 2 commits November 29, 2023 07:34

rename minTeamsCount and PreCheckProjectTeamsIdsWithMinCount

be185bc

rename GetProjectTeamsIdsWithPos

817acb0

lantoli merged commit 62ae4e1 into master Nov 29, 2023
28 checks passed

lantoli deleted the INTMDB-1341_project_fix branch November 29, 2023 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fixes project flaky tests #1669

fix: Fixes project flaky tests #1669

lantoli commented Nov 27, 2023

lantoli Nov 28, 2023

lantoli Nov 28, 2023

lantoli Nov 28, 2023

lantoli Nov 28, 2023

marcosuma Nov 29, 2023

lantoli Nov 29, 2023 •

edited

Loading

marcosuma Nov 29, 2023

lantoli Nov 29, 2023 •

edited

Loading

AgustinBettati Nov 29, 2023

lantoli Nov 29, 2023

marcosuma Nov 29, 2023

lantoli Nov 29, 2023 •

edited

Loading

marcosuma Nov 29, 2023 •

edited

Loading

lantoli Nov 29, 2023 •

edited

Loading

marcosuma Nov 29, 2023

lantoli Nov 29, 2023

marcosuma Nov 29, 2023

lantoli Nov 29, 2023

lantoli Nov 29, 2023

marcosuma Nov 29, 2023

lantoli Nov 29, 2023

lantoli Nov 29, 2023 •

edited

Loading

marcosuma left a comment

github-actions bot commented Nov 29, 2023

		@@ -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 {

	PreCheck: func() { acc.PreCheckBasic(t); acc.PreCheckProjectTeamsIds(t, 2) },
	PreCheck: func() { acc.PreCheckBasic(t); acc.PreCheckProjectTeamsIdsWithMinSize(t, 2) },

	func PreCheckProjectTeamsIds(tb testing.TB, min int) {
	func PreCheckProjectTeamsIds(tb testing.TB, minProjectCount int) {

fix: Fixes project flaky tests #1669

fix: Fixes project flaky tests #1669

Conversation

lantoli commented Nov 27, 2023

Description

Type of change:

Required Checklist:

Further comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

marcosuma Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

lantoli Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

marcosuma left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2023

lantoli Nov 29, 2023 •

edited

Loading

lantoli Nov 29, 2023 •

edited

Loading

lantoli Nov 29, 2023 •

edited

Loading

marcosuma Nov 29, 2023 •

edited

Loading

lantoli Nov 29, 2023 •

edited

Loading

lantoli Nov 29, 2023 •

edited

Loading