chore: Creates project for test execution #2010

lantoli · 2024-03-12T13:45:20Z

Description

Creates project for test execution. As an example it's used for privatelink_endpoint_service_data_federation_online_archive resource tests.

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

This creates a project for each resource test execution (if needed), example log:

Creating execution project: test-acc-tf-p-430893557017739723, resource: privatelinkendpointservicedatafederationonlinearchive, global project (not found): test-acc-tf-p-keep-privatelinkendpointservicedatafederationonlin
--- PASS: TestAccNetworkPrivatelinkEndpointServiceDataFederationOnlineArchive_basic (11.40s)
=== RUN   TestAccNetworkPrivatelinkEndpointServiceDataFederationOnlineArchive_basicWithRegionDnsName
--- PASS: TestAccNetworkPrivatelinkEndpointServiceDataFederationOnlineArchive_basicWithRegionDnsName (8.42s)
PASS
Deleting execution project: test-acc-tf-p-430893557017739723, resource: privatelinkendpointservicedatafederationonlinearchive

Optionally it can use an existing project for that resource if it exists, example log:

Reusing global project: test-acc-tf-p-keep-privatelinkendpointservicedatafederationonlin, resource: privatelinkendpointservicedatafederationonlinearchive

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

lantoli · 2024-03-12T14:54:36Z

...ononlinearchive/resource_privatelink_endpoint_service_data_federation_online_archive_test.go

@@ -18,9 +18,13 @@ var (
 	atlasRegion  = "US_EAST_1"
 )

+func TestMain(m *testing.M) {


Go runs this function before any test in the package

lantoli · 2024-03-12T14:55:17Z

internal/testutil/acc/atlas.go

+	SkipInUnitTest(tb)
+	require.True(tb, atlasInfo.init, "TestMainExecution must called to be able to use ProjectIDExecution")
+
+	atlasInfo.mu.Lock()


we serialize access to avoid problems while creating the project

lantoli · 2024-03-12T14:56:12Z

internal/testutil/acc/atlas.go

+)
+
+func resourceName() string {
+	pc, _, _, ok := runtime.Caller(2)


this is to get the full function name of the test and extract the resource name

marcosuma · 2024-03-12T15:15:37Z

internal/testutil/acc/atlas.go

+)
+
+// TestMainExecution must be called from TestMain in the test package if ProjectIDExecution is going to be used.
+func TestMainExecution(m *testing.M) {


suggest improving both method names. This one sounds more like an init() and the other one I don't have a suggestion because I am not really sure I understand it. It looks like it's a getProjectID() kind of method

any suggestion? This method is the implementation of TestMain in the tests, it basically do some setup, execute the tests and teardown.

I called the methods TestMainExecution and ProjectIDExecution with the intention that Execution means during the test execution of the package. So for example ProjectIDExecution provides the id of a project that is created before the tests run and deleted after.

looking at what this file is managing, I think this should be about project resource initialisation.

TestMainExecution --> TestResourceInit
ProjectIDExecution --> GetOrCreateProjectID

I would avoid having this generic TestMain logic when in reality it is solely focusing on project resource re-usage (reminds me of YAGNI).

I still have not digested this file in full (as I mentioned somewhere, it looks too complicated), looping in the rest of the team @maastha @AgustinBettati @EspenAlbert @oarbusi

TestMainExecution is really for initialising and destroying resources, and calling test runs in the middle.

It's our custom implementation for TestMain adding resource lifecycle management

some info here about TestMain: https://medium.com/goingogo/why-use-testmain-for-testing-in-go-dafb52b406bc

I do agree that we should focus on more concrete naming so that it is more clear what is being done. I was more on the side of a name such as SetupSharedProject over TestMainExecution.

TestMainExecution is really for initialising and destroying resources

Regarding this point I was slightly confused because this function does not contain all the initialising logic, for example the creation of the project is handled in ProjectIDExecution. Was this for a specific reason?

yes, because i realized that for instance right now we always run all packages even if one test won't be run. so I lazy initialize the project creation in case test is being executed but doesn't need to get the execution project. in that case we don't need to create the project

How about ProjectIDForExecution or GetProjectIDForCurrentExecution or something?

Additionally, I think some methods/files could use more comments as to the "why/how", for example shared_resource.go (or the TestMain() but not sure if we need to do for every resource), as the mechanism could be hard for someone new to understand or use or extend completely or in case we run into any issues later.

I'm not sure about document in our code how Go features like TestMain work, but I'm fine to document more our code. I'm going to merge but please feel free to suggest doc, and i'll think in more docs in the following PRs, thx!

marcosuma · 2024-03-12T15:18:55Z

internal/testutil/acc/atlas.go

+)
+
+// TestMainExecution must be called from TestMain in the test package if ProjectIDExecution is going to be used.
+func TestMainExecution(m *testing.M) {


can't this class also be called by multiple places in parallel?

no, Go test calls TestMain and we call this function from there, it's like the main function but when running tests

ok noted. I am curious to know more: TestMain is called once per test package. So if in a subsequent PR you also add func TestMain(m *testing.M) {...} to another package, will this still work?
From how I am getting this (also based on your comment Go runs this function before any test in the package), it looks like there can be multiple TestMain calls from all the different test packages (which I think we have more than one)

yeah, the idea is to add TestMain to all packages.
when you run tests you choose the package, and as TestMain is a public function (uppercase) there can be only one TestMain for that package, and that's the function to be called when running the test for that package.

So it can't happen that two TestMain are run in a test execution, the execution is only for one package, and that can have at most one TestMain

marcosuma · 2024-03-12T15:21:54Z

internal/testutil/acc/atlas.go

+
+	exitCode := m.Run()
+
+	if !InUnitTest() && atlasInfo.needsDeletion {


reading this if makes me thinking that we're designing a non-trivial set of rules for which resources should be re-used or deleted. I see a potential risk here that tests could start randomly failing due to concurrency issues and figuring what happened is hard.

Let me start first with:

Why this if? What is the rationale?

Could we make things simpler? e.g. re-use resource for every test file and make it just a serial process?

Is there an alternative way to write this?

by default a project is created when you call ProjectIDExecution and it's deleted after all tests run. This if deletes the project created for this test execution.

i made a further optimization to allow reuse of a global project for that resource if defined so we avoid creating the project during the test run.

i'm not sure the code can be simplified to fulfil those requirements, but we'll probably learn more as we use this in other resources and might be able to simplify

i've simplified the if condition a little bit, let me know if it helps

1 - the if is to delete the project if it was created before running the tests
2 - we're reusing the resource (project) for all tests in the resource but we allow the tests to run in parallel as now so performance is not affected
3 - surely it can be written in other ways, but I think this approach is ok at the moment, we'll probably learn more soon and will be able to improve it. any suggestions?

lantoli · 2024-03-13T02:45:32Z

internal/testutil/acc/atlas.go

+			atlasInfo.projectName = RandomProjectName()
+			tb.Logf("Creating execution project: %s, resource: %s, global project (not found): %s\n", atlasInfo.projectName, atlasInfo.resourceName, globalName)
+			atlasInfo.projectID = createProject(tb, atlasInfo.projectName)
+			atlasInfo.needsDeletion = true


this flags that we created a project for the test execution and we'll need to delete it after all tests finish

marcosuma · 2024-03-13T07:12:09Z

internal/testutil/acc/atlas.go

+	projectNameMaxLen = 64
+)
+
+func resourceName() string {


I am probably on the side of making things as simple as possible. Can't we just pass the name of the resource when things are initialised?

i think this is less error prone than passing the resource name as a string in each call. i think it's better if it can be got automatically.

(something similar happens in the GH Actions where we have some errors from time to time because copying/pasting incorrectly to pass redundant info)

ok noted, this method makes me thinking that even if this is code written for testing infra I think it should be unit tested. If we don't test it, I suspect it will become hard to touch for other engineers.

also this method is not really very important, it could fail and return "" and everything would work (except trying to reuse the global project for that resource).

but you have a point, let me see if it can be tested, it's a bit tricky because it depends on the resource test structure.

marcosuma · 2024-03-13T07:14:24Z

internal/testutil/acc/atlas.go

+	return matches[1]
+}
+
+func createProject(tb testing.TB, name string) string {


[nit] this looks like a more generic util method to be put outside this file.

AgustinBettati · 2024-03-13T10:56:52Z

internal/testutil/acc/name.go

-func ProjectIDGlobal(tb testing.TB) string {
-	tb.Helper()
-	return projectID(tb, prefixProjectKeep+"-global")
-}
-
-func projectID(tb testing.TB, name string) string {
-	tb.Helper()
-	SkipInUnitTest(tb)
-	resp, _, _ := ConnV2().ProjectsApi.GetProjectByName(context.Background(), name).Execute()
-	id := resp.GetId()
-	require.NotEmpty(tb, id, "Project name not found: %s", name)
-	return id
-}


[q] Why are we removing the logic of having a Global project? I would imagine we would have a mix of both have a global project, and also projects that are created per resource execution

because I realized that projects can't have more than 25 clusters so I don't think that's an option any more, at least for generic projects that don't need any special configuration.
for projects than need some setup (like triggers) we'll probably have a global project for triggers.

but in the resource in this PR it doesn't need any special config so i think it's better not to use a global project for it (it can use a global project for that resource, but not shared by all/many resources)

AgustinBettati · 2024-03-13T11:05:09Z

internal/testutil/acc/atlas.go

+		if atlasInfo.resourceName != "" {
+			globalName = (prefixProjectKeep + "-" + atlasInfo.resourceName)[:projectNameMaxLen]
+			globalID = projectID(globalName)
+		}


[q] I am struggling to understand in which case resourceName would be empty. If it is empty it is likely an indicator that TestMainExecution was not called, and in this case I would opt for stopping the execution all together.

it's just a precaution, imagine a resource doesn't follow the expected structure. but i agree it shouldn't happen but in some edge cases

AgustinBettati · 2024-03-13T11:09:13Z

internal/testutil/acc/atlas.go

+			atlasInfo.projectName = globalName
+			tb.Logf("Reusing global project: %s, resource: %s\n", atlasInfo.projectName, atlasInfo.resourceName)
+			atlasInfo.projectID = globalID


Is there a possibility of having a separate function for providing the global project? I believe this would help simplify this function and make its usage more clear. In that case I understand that needsDeletion property could be removed as well.

i had in mind that projects just ask for a project and a new one or a resource-global one is given but they don't need to know. and there is a nice fallback mechanism so you can create global-resource clusters for the most demanding resources and maybe for resources that just create a couple of projects you don't want to create a global-resource one as the benefit is not so big and you need that project always created.

do you think it's better to remove this abstraction and projects decide if they want an execution project or a global-project?

in any way there will be a way to ask for specific projects when they need some extra config like triggers, but i don't think it's so useful to allow this for regular projects

gssbzn · 2024-03-13T13:42:33Z

...ononlinearchive/resource_privatelink_endpoint_service_data_federation_online_archive_test.go

@@ -18,9 +18,13 @@ var (
 	atlasRegion  = "US_EAST_1"
 )

+func TestMain(m *testing.M) {
+	acc.TestMainExecution(m)


don't handle the exist status internally let TestMain handle that which is supposed to do

it's a bit more verbose but I like the idea and it's more explicit, will change to something like:

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

thx!

lantoli · 2024-03-13T14:42:21Z

...nlinearchive/data_source_privatelink_endpoint_service_data_federation_online_archive_test.go

@@ -15,7 +15,7 @@ var (

 func TestAccNetworkPrivatelinkEndpointServiceDataFederationOnlineArchiveDS_basic(t *testing.T) {
 	var (
-		projectID               = acc.ProjectIDGlobal(t)
+		projectID               = acc.ProjectIDExecution(t)


all functions like this will start with ProjectID, we have ProjectIDExecution, ProjectIDGlobal and might have ProjectIDTrigger, etc.

gssbzn · 2024-03-13T15:10:12Z

internal/testutil/acc/atlas.go

+func createProject(tb testing.TB, name string) string {
+	tb.Helper()
+	orgID := os.Getenv("MONGODB_ATLAS_ORG_ID")
+	require.NotNil(tb, "Project creation failed: %s, org not set", name)


Suggested change

require.NotNil(tb, "Project creation failed: %s, org not set", name)

require.NotNilf(tb, "Project creation failed: %s, org not set", name)

gssbzn · 2024-03-13T15:10:48Z

internal/testutil/acc/atlas.go

+	require.NotNil(tb, "Project creation failed: %s, org not set", name)
+	params := &admin.Group{Name: name, OrgId: orgID}
+	resp, _, err := ConnV2().ProjectsApi.CreateProject(context.Background(), params).Execute()
+	require.NoError(tb, err, "Project creation failed: %s, err: %s", name, err)


Suggested change

require.NoError(tb, err, "Project creation failed: %s, err: %s", name, err)

require.NoErrorf(tb, err, "Project creation failed: %s, err: %s", name, err)

gssbzn · 2024-03-13T15:11:28Z

internal/testutil/acc/atlas.go

+	resp, _, err := ConnV2().ProjectsApi.CreateProject(context.Background(), params).Execute()
+	require.NoError(tb, err, "Project creation failed: %s, err: %s", name, err)
+	id := resp.GetId()
+	require.NotEmpty(tb, id, "Project creation failed: %s", name)


Suggested change

require.NotEmpty(tb, id, "Project creation failed: %s", name)

require.NotEmptyf(tb, id, "Project creation failed: %s", name)

gssbzn · 2024-03-13T15:12:27Z

internal/testutil/acc/atlas.go

+	SkipInUnitTest(tb)
+	resp, _, _ := ConnV2().ProjectsApi.GetProjectByName(context.Background(), name).Execute()
+	id := resp.GetId()
+	require.NotEmpty(tb, id, "Project name not found: %s", name)


can you make sure all assertions where you do string formatting end with f for format, other wise is a concatenation

I think they do the same, e.g.:

a := "hello" require.Empty(t, a, "a is not empty: %s, watchout", a) Error: Should be empty, but was hello Test: TestAccNetworkPrivatelinkEndpointServiceDataFederationOnlineArchive_basic Messages: a is not empty: hello, watchout

the signature is: func Empty(t TestingT, object interface{}, msgAndArgs ...interface{}) {

example of implementation:

// assert.NotNilf(t, err, "error message %s", "formatted") func NotNilf(t TestingT, object interface{}, msg string, args ...interface{}) bool { if h, ok := t.(tHelper); ok { h.Helper() } return NotNil(t, object, append([]interface{}{msg}, args...)...) }

i think it's just to make sure that the first param is a string (with the msg)

yeah I see internally does fmt.Sprintf(msgAndArgs[0].(string), msgAndArgs[1:]...) I assumed this was closer to t.Error vs t.Errorf 🤷

gssbzn · 2024-03-13T15:13:34Z

internal/testutil/acc/atlas.go

+	return id
+}
+
+func deleteProject(id string) {


out of curiosity why create is a helper but delete is not?

the creation is done lazily inside a test so we have the T, but deletion is done in TestMain when all test have finished and we don't have test context anymore

gssbzn · 2024-03-13T15:14:31Z

internal/testutil/acc/atlas.go

+func deleteProject(id string) {
+	_, _, err := ConnV2().ProjectsApi.DeleteProject(context.Background(), id).Execute()
+	if err != nil {
+		fmt.Printf("Project deletion failed: %s, error: %s", id, err)


which is delete is a helper you can then do

Suggested change

fmt.Printf("Project deletion failed: %s, error: %s", id, err)

tb.Logf("Project deletion failed: %s, error: %s", id, err)

can't be because it's called from TestMain not from a test

I see, this may make this approach a bit noisy on the logs since you usually only want logs when running on verbose mode (-v) and right now with printf is harder to control

we're running with -v in GH Actions, but you have a point, will consider in next PRs if delete it, thanks!

gssbzn · 2024-03-13T15:17:51Z

internal/testutil/acc/shared_resource.go

+// CleanupSharedResources must be called from TestMain test package in order to use ProjectIDExecution.
+func CleanupSharedResources() {
+	if sharedInfo.projectID != "" {
+		fmt.Printf("Deleting execution project: %s, id: %s\n", sharedInfo.projectName, sharedInfo.projectID)


how are we running tests? normally you want logs when running in verbose mode -v

we use verbose flag:

TF_ACC=1 go test $(ACCTEST_PACKAGES) -run '$(ACCTEST_REGEX_RUN)' -skip '$(ACCTEST_REGEX_SKIP)' -v -parallel $(PARALLEL_GO_TEST) $(TESTARGS) -timeout $(ACCTEST_TIMEOUT) -ldflags="$(LINKER_FLAGS)"

gssbzn · 2024-03-13T15:28:33Z

internal/testutil/acc/atlas.go

+	SkipInUnitTest(tb)
+	resp, _, _ := ConnV2().ProjectsApi.GetProjectByName(context.Background(), name).Execute()
+	id := resp.GetId()
+	require.NotEmpty(tb, id, "Project name not found: %s", name)


yeah I see internally does fmt.Sprintf(msgAndArgs[0].(string), msgAndArgs[1:]...) I assumed this was closer to t.Error vs t.Errorf 🤷

gssbzn · 2024-03-13T15:29:49Z

internal/testutil/acc/atlas.go

+func deleteProject(id string) {
+	_, _, err := ConnV2().ProjectsApi.DeleteProject(context.Background(), id).Execute()
+	if err != nil {
+		fmt.Printf("Project deletion failed: %s, error: %s", id, err)


I see, this may make this approach a bit noisy on the logs since you usually only want logs when running on verbose mode (-v) and right now with printf is harder to control

EspenAlbert · 2024-03-13T15:48:00Z

internal/testutil/acc/shared_resource.go

+	defer sharedInfo.mu.Unlock()
+
+	// lazy creation so it's only done if really needed
+	if sharedInfo.projectID == "" {


Maybe support os.Getenv("MONGODB_ATLAS_PROJECT_ID") similar to acc.GetClusterInfo?

I think that is a good idea, but i'm going to do in a next PR so this doesn't get more complicated, thx!

AgustinBettati

LGTM, thanks for followups simplifying this initial approach

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

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

lantoli commented Mar 12, 2024

View reviewed changes

marcosuma reviewed Mar 12, 2024

View reviewed changes

lantoli commented Mar 13, 2024

View reviewed changes

marcosuma reviewed Mar 13, 2024

View reviewed changes

AgustinBettati reviewed Mar 13, 2024

View reviewed changes

gssbzn reviewed Mar 13, 2024

View reviewed changes

lantoli added 4 commits March 13, 2024 15:21

execution project

d515f8b

no need to empty these values

68030d6

simplify TestMainExecution conditions

1d0fe09

address feedback

c29b2ce

lantoli force-pushed the CLOUDP-237263_project_test_execution branch from b7c7488 to c29b2ce Compare March 13, 2024 14:41

lantoli commented Mar 13, 2024

View reviewed changes

gssbzn reviewed Mar 13, 2024

View reviewed changes

add comments

1a1f21c

gssbzn reviewed Mar 13, 2024

View reviewed changes

gssbzn approved these changes Mar 13, 2024

View reviewed changes

EspenAlbert reviewed Mar 13, 2024

View reviewed changes

clarify ProjectIDExecution behavior

d6bb0d6

AgustinBettati approved these changes Mar 13, 2024

View reviewed changes

lantoli merged commit e054e18 into master Mar 13, 2024
45 checks passed

lantoli deleted the CLOUDP-237263_project_test_execution branch March 13, 2024 17:17


		exitCode := m.Run()

		if !InUnitTest() && atlasInfo.needsDeletion {

	require.NotNil(tb, "Project creation failed: %s, org not set", name)
	require.NotNilf(tb, "Project creation failed: %s, org not set", name)

	require.NoError(tb, err, "Project creation failed: %s, err: %s", name, err)
	require.NoErrorf(tb, err, "Project creation failed: %s, err: %s", name, err)

	require.NotEmpty(tb, id, "Project creation failed: %s", name)
	require.NotEmptyf(tb, id, "Project creation failed: %s", name)

	fmt.Printf("Project deletion failed: %s, error: %s", id, err)
	tb.Logf("Project deletion failed: %s, error: %s", id, err)

chore: Creates project for test execution #2010

chore: Creates project for test execution #2010

Conversation

lantoli commented Mar 12, 2024 • edited

Description

Type of change:

Required Checklist:

Further comments

lantoli Mar 12, 2024 • edited

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 Mar 13, 2024 • edited

Choose a reason for hiding this comment

lantoli Mar 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maastha Mar 13, 2024 • edited

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 Mar 12, 2024 • edited

Choose a reason for hiding this comment

lantoli Mar 13, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gssbzn Mar 13, 2024 • edited

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

lantoli commented Mar 12, 2024 •

edited

lantoli Mar 12, 2024 •

edited

AgustinBettati Mar 13, 2024 •

edited

lantoli Mar 13, 2024 •

edited

maastha Mar 13, 2024 •

edited

lantoli Mar 12, 2024 •

edited

lantoli Mar 13, 2024 •

edited

gssbzn Mar 13, 2024 •

edited