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

chore: Reuses project in tests for network_container resource #2039

Merged
merged 13 commits into from Mar 19, 2024
Merged

chore: Reuses project in tests for network_container resource #2039

merged 13 commits into from Mar 19, 2024

Conversation

lantoli
Copy link
Member

@lantoli lantoli commented Mar 18, 2024 •
edited

Description

Reuses project in tests for network_container resource.

The data sources are tested with the test resources so no need to create the same resources twice.

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

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 lantoli force-pushed the CLOUDP-237991_network_container branch from ab08357 to b53628e Compare March 18, 2024 11:36
@lantoli lantoli marked this pull request as ready for review March 18, 2024 17:58
@lantoli lantoli requested a review from a team as a code owner March 18, 2024 17:58
marcosuma
marcosuma reviewed Mar 19, 2024
View reviewed changes
internal/service/networkcontainer/resource_network_container_test.go
Comment on lines +44 to +52
resource.TestCheckResourceAttrSet(dataSourceName, "project_id"),
resource.TestCheckResourceAttr(dataSourceName, "provider_name", constant.AWS),
resource.TestCheckResourceAttrSet(dataSourceName, "provisioned"),

resource.TestCheckResourceAttrWith(dataSourcePluralName, "results.#", acc.IntGreatThan(0)),
resource.TestCheckResourceAttrSet(dataSourcePluralName, "results.0.id"),
resource.TestCheckResourceAttrSet(dataSourcePluralName, "results.0.atlas_cidr_block"),
resource.TestCheckResourceAttrSet(dataSourcePluralName, "results.0.provider_name"),
resource.TestCheckResourceAttrSet(dataSourcePluralName, "results.0.provisioned"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a trend we're going to use in all resources? testing data sources within the resource test? Or is there something special about this?

Depending on the answer: could we have some comments? (always having in mind readability)

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's not really a new pattern, we have both approaches depending on the resource. We do the same tests to the resource and datasource (checking the same attributes) and we create the same resource to test the datasource as the "resource basic test".
i'll take it to the tech meeting

Copy link
Collaborator

Choose a reason for hiding this comment

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

A small detail to consider it that when we have acceptance tests that test both the resource and data source you will notice that the test name does not include TestAcc*RS_*, and instead defines TestAcc*_* (example)

Copy link
Member Author

Choose a reason for hiding this comment

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

thx, good point

marcosuma
marcosuma approved these changes Mar 19, 2024
View reviewed changes
Copy link
Collaborator

@marcosuma marcosuma left a comment

Choose a reason for hiding this comment

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

LGTM - only one minor comment regarding this new pattern of testing data source into resource

lantoli
lantoli commented Mar 19, 2024
View reviewed changes
internal/service/networkcontainer/resource_network_container_test.go
resource.TestCheckResourceAttr(dataSourceName, "provider_name", constant.AWS),
resource.TestCheckResourceAttrSet(dataSourceName, "provisioned"),

resource.TestCheckResourceAttrWith(dataSourcePluralName, "results.#", acc.IntGreatThan(0)),
Copy link
Member Author

Choose a reason for hiding this comment

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

@marcosuma this follows your idea of doing more generic tests on plural ds (like checking that there is more than 1 as opposed to to a fix number) so they can run in parallel

@lantoli lantoli merged commit 9f95800 into master Mar 19, 2024
47 checks passed
@lantoli lantoli deleted the CLOUDP-237991_network_container branch March 19, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AgustinBettati AgustinBettati AgustinBettati left review comments

@marcosuma marcosuma marcosuma approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lantoli @marcosuma @AgustinBettati