Skip to content

Conversation

@prajwal-choudhari-comprinno

This PR adds unit tests for the cloudfront_access_logging_enabled check. It covers different scenarios to ensure the check behaves correctly for CloudFront distributions.

Test Cases Included:

No distributions present
Access logging is enabled
Access logging disabled
Client error from AWS
File Added:
library/aws/tests/cloudfront/test_cloudfront_access_logging_enabled.py

"""Test error handling when a ClientError occurs."""
self.mock_cf.list_distributions.side_effect = ClientError({"Error": {"Code": "AccessDenied"}}, "ListDistributions")
report = self.check.execute(self.mock_session)
assert report.status == CheckStatus.UNKNOWN
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. ❌ Missing Test Case for RealtimeLogConfigArn Enabled

    • The implementation checks if either Logging.Enabled or RealtimeLogConfigArn is present.

    • Missing test where:

      • Logging.Enabled = False

      • RealtimeLogConfigArn is set
        ➤ This path is untested and may introduce blind spots.


  1. ⚠️ No Test Case for Exception in get_distribution_config

    • You handle exceptions in get_distribution_config() (per distribution), but no test simulates a failure at this level.
      ➤ Add a test to mock exception only for get_distribution_config.


  1. ⚠️ ClientError Test Only Covers list_distributions

    • While the ClientError is tested for list_distributions, there’s no test where get_distribution_config raises ClientError for one distribution.
      ➤ Add test for error during config fetch per distribution.


  1. ⚠️ Missing Mixed Scenario Test

    • No test checks for mixed results (e.g., one distribution with logging, one without).
      ➤ This is important to confirm how report.status behaves when some distributions pass and others fail.


✅ Improvements to Make

Issue Suggested Improvement
Missing Realtime log check Add a test where RealtimeLogConfigArn is enabled and legacy logging is disabled
No partial failure test Add a test where get_distribution_config fails for one distribution
Only one error type tested Add ClientError or general exception test for get_distribution_config
No mixed outcome Add test with multiple distributions with mixed logging states

Choose a reason for hiding this comment

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

cover all the points

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants