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

Unit test cleanup #961

Conversation

tremble
Copy link
Contributor

@tremble tremble commented Aug 10, 2022

SUMMARY

Speaking to @mattclay, pytest based unit tests are generally considered preferred over unittest based unit tests. For the sake of having "good" examples in amazon.aws, migrates unittest based tests over to pytest

Additionally:

  • Moves tests about to reflect module_utils
  • Cleans up the boto3/botocore test skipping
  • uses "pytest.raises" rather than try/except blocks
  • Cleans up unused variables
  • Cleans up unused imports
  • fixes s3_object unit test (was trying to import from the old location, redirects don't handle this)
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

tests/units

ADDITIONAL INFORMATION

@tremble tremble requested review from alinabuzachis, abikouo, jatorcasso, goneri and jillr and removed request for abikouo August 10, 2022 13:09
@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request module module needs_maintainer needs_triage new_plugin New plugin tests tests labels Aug 10, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 42s
✔️ build-ansible-collection SUCCESS in 5m 11s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 12m 28s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 19s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 9m 34s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 23s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 54s

Copy link
Collaborator

@alinabuzachis alinabuzachis left a comment

Choose a reason for hiding this comment

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

Nice!

@tremble tremble force-pushed the tests/unittest/cleanup branch from 84d7e57 to 8c41b67 Compare August 10, 2022 13:35
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 3m 43s
✔️ build-ansible-collection SUCCESS in 5m 04s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 8m 50s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 04s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 7m 52s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 7m 26s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 30s

@tremble tremble added the mergeit Merge the PR (SoftwareFactory) label Aug 10, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

✔️ ansible-galaxy-importer SUCCESS in 5m 04s
✔️ build-ansible-collection SUCCESS in 5m 10s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 8m 25s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 49s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 8m 18s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 14s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 5m 53s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 898ade5 into ansible-collections:main Aug 10, 2022
@tremble tremble added the backport-4 PR should be backported to the stable-4 branch label Aug 10, 2022
@patchback
Copy link

patchback bot commented Aug 10, 2022

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/898ade59bfa6ca5692303f8bc20eefde0734ba94/pr-961

Backported as #962

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Aug 10, 2022
Unit test cleanup

SUMMARY
Speaking to @mattclay, pytest based unit tests are generally considered preferred over unittest based unit tests.  For the sake of having "good" examples in amazon.aws, migrates unittest based tests over to pytest
Additionally:

Moves tests about to reflect module_utils
Cleans up the boto3/botocore test skipping
uses "pytest.raises" rather than try/except blocks
Cleans up unused variables
Cleans up unused imports
fixes s3_object unit test (was trying to import from the old location, redirects don't handle this)

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
tests/units
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>
(cherry picked from commit 898ade5)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Aug 10, 2022
[PR #961/898ade59 backport][stable-4] Unit test cleanup

This is a backport of PR #961 as merged into main (898ade5).
SUMMARY
Speaking to mattclay, pytest based unit tests are generally considered preferred over unittest based unit tests.  For the sake of having "good" examples in amazon.aws, migrates unittest based tests over to pytest
Additionally:

Moves tests about to reflect module_utils
Cleans up the boto3/botocore test skipping
uses "pytest.raises" rather than try/except blocks
Cleans up unused variables
Cleans up unused imports
fixes s3_object unit test (was trying to import from the old location, redirects don't handle this)

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
tests/units
ADDITIONAL INFORMATION

Reviewed-by: Mark Chappell <None>
softwarefactory-project-zuul bot pushed a commit to ansible-collections/community.aws that referenced this pull request Aug 17, 2022
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections/amazon.aws#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>
@tremble tremble deleted the tests/unittest/cleanup branch September 9, 2022 08:50
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Sep 16, 2022
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@8b4afa9
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Sep 16, 2022
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@8b4afa9
goneri pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Sep 21, 2022
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@8b4afa9
goneri pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Sep 21, 2022
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@8b4afa9
goneri pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Sep 21, 2022
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@8b4afa9
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Sep 7, 2023
…ible-collections#961)

IAM Role Removal Does Not Require Removal of Permission Boundary

SUMMARY

Removes unnecessary removal of permission boundary from a role when deleting a role. Unlike inline policies, permission boundaries do not need to be removed from an IAM role before deleting the IAM role. This behavior causes issues when a permission boundary is inherited that prevents removal of the permission boundary.
Fixes ansible-collections#959

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
iam_role

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@e670b34
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Sep 7, 2023
…ible-collections#961)

IAM Role Removal Does Not Require Removal of Permission Boundary

SUMMARY

Removes unnecessary removal of permission boundary from a role when deleting a role. Unlike inline policies, permission boundaries do not need to be removed from an IAM role before deleting the IAM role. This behavior causes issues when a permission boundary is inherited that prevents removal of the permission boundary.
Fixes ansible-collections#959

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
iam_role

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@e670b34
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…ible-collections#961)

IAM Role Removal Does Not Require Removal of Permission Boundary

SUMMARY

Removes unnecessary removal of permission boundary from a role when deleting a role. Unlike inline policies, permission boundaries do not need to be removed from an IAM role before deleting the IAM role. This behavior causes issues when a permission boundary is inherited that prevents removal of the permission boundary.
Fixes ansible-collections#959

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
iam_role

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…ible-collections#961)

IAM Role Removal Does Not Require Removal of Permission Boundary

SUMMARY

Removes unnecessary removal of permission boundary from a role when deleting a role. Unlike inline policies, permission boundaries do not need to be removed from an IAM role before deleting the IAM role. This behavior causes issues when a permission boundary is inherited that prevents removal of the permission boundary.
Fixes ansible-collections#959

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
iam_role

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 20, 2023
…ible-collections#961)

IAM Role Removal Does Not Require Removal of Permission Boundary

SUMMARY

Removes unnecessary removal of permission boundary from a role when deleting a role. Unlike inline policies, permission boundaries do not need to be removed from an IAM role before deleting the IAM role. This behavior causes issues when a permission boundary is inherited that prevents removal of the permission boundary.
Fixes ansible-collections#959

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
iam_role

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@e670b34
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Oct 6, 2023
…ible-collections#961)

IAM Role Removal Does Not Require Removal of Permission Boundary

SUMMARY

Removes unnecessary removal of permission boundary from a role when deleting a role. Unlike inline policies, permission boundaries do not need to be removed from an IAM role before deleting the IAM role. This behavior causes issues when a permission boundary is inherited that prevents removal of the permission boundary.
Fixes ansible-collections#959

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
iam_role

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@e670b34
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Oct 6, 2023
…ible-collections#961)

IAM Role Removal Does Not Require Removal of Permission Boundary

SUMMARY

Removes unnecessary removal of permission boundary from a role when deleting a role. Unlike inline policies, permission boundaries do not need to be removed from an IAM role before deleting the IAM role. This behavior causes issues when a permission boundary is inherited that prevents removal of the permission boundary.
Fixes ansible-collections#959

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
iam_role

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@e670b34
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 20, 2023
…ible-collections#961)

IAM Role Removal Does Not Require Removal of Permission Boundary

SUMMARY

Removes unnecessary removal of permission boundary from a role when deleting a role. Unlike inline policies, permission boundaries do not need to be removed from an IAM role before deleting the IAM role. This behavior causes issues when a permission boundary is inherited that prevents removal of the permission boundary.
Fixes ansible-collections#959

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
iam_role

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@e670b34
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
…ible-collections#961)

IAM Role Removal Does Not Require Removal of Permission Boundary

SUMMARY

Removes unnecessary removal of permission boundary from a role when deleting a role. Unlike inline policies, permission boundaries do not need to be removed from an IAM role before deleting the IAM role. This behavior causes issues when a permission boundary is inherited that prevents removal of the permission boundary.
Fixes ansible-collections#959

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
iam_role

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 19, 2024
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@8b4afa9
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Oct 16, 2024
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@8b4afa9
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Oct 16, 2024
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@8b4afa9
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Oct 24, 2024
Minor linting fixups - 2022-08-12

SUMMARY
Various linting and unit test fixups

unused variables
overly broad Exception catching (highlighted some broken tests)
removes direct use of unittest in favour of pytest (see also ansible-collections#961)
cleans up skipping of tests when botocore/boto3 aren't installed
passes error message from VPNConnectionException into its super to make testing easier, should never be directly exposed to the user

Removes tests for 3 modules which now have integration tests, they're either recording based (fragile)or test things which are separately tested in the integration tests.

lambda
s3_bucket_notifications
route53_zone

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/ec2_vpc_vpn.py
tests/unit
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@8b4afa9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-4 PR should be backported to the stable-4 branch community_review feature This issue/PR relates to a feature request mergeit Merge the PR (SoftwareFactory) module module needs_maintainer needs_triage new_plugin New plugin tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants