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.

Sign up for GitHub

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

Jump to bottom

doc: Updates contributing file with code and test best practices #1666

Merged
merged 4 commits into from
Nov 27, 2023
Merged

doc: Updates contributing file with code and test best practices #1666

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48b7b51
update contributing
lantoli Nov 27, 2023
43adc9c
Update CONTRIBUTING.md
lantoli Nov 27, 2023
90ff239
Update CONTRIBUTING.md
lantoli Nov 27, 2023
3a6e40b
Update CONTRIBUTING.md
lantoli Nov 27, 2023
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
10 changes: 9 additions & 1 deletion CONTRIBUTING.md
View file
Open in desktop
lantoli marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,15 @@ To do this you can:

- Lastly, in the terminal run `terraform init` again and this time terraform will pull provider version from `tf_cache` network mirror. You can confirm this by noting the `Terraform has been successfully initialized! Using mongodb/mongodbatlas Vx.x.x from the shared cache directory` message.


## Code and Test Best Practices
lantoli marked this conversation as resolved.
Show resolved Hide resolved

- Each resource (and associated data sources) is in a package in `internal/service`.
lantoli marked this conversation as resolved.
Show resolved Hide resolved
- There can be multiple helper classes 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`.
lantoli marked this conversation as resolved.
Show resolved Hide resolved
- 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.
Copy link
Member

@wtrocki wtrocki Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Format for brevity consider adding chapters:

Suggested change
- 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.
## Code Changes
...
## Unit and Acceptance tests

Contributing is usually focused on guiding new person to do specific tasks.

Instead of

  • 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.

Consider:

Create *_test.go file for both Unit and Acceptance tests.
For example see: link.

Consider reader empathy:

package so coupling is minimized.

do we need that info for the new contributions?

Copy link
Member

Choose a reason for hiding this comment

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

Those aren't blocking PR. Just some nits and CONTRIBUTING.md format tips for future.

- Migration tests are in `_migration_test.go` files.
- Helper methods can have their own tests, e.g. `common_advanced_cluster_test.go` has tests for `common_advanced_cluster.go`.
lantoli marked this conversation as resolved.
Show resolved Hide resolved
- `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 a different structure.
lantoli marked this conversation as resolved.
Show resolved Hide resolved

## Documentation Best Practises

Expand Down