chore: Reuses projects in executions in some resources and rename mig tests #2007

lantoli · 2024-03-11T13:52:52Z

Description

Reuses projects in executions in some resources and rename mig tests.

NOTE: To minimize the use of test resources, multiple small changes across many files are done in this PR, as opposed to extend them through many PRs and having to run the test groups multiple times.

TestMain added to all resources so they can use share resources like project execution.
TestAccMigrationXXX migration test names are renamed to TestMigXXX so acc test can be run only with a regexp and they don't need the skip param. Also we make test go "cacheable-friendly" as go test with skip param can't use cache.
Remove unneeded "${...}" in TF tests, e.g: "${mongodbatlas_api_key.test.org_id}" changed to: mongodbatlas_api_key.test.org_id
ClusterInfo changed to use ProjectIDExecution so execution project is reused in the following resources:
- cloud_backup_schedule
- cloud_backup_snapshot
- global_cluster_config
- search_index
- stream_connection
RandomProjectName changed in some resources to use ProjectIDExecution so execution project is reused in the following resources:
- alert_configuration
- database_user

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

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.
If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

.github/workflows/migration-tests.yml

marcosuma · 2024-03-14T13:09:27Z

internal/service/alertconfiguration/data_source_alert_configuration_test.go

 	)

-	resource.ParallelTest(t, resource.TestCase{
+	resource.Test(t, resource.TestCase{


is this intentional?

I guess it is based on the lines below :) maybe I missed it in the description but I am not understanding why we are not making it parallel anymore

My understanding is that before, each test was creating a project, so now that the different tests use the same one, the tests will fail if run in parallel

yes, I changed alert_configuration and database_user test to be serial on purpose. The reason is that they are resources common to the project.
For instance one test for alert_configuration plural datasource creates 2 alerts and ask if there are two, it will fail if other tests are creating others.
alert_notification creates like 20 projects and each one takes only like 5 seconds, so now they will create only 1 project and will take 2 minutes to run all.
however tests than don't change common project elements (which are the most expensive to run like the ones using clusters) can continue working on parallel.

as an example in this PR, these resources are reusing projects and running in parallel:
cloud_backup_schedule
cloud_backup_snapshot
global_cluster_config
search_index
stream_connection

both alert_configuration and database_user are in "config" test group, in your link it takes 3m5s

in this PR it takes 2m42s.

Current times:
ok github.com/mongodb/terraform-provider-mongodbatlas/internal/service/databaseuser 37.419s
ok github.com/mongodb/terraform-provider-mongodbatlas/internal/service/alertconfiguration 56.755s

PR times:
ok github.com/mongodb/terraform-provider-mongodbatlas/internal/service/databaseuser 39.836s
ok github.com/mongodb/terraform-provider-mongodbatlas/internal/service/alertconfiguration 50.120s

So even in serial it takes less time now :-)

Most of time in your log is creating the project, that's why they take in the 20s.

Also notice that all packages/resources in a test group run in parallel in different processes, so serial tests in database_user are not serialized together with alert_notification, serial is only in each resource.

Neither of these two resources is in the critical path for config test group.

Anyway I'm going to try what @AgustinBettati said of only running serially the plural ds test, that should work.

@marcosuma @andreaangiolillo after @AgustinBettati idea to run all in parallel except plural ds these are the new PR times 🎉

ok github.com/mongodb/terraform-provider-mongodbatlas/internal/service/databaseuser 23.636s
ok github.com/mongodb/terraform-provider-mongodbatlas/internal/service/alertconfiguration 33.800s

ok it actually did improve and your rationale seems right to me: avoiding creating the project all the times actually contributes to saving time.

Two follow-ups coming from this PR and conversation (not urgent, but worth a discussion):

couldn't we also convert plural tests to be parallel by changing the expectations? (e.g. remove the "check number of alerts equal n" in favour of "check alert with 'this' and 'this' name exists)

In n months (just random time in the future) when someone will see the code base and see some tests as running in parallel and some sequential, what do you think it could go wrong? Is there a risk that without proper clarification on the code base they might think it's an error and start investigating why?

1 - yes, that is a good idea, let me think about it
2 - we already do have a mix of parallel and serial (before this PR). let me see 1 if we can do the ds plural serial so we minimize exceptions.

EspenAlbert · 2024-03-14T13:23:22Z

internal/testutil/acc/database_user.go

@@ -1,130 +1,54 @@
 package acc

 import (


Any reason for not moving all these methods to the databaseuser_test package? Seems like they are only used there

no reason apart from trying to minimize the changes in this PR as it's getting very big. it's true that I moved checkExists and checkDestroy, I'll move all then, thx for the comment

sorry at the end I think i'm not going to do this refactor in this PR as it's not related to the main topic of the PR and will increase changes still more

AgustinBettati · 2024-03-14T16:07:24Z

internal/service/alertconfiguration/data_source_alert_configuration_test.go

-	resource.ParallelTest(t, resource.TestCase{
+	resource.Test(t, resource.TestCase{
 		PreCheck:                 func() { acc.PreCheckBasic(t) },
 		ProtoV6ProviderFactories: acc.TestAccProviderV6Factories,
 		CheckDestroy:             checkDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: configWithThreshold(orgID, projectName, true, 1),
+				Config: configWithThreshold(projectID, true, 1),


[q] taking this test as an example the terraform configuration is creating a single alert configuration and running a data source for that specific alert, could we preserve running this in parallel?
I would asume the only test cases that needs to be run sequentially are those that rely on asserting the plural data source, and effectively check a certain amount of resources. This also applies for database user tests that where adjusted from ParallelTest to Test

@AgustinBettati great callout!!! i've changed and it worked

AgustinBettati · 2024-03-14T16:21:36Z

internal/service/teams/resource_teams_test.go

+func TestMain(m *testing.M) {
+	acc.SetupSharedResources()
+	exitCode := m.Run()
+	acc.CleanupSharedResources()
+	os.Exit(exitCode)
+}


regarding where TestMain should be placed within the test package: I do not see a clear convention other than place the TestMain function wherever it makes the most sense.
With this in mind I believe having a main_test.go file (mentioned by @maastha) could be the most transparent option so it is clear the function impacts the entire test package. This is an example I found from the terraform repository: e2etest/main_test.go

@AgustinBettati @maastha ok, changed to their own main_test.go, also I like the idea of setup returning the cleanup function so cleanup doesn't need to be a public function

andreaangiolillo · 2024-03-14T16:19:46Z

internal/service/advancedcluster/resource_advanced_cluster_test.go

@@ -14,6 +14,13 @@ import (
 	"go.mongodb.org/atlas-sdk/v20231115007/admin"
 )

+func TestMain(m *testing.M) {


This PR is really hard to review 😢 Just an FYI that people in cloud were discussing this interesting blog post The ideal PR is 50 lines long

thx for the comment. Sorry, I know and I normally try to do small PRs. In this case I try to explain in the PR description why I chose to go for a bigger PR here.

.github/workflows/migration-tests.yml

internal/service/accesslistapikey/resource_access_list_api_key_migration_test.go

andreaangiolillo · 2024-03-14T16:47:32Z

internal/service/alertconfiguration/data_source_alert_configuration_test.go

-	resource.ParallelTest(t, resource.TestCase{
+	resource.Test(t, resource.TestCase{
 		PreCheck:                 func() { acc.PreCheckBasic(t) },
 		ProtoV6ProviderFactories: acc.TestAccProviderV6Factories,
 		CheckDestroy:             checkDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: configWithThreshold(orgID, projectName, true, 1),
+				Config: configWithThreshold(projectID, true, 1),


andreaangiolillo · 2024-03-14T16:49:28Z

internal/service/alertconfiguration/data_source_alert_configuration_test.go

 	)

-	resource.ParallelTest(t, resource.TestCase{
+	resource.Test(t, resource.TestCase{


It'd be great to show in the PR description the impact of running these tests sequentially vs in parallel: this info should be easy to get, just check the time needed to run the tests in this PR vs in the last nightly run

as I commented to Marco the "config" test group that runs database_user and alert_notification is taking less time now than before even being in serial. anyway i'm going to follow Agustin idea of just running in serial the plural ds tests.

internal/service/databaseuser/resource_database_user_test.go

…oup/project or cluster

AgustinBettati

LGTM, thanks for the follow up adjustments

AgustinBettati · 2024-03-15T09:24:02Z

internal/testutil/acc/shared_resource.go

+// It returns the cleanup function that must be called at the end of TestMain.
+func SetupSharedResources() func() {


like this approach of having the setup and cleanup coupled together to avoid any potential strange use case

lantoli · 2024-03-15T10:03:13Z

I'm going to merge as I think the general idea is fine, if you have any comment about some specific area let me know and will address in a following PR. In that way we don't have to run all test groups because a change in a specific area.

lantoli changed the title ~~chore: Reuses project in resources using GetClusterInfo~~ Reuses projects in executions and rename mig tests. Mar 13, 2024

lantoli changed the title ~~Reuses projects in executions and rename mig tests.~~ Reuses projects in executions and rename mig tests Mar 13, 2024

rename mig tests

c45b2e2

lantoli force-pushed the CLOUDP-237006_cluster_info branch from e89be08 to c45b2e2 Compare March 13, 2024 17:37

lantoli changed the title ~~Reuses projects in executions and rename mig tests~~ chore: Reuses projects in executions and rename mig tests Mar 13, 2024

TestMain in all packages [skip ci]

5819a8b

lantoli changed the title ~~chore: Reuses projects in executions and rename mig tests~~ chore: Reuses projects in executions in some resources and rename mig tests Mar 14, 2024

lantoli added 6 commits March 14, 2024 09:33

pass test context to GetClusterInfo

b308d7c

use ProjectIDExecution in GetClusterInfo

9117ea6

alert_configuration

1530f64

fix ProjectIDStr

28f32e1

database_user

91c4a74

remove unneded "${...}"

975661c

lantoli marked this pull request as ready for review March 14, 2024 12:25

lantoli requested a review from a team as a code owner March 14, 2024 12:25

marcosuma reviewed Mar 14, 2024

View reviewed changes

.github/workflows/migration-tests.yml Show resolved Hide resolved

marcosuma reviewed Mar 14, 2024

View reviewed changes

EspenAlbert reviewed Mar 14, 2024

View reviewed changes

AgustinBettati reviewed Mar 14, 2024

View reviewed changes

andreaangiolillo reviewed Mar 14, 2024

View reviewed changes

lantoli added 3 commits March 15, 2024 06:17

run database_user tests in parallel

88ee3a1

run alert_configuration tests in parallel

f12501b

TestMain in main_test.go file

f0d821a

oarbusi reviewed Mar 15, 2024

View reviewed changes

internal/service/databaseuser/resource_database_user_test.go Outdated Show resolved Hide resolved

fix checkDestroy so it returns ok if it doesn't find anything like gr…

e13a8b2

…oup/project or cluster

AgustinBettati approved these changes Mar 15, 2024

View reviewed changes

lantoli merged commit 09357b9 into master Mar 15, 2024
45 checks passed

lantoli deleted the CLOUDP-237006_cluster_info branch March 15, 2024 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Reuses projects in executions in some resources and rename mig tests #2007

chore: Reuses projects in executions in some resources and rename mig tests #2007

lantoli commented Mar 11, 2024 •

edited

Loading

marcosuma Mar 14, 2024

marcosuma Mar 14, 2024

oarbusi Mar 14, 2024

lantoli Mar 14, 2024 •

edited

Loading

lantoli Mar 14, 2024

lantoli Mar 15, 2024 •

edited

Loading

lantoli Mar 15, 2024 •

edited

Loading

marcosuma Mar 18, 2024

marcosuma Mar 18, 2024

lantoli Mar 18, 2024

EspenAlbert Mar 14, 2024

lantoli Mar 14, 2024

lantoli Mar 15, 2024

AgustinBettati Mar 14, 2024

andreaangiolillo Mar 14, 2024

lantoli Mar 15, 2024

AgustinBettati Mar 14, 2024

lantoli Mar 15, 2024

marcosuma Mar 18, 2024

andreaangiolillo Mar 14, 2024

lantoli Mar 15, 2024

andreaangiolillo Mar 14, 2024

andreaangiolillo Mar 14, 2024

lantoli Mar 15, 2024

AgustinBettati left a comment

AgustinBettati Mar 15, 2024

lantoli commented Mar 15, 2024 •

edited

Loading

		// It returns the cleanup function that must be called at the end of TestMain.
		func SetupSharedResources() func() {

chore: Reuses projects in executions in some resources and rename mig tests #2007

chore: Reuses projects in executions in some resources and rename mig tests #2007

Conversation

lantoli commented Mar 11, 2024 • edited Loading

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

lantoli Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

lantoli Mar 15, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgustinBettati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli commented Mar 15, 2024 • edited Loading

lantoli commented Mar 11, 2024 •

edited

Loading

lantoli Mar 14, 2024 •

edited

Loading

lantoli Mar 15, 2024 •

edited

Loading

lantoli Mar 15, 2024 •

edited

Loading

lantoli commented Mar 15, 2024 •

edited

Loading