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

Improve unit test coverage of module_utils.policy #1136

Conversation

tremble
Copy link
Contributor

@tremble tremble commented Oct 7, 2022

SUMMARY
  • Improve unit test coverage of module_utils.policy
  • Slight refactor of _hashable_policy to reduce complexity (and make testing easier)
  • Deprecates sort_json_policy_dict, it's very naive and was only used by ecs_ecr
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/module_utils/policy.py

ADDITIONAL INFORMATION

@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request module_utils module_utils needs_triage plugins plugin (any type) tests tests labels Oct 7, 2022
@softwarefactory-project-zuul

This comment was marked as resolved.

@tremble tremble force-pushed the complexity/module_utils-policy branch from 2fa6797 to c1b77b6 Compare October 7, 2022 12:36
@tremble tremble requested a review from goneri October 7, 2022 12:38
@softwarefactory-project-zuul

This comment was marked as resolved.

@tremble tremble force-pushed the complexity/module_utils-policy branch from c1b77b6 to af4171a Compare October 7, 2022 13:13
@softwarefactory-project-zuul

This comment was marked as outdated.

@softwarefactory-project-zuul

This comment was marked as outdated.

@tremble
Copy link
Contributor Author

tremble commented Oct 7, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 13s
✔️ build-ansible-collection SUCCESS in 5m 22s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 8m 28s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 43s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 10m 31s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 7m 30s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 41s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 54s
✔️ cloud-tox-py3 SUCCESS in 3m 44s
✔️ ansible-test-splitter SUCCESS in 3m 10s
⚠️ integration-amazon.aws-1 SKIPPED
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
✔️ integration-community.aws-1 SUCCESS in 5m 53s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 34s

if isinstance(tupleified, list):
tupleified = tuple(tupleified)
hashed_policy = _hashable_policy(each, [])
tupleified = _tuplify_list(hashed_policy)
policy_list.append(tupleified)
elif isinstance(policy, string_types) or isinstance(policy, binary_type):
Copy link
Member

Choose a reason for hiding this comment

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

You can write this like this: elif isinstance(policy, (binary_type, string_types)):

>>> a = True
>>> isinstance(a, (int, bool))
True
>>> a = 1
>>> isinstance(a, (int, bool))
True
>>> a = "a string"
>>> isinstance(a, (int, bool))
False

Copy link
Member

@goneri goneri left a comment

Choose a reason for hiding this comment

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

Thanks, I'm always feel a bit intimidated by this _hashable_policy() function :-).

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

Build succeeded (gate pipeline).

✔️ ansible-galaxy-importer SUCCESS in 3m 56s
✔️ build-ansible-collection SUCCESS in 5m 01s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 9m 25s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 8m 45s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 8m 26s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 6m 25s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 28s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 5m 52s
✔️ cloud-tox-py3 SUCCESS in 3m 36s
✔️ ansible-test-splitter SUCCESS in 3m 08s
⚠️ integration-amazon.aws-1 SKIPPED
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-community.aws-1 SKIPPED
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 24s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 2d5b2e8 into ansible-collections:main Oct 10, 2022
@tremble tremble deleted the complexity/module_utils-policy branch October 21, 2022 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review feature This issue/PR relates to a feature request mergeit Merge the PR (SoftwareFactory) module_utils module_utils needs_triage plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants