Skip to content

Conversation

@robmorgan
Copy link
Owner

Summary

Adds full BDD testing infrastructure for AWS Lambda, enabling Gherkin-based tests for Lambda functions with the Virtual Cloud emulator.

New Files

Core Implementation

  • pkg/awshelpers/lambda.go - Lambda SDK client helper with Virtual Cloud endpoint support
  • pkg/assertions/aws/lambda.go - LambdaAsserter interface with 15+ assertion methods
  • pkg/steps/aws/lambda.go - 25+ Gherkin step definitions

Terraform Examples

  • examples/aws/lambda/basic-function/ - Basic Lambda with configurable runtime, handler, timeout, memory
  • examples/aws/lambda/function-with-alias/ - Lambda with published version and alias
  • examples/aws/lambda/function-url/ - Lambda with public or IAM-authenticated function URL

Feature Files

  • features/aws/lambda/lambda_function.feature - Basic function creation and configuration tests
  • features/aws/lambda/lambda_alias.feature - Version and alias tests
  • features/aws/lambda/lambda_function_url.feature - Function URL tests

Supported Assertions

Category Assertions
Existence AssertFunctionExists, AssertFunctionNotExists
Configuration Runtime, Handler, Timeout, Memory, Environment Variables
Versions & Aliases Version exists, Alias exists, Alias points to version
Function URLs URL exists, Auth type (NONE/AWS_IAM)
Layers Function has layer
Event Source Mapping exists

Test plan

  • go build ./... passes
  • make go-test-cover passes
  • Feature tests with ./bin/infraspec features/aws/lambda/ --virtual-cloud

🤖 Generated with Claude Code

- Add LambdaAsserter interface and implementation with assertions for:
  - Function existence and configuration (runtime, handler, timeout, memory)
  - Environment variables
  - Versions and aliases
  - Function URLs and auth types
  - Layers and event source mappings
- Add Lambda helper in awshelpers for SDK client creation
- Add Gherkin step definitions for all Lambda assertions
- Add Terraform examples:
  - basic-function: Simple Lambda with configurable runtime/handler
  - function-with-alias: Lambda with published version and alias
  - function-url: Lambda with public/IAM-authenticated URL
- Add feature files for testing Lambda scenarios

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@vercel
Copy link
Contributor

vercel bot commented Dec 29, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
infraspec Ready Ready Preview, Comment Dec 29, 2025 10:26am

@claude
Copy link
Contributor

claude bot commented Dec 29, 2025

Pull Request Review: Lambda BDD Testing Support

This PR adds comprehensive Lambda testing support with well-structured assertions, step definitions, and Terraform examples. The implementation follows the established patterns in the codebase. Below is my detailed review:


Strengths

  1. Excellent Code Organization: The implementation follows the project's established patterns perfectly, with clear separation between assertions (pkg/assertions/aws/lambda.go), step definitions (pkg/steps/aws/lambda.go), and helpers (pkg/awshelpers/lambda.go).

  2. Comprehensive Coverage: The PR provides 15+ assertion methods covering basic operations, configuration, versions/aliases, function URLs, layers, and event source mappings.

  3. Well-Structured Terraform Examples: Three different examples (basic-function, function-with-alias, function-url) demonstrate various Lambda use cases with proper IAM roles and permissions.

  4. Consistent Step Patterns: Both direct name and output-based step definitions are provided, maintaining consistency with other AWS services.

  5. Good Feature File Coverage: Clear Gherkin scenarios that demonstrate real-world use cases.

  6. Virtual Cloud Support: Proper integration with the Virtual Cloud emulator via awshelpers.GetVirtualCloudEndpoint().


🐛 Issues to Fix

1. Incorrect Error Type Checking in AssertFunctionNotExists (pkg/assertions/aws/lambda.go:79-84)

Problem: The code declares a typed error variable but uses strings.Contains() instead of errors.As():

var notFoundErr *types.ResourceNotFoundException
if !strings.Contains(err.Error(), "ResourceNotFoundException") {
    return fmt.Errorf("unexpected error checking Lambda function %s: %w", functionName, err)
}
_ = notFoundErr // Silence unused variable warning

Fix: Use proper type assertion:

import "errors"

var notFoundErr *types.ResourceNotFoundException
if !errors.As(err, &notFoundErr) {
    return fmt.Errorf("unexpected error checking Lambda function %s: %w", functionName, err)
}

This is the idiomatic Go way to check AWS SDK error types and avoids brittle string matching.


2. Missing Null Safety in AssertFunctionRuntime (pkg/assertions/aws/lambda.go:96)

Problem: Direct string conversion without null check:

if string(config.Runtime) != runtime {

Issue: If config.Runtime is empty/nil, this could cause unexpected behavior. While AWS Lambda functions always have a runtime, defensive programming is safer.

Recommendation: Use aws.ToString() pattern or add explicit check for empty runtime with a clear error message.


3. Unused Import Warning Hack (pkg/steps/aws/lambda.go:321-322)

// Silence unused import warning for strconv
var _ = strconv.Itoa

Problem: The strconv import appears to be unused. This hack is a code smell.

Fix: Remove the strconv import entirely if it's not used. If you anticipate needing it in the future, it's better to add it when actually needed rather than keeping dead code.


🔒 Security Considerations

  1. IAM Permissions: The Terraform examples correctly use AWSLambdaBasicExecutionRole managed policy. ✅

  2. Function URL Authorization: Good validation of authorization types (NONE vs AWS_IAM) in variables.tf. ✅

  3. No Credential Exposure: Code properly uses AWS SDK credential chain. ✅


Performance Considerations

  1. Client Reuse: Each assertion method creates a new Lambda client. While this is consistent with other services in the codebase, consider if you need to make multiple assertions on the same function - the client creation overhead is minimal but could be optimized in the future.

  2. Helper Method Efficiency: The getFunctionConfiguration() helper is well-designed to avoid redundant API calls when checking multiple configuration attributes. ✅


🧪 Test Coverage

Missing: The PR description shows feature tests are pending:

  • Feature tests with ./bin/infraspec features/aws/lambda/ --virtual-cloud

Recommendation: Before merging, ensure these tests pass. Also consider adding unit tests for the assertion methods, following patterns from other services.


📝 Code Quality & Best Practices

  1. Error Messages: Clear and actionable error messages throughout. ✅

  2. Interface Compliance: Proper interface declaration and compile-time verification with var _ LambdaAsserter = (*AWSAsserter)(nil). ✅

  3. Documentation: Interface methods are well-documented. Consider adding godoc comments for individual assertion functions.

  4. Naming Conventions: Follows Go and project conventions consistently. ✅

  5. Context Usage: Uses context.TODO() which is acceptable for now, but consider if you want to support context propagation in the future for timeouts/cancellation.


🎯 Architecture & Design

  1. Separation of Concerns: Excellent separation between assertions, steps, and helpers. ✅

  2. Interface Design: The LambdaAsserter interface is well-designed with logical grouping of methods. ✅

  3. Virtual Cloud Integration: Properly integrated with the emulator endpoint system. ✅

  4. Step Registration: Correctly registered in pkg/steps/aws/aws.go. ✅


📋 Terraform Examples Review

basic-function/main.tf:

  • Good use of data.archive_file for inline Lambda code ✅
  • Configurable runtime, handler, timeout, memory ✅
  • Proper IAM role with AssumeRole policy ✅

function-with-alias/main.tf:

  • publish = true correctly set for versioning ✅
  • Alias properly references function version ✅

function-url/main.tf:

  • CORS configuration is comprehensive ✅
  • Conditional aws_lambda_permission for public access is smart ✅
  • Validation in variables.tf ensures only valid auth types ✅

🔧 Recommendations

  1. Priority High: Fix the error type checking in AssertFunctionNotExists.
  2. Priority Medium: Remove the unused strconv import and the silence hack.
  3. Priority Medium: Add null safety for runtime check.
  4. Priority Low: Run feature tests and confirm they pass before merging.
  5. Priority Low: Consider adding godoc comments for exported assertion functions.

📊 Overall Assessment

This is a well-implemented PR that follows the project's conventions and provides comprehensive Lambda testing capabilities. The code quality is high, and the design is clean. With the minor issues fixed (particularly the error handling), this will be a solid addition to the codebase.

Recommendation: Approve with requested changes for the error handling fix.


Great work on this implementation! The structure is solid, and it provides excellent coverage for Lambda testing scenarios. 🚀

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link
Contributor

claude bot commented Dec 29, 2025

Pull Request Review: Lambda BDD Testing Support

This is a solid implementation that adds comprehensive Lambda testing support to InfraSpec. The code follows established patterns and integrates well with the existing architecture. Here's my detailed feedback:


✅ Strengths

1. Excellent Code Organization

  • Clean separation between assertions (pkg/assertions/aws/lambda.go), step definitions (pkg/steps/aws/lambda.go), and helpers (pkg/awshelpers/lambda.go)
  • Follows the established patterns from other AWS services (S3, RDS, EC2, IAM)
  • Proper integration with the step registration system in pkg/steps/aws/aws.go

2. Comprehensive Test Coverage

  • 15+ assertion methods covering existence, configuration, versions, aliases, URLs, and layers
  • Well-structured Gherkin feature files with realistic scenarios
  • Multiple Terraform examples demonstrating different use cases

3. Virtual Cloud Integration

  • Lambda endpoint properly configured in service map (pkg/steps/terraform/terraform.go:225)
  • pkg/awshelpers/lambda.go correctly implements endpoint override for Virtual Cloud testing
  • Follows the endpoint configuration pattern described in CLAUDE.md

4. Consistent API Design

  • Both direct name and "from output" variants for all assertions
  • Helper methods reduce code duplication
  • Interface-based design allows for easy testing and mocking

🐛 Issues & Suggestions

Critical: Unused Variable in AssertFunctionNotExists

Location: pkg/assertions/aws/lambda.go:79-84

// Check if the error is a ResourceNotFoundException
var notFoundErr *types.ResourceNotFoundException
if !strings.Contains(err.Error(), "ResourceNotFoundException") {
    // If it's a different error, return it
    return fmt.Errorf("unexpected error checking Lambda function %s: %w", functionName, err)
}
_ = notFoundErr // Silence unused variable warning

Problem: The code declares notFoundErr but never uses it. Instead of proper type assertion, it falls back to string matching.

Fix: Use proper error type checking:

// Check if the error is a ResourceNotFoundException
var notFoundErr *types.ResourceNotFoundException
if !errors.As(err, &notFoundErr) {
    // If it's a different error, return it
    return fmt.Errorf("unexpected error checking Lambda function %s: %w", functionName, err)
}

Don't forget to add "errors" to the imports.


Code Quality: Unused Import

Location: pkg/steps/aws/lambda.go:322

// Silence unused import warning for strconv
var _ = strconv.Itoa

Problem: strconv is imported on line 6 but never used. This is a code smell.

Fix: Remove the strconv import entirely. The Godog regex captures already handle integer parsing for timeout and memory parameters.


Enhancement: Client Reuse Pattern

Current Pattern: Many methods create a new client for each call:

func (a *AWSAsserter) AssertFunctionExists(functionName string) error {
    client, err := awshelpers.NewLambdaClientWithDefaultRegion()
    // ...
}

func (a *AWSAsserter) AssertFunctionVersionExists(functionName, version string) error {
    client, err := awshelpers.NewLambdaClientWithDefaultRegion()
    // ...
}

Observation: Unlike S3 which has createS3Client() method on the asserter, Lambda creates clients directly. This is inconsistent.

Suggestion: Consider adding a helper method for consistency:

func (a *AWSAsserter) createLambdaClient() (*lambda.Client, error) {
    return awshelpers.NewLambdaClientWithDefaultRegion()
}

Then use client, err := a.createLambdaClient() throughout. This matches the S3 pattern and makes future refactoring easier (e.g., if you want to add client caching).


Documentation: Missing Comments on Interface Methods

The LambdaAsserter interface methods have minimal documentation. While the implementation has good comments, adding interface-level documentation helps users understand the contract.

Example:

type LambdaAsserter interface {
    // AssertFunctionExists verifies that a Lambda function exists in AWS
    // Returns an error if the function doesn't exist or cannot be accessed
    AssertFunctionExists(functionName string) error
    
    // AssertFunctionNotExists verifies that a Lambda function does NOT exist
    // Returns an error if the function exists or on unexpected errors
    AssertFunctionNotExists(functionName string) error
    // ... etc
}

🔒 Security Considerations

Good Security Practices:

  • No hardcoded credentials or sensitive data
  • Uses AWS SDK's built-in credential chain
  • Proper use of IAM role in Terraform examples
  • Function URLs support both NONE and AWS_IAM auth types

Terraform Example Security:

The function-url example has a good security pattern:

resource "aws_lambda_permission" "function_url" {
  count = var.authorization_type == "NONE" ? 1 : 0
  # Only grants public access when explicitly configured
}

This prevents accidental public exposure. Well done!


🚀 Performance Considerations

API Call Efficiency:

  • getFunctionConfiguration helper avoids duplicate API calls ✅
  • Each assertion makes minimal AWS API calls ✅
  • No pagination issues (Lambda operations are single-resource)

Potential Optimization:

If multiple assertions run against the same function in a single test, they'll each call GetFunctionConfiguration. Consider caching configuration within a test context if this becomes a bottleneck. However, for current use cases, this is premature optimization.


🧪 Test Coverage

What's Tested:

  • Basic function creation ✅
  • Runtime, handler, timeout, memory configuration ✅
  • Environment variables ✅
  • Aliases and versions ✅
  • Function URLs with different auth types ✅

What's Missing:

  • No tests for AssertFunctionHasLayer (declared but not used in features)
  • No tests for AssertEventSourceMappingExists (declared but not used in features)
  • No negative test cases (e.g., verifying a function should NOT exist)

Recommendation: Either add feature tests for layers and event source mappings, or remove those methods in this PR and add them when needed. Following YAGNI principle from CLAUDE.md.


📝 Conventional Commits

The commit message follows the Conventional Commits spec:

feat(lambda): add BDD testing support for Lambda functions

Perfect use of scope and type! ✅


🎯 Recommendations

High Priority:

  1. Fix the error type assertion in AssertFunctionNotExists (see Critical section above)
  2. Remove unused strconv import

Medium Priority:

  1. Add createLambdaClient() helper method for consistency with other services
  2. Add interface-level documentation comments
  3. Either add tests for layers/event source mappings or remove unused methods

Low Priority:

  1. Add negative test scenarios in feature files
  2. Consider adding a test for the AssertFunctionNotExists path

✨ Summary

This is a high-quality PR that follows InfraSpec's established patterns and conventions. The implementation is clean, well-organized, and production-ready. The critical issues are minor and easily fixed. The Terraform examples are particularly well-crafted and provide good real-world patterns.

Approval Status: ✅ Approved with minor changes requested

Great work on maintaining consistency with the existing codebase! The Lambda implementation will be a valuable addition to InfraSpec's testing capabilities.

Emulation is now the default behavior - no flag needed. Use --live
flag when testing against real AWS infrastructure.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link
Contributor

claude bot commented Dec 29, 2025

Pull Request Review - Lambda BDD Testing Support

Summary

This PR adds comprehensive Lambda testing support to InfraSpec with BDD-style Gherkin tests. The implementation is well-structured and follows established patterns in the codebase.


✅ Strengths

Code Quality

  • Consistent patterns: Follows existing conventions from other AWS services (DynamoDB, S3, RDS, EC2)
  • Good separation of concerns: Clear division between assertions (pkg/assertions/aws/lambda.go), step definitions (pkg/steps/aws/lambda.go), and helpers (pkg/awshelpers/lambda.go)
  • Interface-driven design: Proper use of LambdaAsserter interface with compile-time verification
  • Comprehensive coverage: 15+ assertion methods covering existence, configuration, versions, aliases, URLs, layers, and event sources

Terraform Examples

  • Well-designed examples: Three practical scenarios (basic function, alias/versioning, function URLs)
  • Proper AWS provider configuration: Includes version constraints (>= 5.72.1)
  • Realistic implementations: Inline Python code, IAM roles with proper assume role policies, CORS configuration for URLs
  • Good parameterization: Variables for common configurations (runtime, handler, timeout, memory, environment)

Gherkin Feature Files

  • Clear BDD scenarios: Well-written user stories with proper Given-When-Then structure
  • Good test coverage: Tests basic configuration, environment variables, aliases, and function URLs (NONE and AWS_IAM auth)

🐛 Issues Found

1. Improper Error Type Checking (Bug - High Priority)

Location: pkg/assertions/aws/lambda.go:78-84

Current code:

var notFoundErr *types.ResourceNotFoundException
if !strings.Contains(err.Error(), "ResourceNotFoundException") {
    return fmt.Errorf("unexpected error checking Lambda function %s: %w", functionName, err)
}
_ = notFoundErr // Silence unused variable warning

Problems:

  • Declares notFoundErr but never uses it properly
  • Uses string matching instead of proper error type assertion
  • The underscore assignment is a code smell

Recommended Fix:

var notFoundErr *types.ResourceNotFoundException
if !errors.As(err, &notFoundErr) {
    return fmt.Errorf("unexpected error checking Lambda function %s: %w", functionName, err)
}

Don't forget to add "errors" import.

2. Unused Import (Code Quality - Low Priority)

Location: pkg/steps/aws/lambda.go:321-322

// Silence unused import warning for strconv
var _ = strconv.Itoa

Issue: The strconv import appears unnecessary. Check if it can be removed entirely. If needed, it should be used naturally rather than with a dummy assignment.

3. Missing Unit Tests (Test Coverage - High Priority)

Issue: No unit tests found for:

  • pkg/assertions/aws/lambda.go - No lambda_test.go
  • pkg/steps/aws/lambda.go - No step definition tests
  • pkg/awshelpers/lambda.go - No helper tests

Recommendation: According to CLAUDE.md testing guidelines, add unit tests for at least:

  • AssertFunctionExists (success and not found cases)
  • AssertFunctionRuntime (matching and mismatched runtime)
  • AssertFunctionEnvironmentVariable (exists, missing variable, missing all variables)
  • AssertFunctionURLAuthType (matching auth types)

4. Lambda Service Endpoint Configuration (Integration - Medium Priority)

Question: Has the Lambda service endpoint been added to the serviceMap in pkg/steps/terraform/terraform.go?

According to CLAUDE.md Virtual Cloud Integration section, ensure there's an entry like:

"LAMBDA": "lambda",

This is critical for routing Lambda API calls to the embedded emulator.


🎯 Recommendations

Performance

  • ✅ Proper use of helper method getFunctionConfiguration to avoid repeated client creation
  • ⚠️ Consider caching Lambda client instances if multiple assertions run in sequence (optimization for future)

Security

  • ✅ No credential exposure
  • ✅ Proper use of AWS SDK v2 client configuration
  • ✅ IAM roles follow least-privilege principle in examples

Code Organization

  • ✅ Follows Go project layout standards
  • ✅ Clear package boundaries (public assertions in pkg/, no internal leakage)
  • ✅ Consistent naming: all assertion functions start with Assert

Documentation

  • ⚠️ Consider adding godoc comments for exported functions in lambda.go
  • ⚠️ Add inline comments for complex logic if any

📋 Action Items Before Merge

Critical

  1. Fix error type checking in AssertFunctionNotExists (use errors.As)
  2. Add unit tests for assertion functions
  3. Verify Lambda endpoint mapping in serviceMap

Nice to Have

  1. Remove unused strconv import or use it properly
  2. Add godoc comments for public functions
  3. Run make go-test-cover and ensure coverage meets project standards

🎉 Overall Assessment

Strong implementation that demonstrates good understanding of the InfraSpec architecture. The code quality is high, follows established patterns, and the Terraform examples are practical and well-designed.

The main concerns are:

  1. The error handling bug (easy fix)
  2. Missing unit tests (required before merge per project standards)
  3. Endpoint configuration verification

Recommendation: Request changes for the critical items, then approve once addressed.


📚 References

Add pre-defined AWS managed policy support to fix Lambda BDD tests that
use AWSLambdaBasicExecutionRole and other managed policies.

- Add aws_managed_policies.go with 25 common AWS managed policies
- Include realistic policy documents for Lambda, S3, DynamoDB, EC2, RDS,
  IAM, SQS, CloudWatch, and SSM services
- Add fallback stub policy for unrecognized managed policy ARNs
- Update attachRolePolicy/detachRolePolicy to handle AWS managed policies
- Update getPolicy/getPolicyVersion to return managed policy data

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link
Contributor

claude bot commented Dec 29, 2025

Pull Request Review: Lambda BDD Testing Support

Summary

This PR adds comprehensive BDD testing infrastructure for AWS Lambda functions, including 15+ assertion methods, 25+ Gherkin step definitions, and three well-designed Terraform examples. The implementation follows the project's established patterns and integrates cleanly with the Virtual Cloud emulator.

Strengths

1. Excellent Code Organization

  • Clear separation of concerns across assertions, step definitions, and helpers
  • Follows existing patterns from other AWS services (S3, DynamoDB, RDS)
  • Interface-based design with LambdaAsserter enables testability

2. Comprehensive Feature Coverage

Assertions cover all major Lambda functionality: existence checks, configuration (runtime, handler, timeout, memory), environment variables, versions/aliases, function URLs, layers, and event source mappings.

3. Well-Designed Terraform Examples

Three examples demonstrate: basic functions with configurable parameters, versioning/aliasing, and function URLs with both public and IAM auth. All include proper IAM roles and validation.

4. Virtual Cloud Integration

Proper endpoint configuration following established patterns, CI workflow updated.

Issues and Concerns

CRITICAL: Missing Unit Tests (Severity: High)

Per CLAUDE.md: Every new AWS operation MUST have unit tests.

Missing test files:

  • pkg/assertions/aws/lambda_test.go (312 lines, 0% coverage)
  • pkg/steps/aws/lambda_test.go (323 lines, 0% coverage)
  • pkg/awshelpers/lambda_test.go (26 lines, 0% coverage)

This is a blocking issue per project guidelines.

Code Quality Issues

1. Unused Variable Warning Suppression (pkg/assertions/aws/lambda.go:84)

Current code declares notFoundErr but never uses it, then suppresses the warning. Use proper type assertion with errors.As instead of strings.Contains to validate error type.

2. Unnecessary Import Suppression (pkg/steps/aws/lambda.go:322)

strconv is imported but never used. Remove the import instead of suppressing the warning.

3. Context.TODO() Usage

Multiple assertions use context.TODO() instead of accepting and propagating context from callers. This prevents proper timeout/cancellation propagation and distributed tracing.

Recommendation: Refactor assertion methods to accept context.Context parameter.

Additional Observations

Positive:

  • Clear, actionable error messages
  • Helper function reuse (getFunctionConfiguration)
  • Proper AWS SDK usage
  • High-quality Gherkin scenarios

Suggestions:

  • Add negative test scenarios to feature files
  • Add godoc examples for key assertions
  • aws_managed_policies.go (632 lines) is excellent - verify it works with Lambda roles

Security Review

No concerns identified. Proper IAM policies, no credential logging, function URLs correctly configured.

Performance

Good practices: single API calls, helper methods prevent redundancy, Virtual Cloud enables fast testing.

Test Plan Status

  • go build ./... passed
  • make go-test-cover passed
  • Feature tests ./bin/infraspec features/aws/lambda/ NOT COMPLETED

Recommendations

Before Merge (MUST):

  1. Add unit tests for all three new files
  2. Fix notFoundErr type assertion (use errors.As)
  3. Remove unused strconv import
  4. Run and verify feature tests

Before Merge (SHOULD):

  1. Refactor assertions to accept context.Context
  2. Add test coverage to meet project standards

Nice to Have:

  1. Add negative test scenarios
  2. Add godoc examples
  3. Test AWS managed policy attachments with Lambda

Overall Assessment

Score: 7.5/10

High-quality work that adds significant value. Main blocker is missing unit tests (mandatory per guidelines). Architecture is sound, follows conventions, comprehensive coverage.

Effort to fix: approximately 4-6 hours for unit tests + 30 min for code quality fixes.

Great job on the feature design and implementation!

- Use errors.As() instead of strings.Contains() for proper AWS SDK
  error type checking in AssertFunctionNotExists
- Add null safety check for empty runtime in AssertFunctionRuntime
- Remove unused strconv import and associated hack

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link
Contributor

claude bot commented Dec 29, 2025

Pull Request Review - Lambda BDD Testing Support

I've completed a thorough review of this PR. Overall, this is excellent work that adds comprehensive Lambda testing support following the project's established patterns. Here's my detailed feedback:


Strengths

1. Excellent Code Organization & Consistency

  • Follows the established InfraSpec patterns perfectly (assertions → steps → examples → features)
  • File naming and structure consistent with other AWS services (S3, DynamoDB, RDS)
  • Clean separation between interface definition (LambdaAsserter) and implementation
  • Proper use of helper functions to avoid code duplication

2. Comprehensive Coverage

  • 15+ assertion methods covering:
    • Basic existence checks
    • Configuration (runtime, handler, timeout, memory, env vars)
    • Versions & aliases
    • Function URLs with auth types
    • Layers and event source mappings
  • 25+ step definitions with both direct and output-based variants
  • 3 complete Terraform examples with realistic configurations
  • 3 feature files with practical BDD scenarios

3. Strong Error Handling

// Example from lambda.go:79-83
var notFoundErr *types.ResourceNotFoundException
if \!errors.As(err, &notFoundErr) {
    return fmt.Errorf("unexpected error checking Lambda function %s: %w", functionName, err)
}
  • Proper AWS SDK error type checking using errors.As()
  • Clear, actionable error messages
  • Distinguishes between "not found" and "unexpected errors"

4. Virtual Cloud Integration

  • Properly integrated with embedded emulator via awshelpers.NewLambdaClientWithDefaultRegion()
  • Follows endpoint configuration pattern in pkg/awshelpers/lambda.go
  • AWS managed policies correctly added to internal/emulator/services/iam/aws_managed_policies.go

5. Terraform Examples Quality

  • Realistic configurations with proper IAM roles and policies
  • Good use of variables for configurability
  • Proper output definitions for testing
  • Archive file data source pattern is clean and practical

🔍 Issues & Concerns

1. ⚠️ CRITICAL: Missing Emulator Implementation

The PR adds Lambda testing infrastructure but the emulator doesn't implement Lambda service handlers yet. This means:

Current state:

  • ✅ Client helper configured: pkg/awshelpers/lambda.go
  • ✅ Terraform will send requests to emulator
  • Emulator has NO Lambda service implementation
  • ❌ All Lambda API calls will return 404 or fail

What's missing:

internal/emulator/services/lambda/    ← This directory doesn't exist\!
├── service.go
├── types.go
├── create_function_handler.go
├── get_function_handler.go
├── update_function_configuration_handler.go
├── create_alias_handler.go
├── create_function_url_config_handler.go
└── ... (other handlers)

Required actions before merge:

  1. Implement Lambda service in the emulator
  2. Add handlers for operations used in tests:
    • CreateFunction, GetFunction, GetFunctionConfiguration
    • PublishVersion, CreateAlias, GetAlias
    • CreateFunctionUrlConfig, GetFunctionUrlConfig
    • GetEventSourceMapping

Recommendation: Either:

  • A) Complete the emulator implementation before merging this PR
  • B) Mark feature tests as @skip or @pending until emulator is ready
  • C) Split into 2 PRs: (1) testing infrastructure (this PR), (2) emulator implementation

2. Missing Unit Tests

No unit tests for the new assertion functions:

Expected test file: pkg/assertions/aws/lambda_test.go

func TestAssertFunctionExists_Success(t *testing.T) { ... }
func TestAssertFunctionExists_NotFound(t *testing.T) { ... }
func TestAssertFunctionRuntime_Mismatch(t *testing.T) { ... }
// ... etc

Per CLAUDE.md:

Unit Testing Requirements

Every new AWS operation MUST have unit tests.

Recommendation: Add unit tests using the pattern from pkg/assertions/aws/s3_test.go or similar.


3. ⚠️ Incomplete Test Plan

From the PR description:

## Test plan
- [x] `go build ./...` passes
- [x] `make go-test-cover` passes
- [ ] Feature tests with `./bin/infraspec features/aws/lambda/ --virtual-cloud`  ← UNCHECKED

The most important test (feature tests) is not completed. This is expected since the emulator implementation is missing.

Recommendation: Complete feature tests before marking PR as ready to merge.


4. Documentation Gap: serviceMap Update

The PR updates CLAUDE.md virtual cloud section but doesn't document adding Lambda to the service endpoint map.

Expected: Update pkg/steps/terraform/terraform.go to add:

serviceMap := map[string]string{
    // ... existing entries ...
    "LAMBDA": "lambda",  // ← Should be added
}

Can you confirm this is already in the code? I don't see it in the diff, but it might be pre-existing.


5. Minor: Context Usage

// lambda.go:54
_, err = client.GetFunction(context.TODO(), &lambda.GetFunctionInput{

Using context.TODO() instead of receiving context from caller. While not critical, the Godog step functions receive a context.Context that could be propagated through.

Recommendation: Consider propagating context from step functions through to AWS SDK calls for better cancellation/timeout handling.


6. Edge Case: Environment Variables

// lambda.go:156-158
if config.Environment == nil || config.Environment.Variables == nil {
    return fmt.Errorf("Lambda function %s has no environment variables", functionName)
}

This will fail if asserting a variable on a function with no environment block. However, AWS Lambda can have an empty environment, which is different from not having the variable.

Current behavior:

  • Function with no env vars → Error: "has no environment variables"
  • Function with env vars but missing key → Error: "environment variable X not found"

This is actually good UX - clear distinction between the two cases. ✅


🎯 Recommendations

Before Merging:

  1. CRITICAL: Implement Lambda emulator service or mark tests as pending
  2. Add unit tests for assertion functions
  3. Verify serviceMap in pkg/steps/terraform/terraform.go includes Lambda
  4. Complete feature test execution and update test plan

Nice to Have:

  1. Propagate context from step functions to SDK calls
  2. Add test scenario for Lambda with VPC configuration
  3. Add test scenario for Lambda with dead-letter queue

🔒 Security Review

✅ No security concerns identified:

  • No credential leakage
  • Proper use of AWS SDK v2
  • No command injection vectors
  • IAM policy documents use proper jsonencode()
  • Inline code is minimal and safe (Python hello world)

📊 Performance Considerations

✅ Efficient implementation:

  • Proper reuse of getFunctionConfiguration() helper to avoid duplicate API calls
  • Client creation pattern consistent with other services
  • No obvious performance bottlenecks

🎨 Code Quality

Excellent adherence to project standards:

  • ✅ Follows conventional commits (feat(lambda): ...)
  • ✅ Proper Go formatting (will be verified by make fmt)
  • ✅ Interface-based design for testability
  • ✅ Clear function names following Assert* pattern
  • ✅ Comprehensive documentation in interface comments

📝 Summary

This is high-quality work that demonstrates strong understanding of the InfraSpec architecture. The testing infrastructure is comprehensive and well-designed.

Blocking issues:

  1. Missing Lambda emulator implementation
  2. Incomplete feature test execution

Action required: Complete the emulator implementation or split this PR into two phases.

Once the emulator is implemented and tests pass, this will be an excellent addition to InfraSpec. Great job! 🚀


Review Status: ⚠️ CHANGES REQUESTED

cc: @robmorgan

@robmorgan robmorgan merged commit 6b24f50 into main Dec 29, 2025
15 checks passed
@robmorgan robmorgan deleted the feat/lambda-bdd-testing branch December 29, 2025 10:56
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