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.

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

doc: Documents testing best practices #2132

Merged
merged 30 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
31f2b9f
split development and testing contributing guides
lantoli Apr 10, 2024
60cdb0f
test types
lantoli Apr 10, 2024
21b927e
local development
lantoli Apr 10, 2024
06670dd
acc and mig tests
lantoli Apr 10, 2024
b9aeb28
periods
lantoli Apr 10, 2024
24226c0
hoverfly in local
lantoli Apr 10, 2024
f9beae5
fix section headers
lantoli Apr 10, 2024
f51edd7
Update contributing/testing-best-practices.md
lantoli Apr 10, 2024
17ca19d
Update contributing/testing-best-practices.md
lantoli Apr 10, 2024
d1870b4
Update contributing/testing-best-practices.md
lantoli Apr 11, 2024
da20a1f
typo
lantoli Apr 11, 2024
2599b3a
extract Shared resources section
lantoli Apr 11, 2024
eb26202
table for local development
lantoli Apr 11, 2024
833db9b
`resource.TestCheckTypeSetElemNestedAttrs` or `resource.TestCheckType…
lantoli Apr 11, 2024
13ae246
use must
lantoli Apr 11, 2024
e677f7b
rephase separate
lantoli Apr 11, 2024
b4a9de6
links to acc & mig tests
lantoli Apr 11, 2024
09d9362
note about mocks
lantoli Apr 11, 2024
b3263a8
clarification added
lantoli Apr 11, 2024
ff3603e
explain import tests
lantoli Apr 11, 2024
2229962
rephrase plural ds
lantoli Apr 12, 2024
9dd870f
change section name Local testing
lantoli Apr 12, 2024
b4d1118
explain basic tests
lantoli Apr 12, 2024
5f47775
apply feedback
lantoli Apr 12, 2024
54a676b
more feedback
lantoli Apr 12, 2024
a1cbcbe
links
lantoli Apr 12, 2024
6916f9b
examples
lantoli Apr 12, 2024
3ea7551
Update contributing/testing-best-practices.md
lantoli Apr 12, 2024
feae31a
clarify common functions
lantoli Apr 12, 2024
94735af
Update contributing/testing-best-practices.md
lantoli Apr 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion contributing/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
Thanks for your interest in contributing to MongoDB Atlas Terraform Provider, the following documents define guidelines necessary to participate in the community.

- [Development Setup](development-setup.md)
- [Code and Test Best Practices](development-best-practices.md)
- [Development Best Practices](development-best-practices.md)
- [Testing Best Practices](testing-best-practices.md)
- [Documentation](documentation.md)
- [Changelog process](changelog-process.md)
- [Atlas SDK](atlas-sdk.md)
9 changes: 1 addition & 8 deletions contributing/development-best-practices.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

## Code and Test Best Practices
## Development Best Practices

## Table of Contents
- [Creating New Resource and Data Sources](#creating-new-resources-and-data-sources)
Expand All @@ -8,13 +8,6 @@

- Each resource (and associated data sources) is in a package in `internal/service`.
- There can be multiple helper files and they can also be used from other resources, e.g. `common_advanced_cluster.go` defines functions that are also used from other resources using `advancedcluster.FunctionName`.
- Unit and Acceptances tests are in the same `_test.go` file. They are not in the same package as the code tests, e.g. `advancedcluster` tests are in `advancedcluster_test` package so coupling is minimized.
- Migration tests are in `_migration_test.go` files.
- Helper methods must have their own tests, e.g. `common_advanced_cluster_test.go` has tests for `common_advanced_cluster.go`.
- `internal/testutils/acc` contains helper test methods for Acceptance and Migration tests.
- Tests that need the provider binary like End-to-End tests don’t belong to the source code packages and go in `test/e2e`.
- [Testify Mock](https://pkg.go.dev/github.com/stretchr/testify/mock) and [Mockery](https://github.com/vektra/mockery) are used for test doubles in Atlas Go SDK unit tests.


### Creating New Resource and Data Sources

Expand Down
60 changes: 60 additions & 0 deletions contributing/testing-best-practices.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@

# Testing Best Practices
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment for the different sections so leaving it here. Can you add examples to our current tests when describing the guidelines? You can even pick a resource and always refer to it

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean for instance when describing variable MONGODB_ATLAS_PROJECT_ID, function ProjectIDExecution, etc. to link to some part in the code?

if that's the case I'm not sure I would do that because it's easy that links to code get outdated when we change the code/tests.

I think it's better that people just search for words like MONGODB_ATLAS_PROJECT_ID in the code to see how it can be used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I left a couple of cases below of where to leave an example.

it's easy that links to code get outdated when we change the code/tests.
Not properly, you can make a link to a file by fixing the commit. Also if an example ever gets outdated it means also the guideline is outdated, so it doesn't really create an extra effort.

Rationale is to think: how can I speed up my team to implement the right practices and "see" what I mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

adding examples


## Types of test

- Unit tests: In Terraform terminology they refer to tests that [validate a resource schema](https://developer.hashicorp.com/terraform/plugin/framework/handling-data/schemas#unit-testing). That is done automatically [here](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/internal/provider/provider_test.go) for all resources and data sources using Terraform Framework Plugin. Additionally, we have general unit testing for testing a resource or unit without calling external systems like MongoDB Atlas.
- Acceptance (acc) tests: In Terraform terminology they refer to the use of real Terraform configurations to exercise the code in plan, apply, refresh, and destroy life cycles (real infrastructure resources are created as part of the test), more info [here](https://developer.hashicorp.com/terraform/plugin/testing/acceptance-tests).
- Migration (mig) tests: These tests are designed to ensure that after an upgrade to a new Atlas provider version, user configs do not result in unexpected plan changes, more info [here](https://developer.hashicorp.com/terraform/plugin/framework/migrating/testing). Migration tests are a subset of Acceptance tests.

## File structure
lantoli marked this conversation as resolved.
Show resolved Hide resolved
- A resource and associated data sources are implemented in a folder that is also a Go package, e.g. `advancedcluster` implementation is in [`internal/service/advancedcluster`](https://github.com/mongodb/terraform-provider-mongodbatlas/tree/master/internal/service/advancedcluster)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- A resource and associated data sources are implemented in a folder that is also a Go package, e.g. `advancedcluster` implementation is in [`internal/service/advancedcluster`](https://github.com/mongodb/terraform-provider-mongodbatlas/tree/master/internal/service/advancedcluster)
- A resource and associated data sources are implemented in a folder that is also a Go package, e.g. `advancedcluster` implementation is in [`internal/service/advancedcluster`](https://github.com/mongodb/terraform-provider-mongodbatlas/tree/f3ff5bb678c1b07c16cc467471f483e483565427/internal/service/advancedcluster)

Copy link
Collaborator

Choose a reason for hiding this comment

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

feel free to discard if there was a rationale for non using permalink, I think it's better though

Copy link
Member Author

@lantoli lantoli Apr 12, 2024

Choose a reason for hiding this comment

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

not a strong opinion, for links that are that not probably going to change like this one (resource folder) i might use master, and from that link you can navigate through the latest advanced_cluster code

Copy link
Member Author

Choose a reason for hiding this comment

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

similar for main_test.go, if the link to that file gets broken it's really because we've changed the convention and we should have updated the doc

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @gssbzn for all your comments. as discussed offline i'll merge the PR

- We enforce "black box" testing, tests must be in a separate "_test" package, e.g. `advancedcluster` tests are in `advancedcluster_test` package.
- Acceptance and general unit tests are in corresponding `_test.go` file as the resource or data source source file. If business logic is extracted into a separate file, unit testing for that logic will be including in its associated `_test.go` file, e.g. [state_transition_search_deployment_test.go](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/searchdeployment/state_transition_search_deployment_test.go).
- Migration tests are in `_migration_test.go` files.
- All resource folders must have a `main_test.go` file to handle resource reuse lifecycle, e.g. [here](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/internal/service/advancedcluster/main_test.go).
lantoli marked this conversation as resolved.
Show resolved Hide resolved
- Helper methods must have their own tests, e.g. `common_advanced_cluster_test.go` has tests for `common_advanced_cluster.go`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this covers two things that are not really related, should helper methods have a dedicated file? why? go doesn't care a package can be all a single file and organization is subjective. Then there's the actual method having tests which I find a bit contradictory with blackbox testing, I would expect helper methods to be unexported and if you enforce black box testing then untested

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

"helper methods" is probably not the best name for what we mean here. They are functions (for example flatteners and expander to go from/to TF schema / Atlas SDK). They're normally used across the resource, data source and plural data source so it makes sense that they are not in the resource file for instance. also some might even be used from other resources.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gssbzn let me know it's better now: feae31a

- `internal/testutils/acc` contains helper test functions for Acceptance tests.
- `internal/testutils/mig` contains helper test functions specifically for Migration tests.
Comment on lines +17 to +18
Copy link
Collaborator

Choose a reason for hiding this comment

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

not for these PR but let's avoid utils in package names please

Copy link
Member Author

@lantoli lantoli Apr 12, 2024

Choose a reason for hiding this comment

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

i know, the problem is that test was confusing in this context because it doesn't have tests, and i wanted to group in a parent to acc, mig (and replay). any suggestion to improve this package name?

Copy link
Member Author

Choose a reason for hiding this comment

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

it'd be similar to httptest in Go library but i couldn´t find a good name for the "http" part

- `internal/testutils/replay` contains helper test functions for [Hoverfly](https://docs.hoverfly.io/en/latest/). Hoverfly is used to capture and replay HTTP traffic with MongoDB Atlas.

## Unit tests
lantoli marked this conversation as resolved.
Show resolved Hide resolved

- Unit tests must not create Terraform resources or use external systems, e.g unit tests using [Atlas Go SDK](https://github.com/mongodb/atlas-sdk-go) must not call MongoDB Atlas.
- Mock of specific interfaces like `admin.ProjectsApi` is prefered to use the whole `client`, e.g. [resource_project.go](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/project/resource_project.go#L792)
- [Testify Mock](https://pkg.go.dev/github.com/stretchr/testify/mock) is used for test doubles.
- Altlas Go SDK mocked interfaces are generated in [mockadmin](https://github.com/mongodb/atlas-sdk-go/tree/main/mockadmin) package using [Mockery](https://github.com/vektra/mockery), example of use in [resource_project_test.go](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/project/resource_project_test.go#L114).

## Acceptance tests

- There must be at least one `basic acceptance test` for each resource, e.g.: [TestAccSearchIndex_basic](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/searchindex/resource_search_index_test.go#L14). They test the happy path with minimum resource configuration.
- `Basic import tests` are done as the last step in the `basic acceptance tests`, not as a different test, e.g. [basicTestCase](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/searchindex/resource_search_index_test.go#L211). Exceptions apply for more specific import tests, e.g. testing with incorrect IDs. [Import tests](https://developer.hashicorp.com/terraform/plugin/sdkv2/resources/import#resource-acceptance-testing-implementation) verify that the [Terraform Import](https://developer.hashicorp.com/terraform/cli/import) functionality is working fine.
- Data sources are tested in the same tests as the resources, e.g. [commonChecks](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/searchindex/resource_search_index_test.go#L262-L263).
- Helper functions such as `resource.TestCheckTypeSetElemNestedAttrs` or `resource.TestCheckTypeSetElemAttr` can be used to check resource and data source attributes more easily, e.g. [resource_serverless_instance_test.go](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/serverlessinstance/resource_serverless_instance_test.go#L61).

## Migration tests

- There must be at least one `basic migration test` for each resource that leverages on the `basic acceptance tests` using helper test functions such as `CreateAndRunTest`, e.g. [TestMigServerlessInstance_basic](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/serverlessinstance/resource_serverless_instance_migration_test.go#L10).

## Local testing

These enviroment variables can be used in local to speed up development process.

Enviroment Variable | Description
--- | ---
`MONGODB_ATLAS_PROJECT_ID` | Re-use an existing project reducing test run duration for resources supporting this variable
`MONGODB_ATLAS_CLUSTER_NAME` | Re-use an existing cluster reducing significantly test run duration for resources supporting this variable
`REPLAY_MODE` | Use [Hoverfly](https://docs.hoverfly.io/en/latest/), more info about possible variable values [here](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/contributing/development-setup.md#replaying-http-requests-with-hoverfly)

## Shared resources

Acceptance and migration tests can reuse projects and clusters in order to be more efficient in resource utilization.

- A project can be reused using `ProjectIDExecution`. It returns the ID of a project that is created for the current execution of tests for a resource, e.g. [TestAccConfigRSDatabaseUser_basic](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/databaseuser/resource_database_user_test.go#L24).
- As the project is shared for all tests for a resource, sometimes tests can affect each other if they're using global resources to the project (e.g. network peering, maintenance window or LDAP config). In that case:
- Run the tests in serial (`resource.Test` instead of `resource.ParallelTest`) if the tests are fast and saving resources is prefered, e.g. [TestAccConfigRSProjectAPIKey_multiple](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/projectapikey/resource_project_api_key_test.go#L149).
- Don’t use `ProjectIDExecution` and create a project for each test if a faster test execution is prefered even if more resources are needed, e.g. [TestAccFederatedDatabaseInstance_basic](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/federateddatabaseinstance/resource_federated_database_instance_test.go#L19).
- A cluster can be reused using `ClusterNameExecution`. This function returns the project id (created with `ProjectIDExecution`) and the name of a cluster that is created for the current execution of tests for a resource, e.g. [TestAccSearchIndex_withSearchType](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/searchindex/resource_search_index_test.go#L20). Similar precautions to project reuse must be taken here. If a global resource to cluster is being tested (e.g. cluster global config) then it's prefered to run tests in serial or create their own clusters.
- Plural data sources can be challenging to test when tests run in parallel or they share projects and/or clusters.
- Avoid checking for a specific total count as other tests can also create resources, e.g. [resource_network_container_test.go](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/networkcontainer/resource_network_container_test.go#L214).
- Don't assume results are in a certain order, e.g. [resource_network_container_test.go](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/networkcontainer/resource_network_container_test.go#L215).