diff --git a/CHANGELOG.md b/CHANGELOG.md index 6df17ef..0fb9981 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,6 @@ All notable changes to this project will be documented in this file. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project uses [semantic versioning](https://semver.org/). --- - ## [Unreleased] ### Changed @@ -16,9 +15,23 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and - Added comprehensive boundary testing for time-based filtering rules - Self-documenting test cases with clear business rule descriptions - **Enhanced Test Reporting**: Added aligned, formatted test output for better readability +- **Improved Test Validation**: Enhanced duplicate detection logic with precise timestamp validation + - Added `verify_duplicate_testcase` function with comprehensive error messaging + - Implemented detailed test case validation showing PASSED vs FILTERED status + - Refactored test logic into organized `duplicate_test` module for better maintainability + +### Added +- **Comprehensive Edge Case Testing**: Dynamic test data generation for boundary conditions + - Automated testing of 7-day and 90-day filtering boundaries + - Real-time duplicate detection validation with timing precision +- **Enhanced Test Output**: Clear, professional test result formatting + - Color-coded test status (✅/❌) with descriptive failure messages + - Detailed debug output for duplicate validation logic ### Technical - Improved test maintainability with descriptive entity names and test descriptions +- Modularized duplicate testing logic for better code organization +- Enhanced error messages for easier debugging of test failures --- diff --git a/Dockerfile.lambda-runtime b/Dockerfile.lambda-runtime index cc3e6b4..db4724a 100644 --- a/Dockerfile.lambda-runtime +++ b/Dockerfile.lambda-runtime @@ -18,6 +18,9 @@ RUN rustup component add rustfmt # Install cargo-lambda RUN cargo install cargo-lambda --version 1.8.5 --locked --quiet +# Install clippy component +RUN rustup component add clippy + # Default user RUN useradd -m dev WORKDIR /app diff --git a/scripts/build.sh b/scripts/build.sh index cb35cff..08879a9 100755 --- a/scripts/build.sh +++ b/scripts/build.sh @@ -22,11 +22,13 @@ if ! nc -z localhost 9000; then exit 1 fi +RUN="docker exec aws-lambda-action-filter-lambda-1" # Run all tests inside the container -docker exec aws-lambda-action-filter-lambda-1 cargo fmt --version -docker exec aws-lambda-action-filter-lambda-1 cargo fmt --check -docker exec aws-lambda-action-filter-lambda-1 cargo build --release --quiet -docker exec aws-lambda-action-filter-lambda-1 cargo test --release --quiet +${RUN} cargo fmt --version +${RUN} cargo fmt --check +${RUN} cargo clippy --quiet --all-features -- -D warnings +${RUN} cargo build --release --quiet +${RUN} cargo test --release --quiet -- --nocapture echo "All tests passed!" diff --git a/tests/edge_case_tests.rs b/tests/edge_case_tests.rs index ca45167..d0ddf7b 100644 --- a/tests/edge_case_tests.rs +++ b/tests/edge_case_tests.rs @@ -37,16 +37,18 @@ struct TestCase { description: &'static str, } +const DUP_TEST_ID: &str = "dedup_test_id"; + #[rustfmt::skip] const EDGE_CASES: &[TestCase] = &[ - TestCase { entity_id: "dedup_first_occurrence", next_offset: 30, last_offset: -10, priority: Priority::Urgent, should_pass: true, description: "Tests deduplication (first occurrence)" }, - TestCase { entity_id: "dedup_first_occurrence", next_offset: 35, last_offset: -15, priority: Priority::Normal, should_pass: true, description: "Tests deduplication (last occurrence wins)" }, - TestCase { entity_id: "more_than_7_days_ago_fail", next_offset: 20, last_offset: -7, priority: Priority::Urgent, should_pass: false, description: "Tests 'more than 7 days ago' rule (should fail)" }, - TestCase { entity_id: "more_than_7_days_ago_pass", next_offset: 20, last_offset: -8, priority: Priority::Urgent, should_pass: true, description: "Tests 'more than 7 days ago' rule (should pass)" }, - TestCase { entity_id: "more_than_7_days_ago_pass_2", next_offset: 25, last_offset: -10, priority: Priority::Urgent, should_pass: true, description: "Tests 'more than 7 days ago' rule (should pass)" }, - TestCase { entity_id: "within_90_days_fail", next_offset: 91, last_offset: -30, priority: Priority::Normal, should_pass: false, description: "Tests 'within 90 days' rule (should fail at 91 days)" }, - TestCase { entity_id: "within_90_days_pass", next_offset: 90, last_offset: -30, priority: Priority::Normal, should_pass: true, description: "Tests 'within 90 days' rule boundary (should pass)" }, - TestCase { entity_id: "within_90_days_pass_2", next_offset: 89, last_offset: -20, priority: Priority::Normal, should_pass: true, description: "Tests 'within 90 days' rule (should pass)" }, + TestCase { entity_id: DUP_TEST_ID, next_offset: 30, last_offset: -10, priority: Priority::Urgent, should_pass: false, description: "Rule: deduplication (first occurrence, expected to be FILTERED)" }, + TestCase { entity_id: DUP_TEST_ID, next_offset: 35, last_offset: -15, priority: Priority::Normal, should_pass: true, description: "Rule: deduplication (last occurrence wins, expected to be PASSED)" }, + TestCase { entity_id: "more_than_7_days_ago_fail", next_offset: 20, last_offset: -7, priority: Priority::Urgent, should_pass: false, description: "Rule: more than 7 days ago (fail <7)" }, + TestCase { entity_id: "more_than_7_days_ago_pass", next_offset: 20, last_offset: -8, priority: Priority::Urgent, should_pass: true, description: "Rule: more than 7 days ago (pass =7)" }, + TestCase { entity_id: "more_than_7_days_ago_pass_2", next_offset: 25, last_offset: -10, priority: Priority::Urgent, should_pass: true, description: "Rule: more than 7 days ago (pass >7)" }, + TestCase { entity_id: "within_90_days_fail", next_offset: 91, last_offset: -30, priority: Priority::Normal, should_pass: false, description: "Rule: within 90 days (fail >90)" }, + TestCase { entity_id: "within_90_days_pass", next_offset: 90, last_offset: -30, priority: Priority::Normal, should_pass: true, description: "Rule: within 90 days (pass =90)" }, + TestCase { entity_id: "within_90_days_pass_2", next_offset: 89, last_offset: -20, priority: Priority::Normal, should_pass: true, description: "Rule: within 90 days (pass <90)" }, ]; fn create_action( @@ -84,51 +86,68 @@ fn generate_test_data() -> Result { Ok(json) } -fn verify_test_expectations(results: &[Action]) -> Result<()> { +fn verify_test_expectations(results: &[Action]) -> (bool, Vec) { // --- - let prefix = "verify_test_expectations"; - // Convert results to a map for O(1) lookup let result_map: HashMap<&str, &Action> = results.iter().map(|action| (action.entity_id.as_str(), action)).collect(); + let mut lines = Vec::new(); + let mut passed = true; // To be set only if setting to false + // Iterate over test expectations and verify against results for test_case in EDGE_CASES { // --- - let found_in_results = result_map.contains_key(test_case.entity_id); + let found = result_map.contains_key(test_case.entity_id); + + if test_case.entity_id == DUP_TEST_ID { + // --- + let (pass, line) = + duplicate_test::verify_duplicate_testcase(test_case, &result_map, found); + + lines.push(line); + + if !pass { + passed = false; + } + continue; // Skip to next test case + } - match (test_case.should_pass, found_in_results) { + match (test_case.should_pass, found) { // --- (true, false) => { // --- - ensure!( - false, - "{prefix}: {} - Expected to pass but was filtered out. {}", - test_case.entity_id, - test_case.description - ); + lines.push(format!( + "❌ {:<28}: FILTERED - Expected to PASS but was not found in results. {}-2", + test_case.entity_id, test_case.description + )); + passed = false; } (false, true) => { // --- - ensure!( - false, - "{prefix}: {} - Expected to be filtered out but found in results. {}", - test_case.entity_id, - test_case.description - ); + lines.push(format!( + "❌ {:<28}: FILTERED - Expected to be FILTERED but was found in results. {}-3", + test_case.entity_id, test_case.description + )); + passed = false; } (true, true) => { // --- - println!("✓ {:<28}: PASS - {}", test_case.entity_id, test_case.description); + lines.push(format!( + "✅ {:<28}: PASSED - {}", + test_case.entity_id, test_case.description + )); } (false, false) => { // --- - println!("✓ {:<28}: FILTERED - {}", test_case.entity_id, test_case.description); + lines.push(format!( + "✅ {:<28}: FILTERED - {}", + test_case.entity_id, test_case.description + )); } } } - - Ok(()) + (passed, lines) } #[test] @@ -152,13 +171,22 @@ fn test_dynamic_edge_cases() -> Result<()> { let results = run_lambda_invoke(temp_file)?; println!("Lambda returned {} actions", results.len()); + results.iter().for_each(|action| { + println!(" :: {}", action.entity_id); + }); + print!("---\n\n"); // Verify all test expectations - verify_test_expectations(&results)?; + let (passed, lines) = verify_test_expectations(&results); + let lines = lines.join("\n"); + + println!("\nAll TestCase Results:"); + println!("{lines}"); + ensure!(passed, "Test failed"); // Additional verification: check expected count // Should have 5 actions: 6 that should pass - 1 duplicate = 5 - // (dedup_first_occurrence appears twice but deduplicated to 1) + // (DUP_TEST_ID appears twice but deduplicated to 1) let expected_count = 5; ensure!( results.len() == expected_count, @@ -181,8 +209,7 @@ fn test_dynamic_edge_cases() -> Result<()> { } // Verify deduplication worked correctly - let duplicate_count = - results.iter().filter(|a| a.entity_id == "dedup_first_occurrence").count(); + let duplicate_count = results.iter().filter(|a| a.entity_id == DUP_TEST_ID).count(); ensure!( duplicate_count == 1, "Expected exactly 1 'duplicate' entity after deduplication, found {}", @@ -210,3 +237,121 @@ fn test_dynamic_edge_cases() -> Result<()> { Ok(()) } + +mod duplicate_test { + // --- + use super::{Action, TestCase}; + use chrono::{Duration, Utc}; + use std::collections::HashMap; + + pub fn verify_duplicate_testcase( + test_case: &TestCase, + result_map: &HashMap<&str, &Action>, + is_found: bool, + ) -> (bool, String) { + // --- + // Early return: Handle case where entity was not found in results + if !is_found { + // --- + return handle_entity_not_found(test_case); + } + + // Entity was found - check if it's the correct duplicate + let duplicate_check_result = check_duplicate_correctness(test_case, result_map); + + match (test_case.should_pass, duplicate_check_result.is_correct) { + // --- + (true, true) => { + // Expected to pass, and correct duplicate was kept + (true, format_success_message(test_case, "PASSED")) + } + (true, false) => { + // Expected to pass, but wrong duplicate was kept + (false, format_wrong_duplicate_error(test_case, &duplicate_check_result)) + } + (false, true) => { + // Expected to be filtered, but correct duplicate was kept (this shouldn't happen) + (false, format_unexpected_correct_duplicate_error(test_case)) + } + (false, false) => { + // Expected to be filtered, and wrong duplicate was kept (which is fine) + (true, format_success_message(test_case, "FILTERED")) + } + } + } + + // Helper function to handle when entity is not found in results + fn handle_entity_not_found(test_case: &TestCase) -> (bool, String) { + // --- + if test_case.should_pass { + // Expected to pass but was filtered out + ( + false, + format!( + "❌ {:<28}: FAILED - {} Expected to PASS but was FILTERED.", + test_case.entity_id, test_case.description + ), + ) + } else { + // Expected to be filtered and was filtered + ( + true, + format!( + "✅ {:<28}: PASSED - {} (was not passed, which is what we expected)", + test_case.entity_id, test_case.description + ), + ) + } + } + + // Struct to hold duplicate correctness check results + struct DuplicateCheckResult { + is_correct: bool, + time_diff: i64, + } + + // Helper function to check if the correct duplicate was kept + fn check_duplicate_correctness( + test_case: &TestCase, + result_map: &HashMap<&str, &Action>, + ) -> DuplicateCheckResult { + // --- + let action = result_map.get(test_case.entity_id).unwrap(); + let now = Utc::now(); + let expected_next_time = now + Duration::days(test_case.next_offset); + + let time_diff = (action.next_action_time - expected_next_time).num_seconds().abs(); + let is_correct = time_diff < 60; // Allow small time differences + + DuplicateCheckResult { is_correct, time_diff } + } + + // Helper functions for formatting messages + fn format_success_message(test_case: &TestCase, status: &str) -> String { + // --- + format!("✅ {:<28}: {:<8} - {}", test_case.entity_id, status, test_case.description) + } + + fn format_wrong_duplicate_error( + test_case: &TestCase, + check_result: &DuplicateCheckResult, + ) -> String { + // --- + format!( + "❌ {:<28}: FILTERED - {}. Wrong DUPLICATE kept, \ + Expected next_offset: {}, but found action with different timing. {}", + test_case.entity_id, + test_case.description, + test_case.next_offset, + check_result.time_diff + ) + } + + fn format_unexpected_correct_duplicate_error(test_case: &TestCase) -> String { + // --- + format!( + "❌ {:<28}: FAILED - {} Expected to be FILTERED but correct duplicate was found in results.", + test_case.entity_id, test_case.description + ) + } +}