Skip to content

Test acm certificates transparency logs enabled#306

Open
prajwal-choudhari-comprinno wants to merge 5 commits intocomprinnotech:testcases_devfrom
prajwal-choudhari-comprinno:test_acm_certificates_transparency_logs_enabled
Open

Test acm certificates transparency logs enabled#306
prajwal-choudhari-comprinno wants to merge 5 commits intocomprinnotech:testcases_devfrom
prajwal-choudhari-comprinno:test_acm_certificates_transparency_logs_enabled

Conversation

@prajwal-choudhari-comprinno
Copy link

@prajwal-choudhari-comprinno prajwal-choudhari-comprinno commented Jun 27, 2025

Context

Adding automated test coverage for the ACM Certificates Transparency Logs check. This ensures that the check behaves correctly in various real-world scenarios, improves confidence in future changes, and maintains the integrity of the Tevico compliance engine.

Description

  • Added test_acm_certificates_transparency_logs_enabled.py to cover the following scenarios:

    • No ACM certificates present (NOT_APPLICABLE)
    • Certificates with transparency logging enabled (PASSED)
    • Certificates with transparency logging disabled (FAILED)
    • AWS ClientError (e.g., access denied) during API call (UNKNOWN)
  • Utilized unittest.mock for Boto3 client mocking.

  • Validated that summaries and status results are as expected for each test case.

No new dependencies were introduced.

Checklist

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

assert report.resource_ids_status[0].summary is not None
# Accept any error message, just check it's present
assert report.resource_ids_status[0].summary

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Add a test for exception during describe_certificate to improve failure path coverage.
  2. Remove unnecessary @patch("boto3.Session.client") decorators to simplify test setup.
  3. Include explicit assertions on reported resource.arn for completeness.

Copy link
Contributor

Choose a reason for hiding this comment

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

NoSuchEntityException or exception during describe_certificate not tested.

ADD

def test_describe_certificate_exception(self):
   self.mock_acm.list_certificates.return_value = {
       "CertificateSummaryList": [{"CertificateArn": "arn:aws:acm:region:account:certificate/bad-cert"}]
   }
   self.mock_acm.describe_certificate.side_effect = ClientError(
       {"Error": {"Code": "AccessDenied"}}, "DescribeCertificate"
   )

   report = self.check.execute(self.mock_session)
   assert report.status == CheckStatus.FAILED
   assert report.resource_ids_status[0].status == CheckStatus.UNKNOWN
   assert "Error describing certificate" in report.resource_ids_status[0].summary

Choose a reason for hiding this comment

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

fixed

assert report.status == CheckStatus.FAILED
assert report.resource_ids_status[0].summary is not None
assert "Error describing certificate" in report.resource_ids_status[0].summary
assert report.resource_ids_status[0].resource.arn == cert_arn
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue Suggestion Why It Matters
❌ Pagination not tested Add a test simulating multiple pages using NextToken. The check explicitly handles paginated results, so it must be verified.
⚠️ Exception detail not asserted Check that exception=str(e) is correctly captured in ResourceStatus. Makes sure root cause is preserved in the report for observability/debugging.
🔍 Summary assertion is vague Prefer assert summary == ... over in ... where possible. Ensures test fails if format/content changes unintentionally.
ℹ️ Redundant mock STS client The STS client is mocked but never used in this check. Can be removed to avoid confusion and reduce test noise.

✅ Additional Test You Should Add

1. Pagination Test

def test_paginated_certificates_list(self):
    """Test paginated ACM certificate list."""
    cert_arn_1 = "arn:aws:acm:region:account:certificate/cert-1"
    cert_arn_2 = "arn:aws:acm:region:account:certificate/cert-2"
# First page with NextToken
self.mock_acm.list_certificates.side_effect = [
    {"CertificateSummaryList": [{"CertificateArn": cert_arn_1}], "NextToken": "token-1"},
    {"CertificateSummaryList": [{"CertificateArn": cert_arn_2}]}
]

self.mock_acm.describe_certificate.side_effect = lambda CertificateArn: {
    "Certificate": {"Options": {"CertificateTransparencyLoggingPreference": "ENABLED"}}
}

report = self.check.execute(self.mock_session)

assert report.status == CheckStatus.PASSED
assert len(report.resource_ids_status) == 2
assert all(r.status == CheckStatus.PASSED for r in report.resource_ids_status)

2. Exception Message Assertion

Update this in test_client_error:

assert report.resource_ids_status[0].exception is not None
assert "AccessDenied" in report.resource_ids_status[0].exception

Choose a reason for hiding this comment

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

fixed

prajwal-choudhari-comprinno added 2 commits July 16, 2025 13:04
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