-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add test to detect inline comments in Jinja2 expressions within YAML files #14817
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
Conversation
...files This test would have caught the bug reported where inline comments (#) within Jinja2 expressions in YAML task files caused Ansible template errors. The test: - Extracts and validates all Jinja2 expressions from YAML files - Specifically detects inline comments within {{ }} and {% %} blocks - Includes regression test for the exact reported bug pattern - Avoids false positives (# in strings, escaped #, comments outside expressions) - Focuses on the critical inline comment issue The original bug was in roles/strongswan/tasks/openssl.yml where comments like "# Per-deployment UUID..." were placed inside a Jinja2 expression, causing "unexpected char '#'" errors during playbook execution. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Claude finished @dguido's task —— View job
PR Review: Add test to detect inline comments in Jinja2 expressions
I'll provide a comprehensive review of this pull request focusing on code quality, best practices, potential issues, performance, security, and test coverage.
Review Checklist:
- Read and analyze changed files
- Review test implementation quality and correctness
- Check CLAUDE.md updates for appropriateness
- Evaluate test coverage and effectiveness
- Check for potential bugs or issues
- Assess performance considerations
- Review security implications
- Provide constructive feedback
Overall Assessment: Excellent ✅
This is a high-quality PR that demonstrates best practices in defensive programming and comprehensive testing. The implementation is thorough, well-documented, and effectively addresses the problem.
Code Quality: Excellent
Strengths:
- Clean, readable code with excellent docstrings and comments
- Comprehensive edge case handling with 13+ test scenarios
- Follows mutation testing principles - verifies tests actually catch failures
- Robust error handling with graceful degradation for runtime issues
- Well-structured functions with single responsibilities
Minor suggestions:
roles/strongswan/tasks/openssl.yml:64-82
- The multiline YAML expressions are correctly formatted without inline comments, confirming the bug was fixed
Test Implementation: Outstanding
Excellent aspects:
- Regression test (
test_regression_openssl_inline_comments()
) uses the exact problematic pattern from the reported bug - Comprehensive edge cases covering strings, escaping, control blocks, multiline expressions
- Smart false positive prevention - correctly ignores
#
in strings, URLs, hex colors - File discovery logic properly excludes
.git
,venv
,configs
directories - Extensive mock context with 20+ realistic Ansible variables
Test coverage analysis:
# Covers the exact bug pattern: problematic = """{{ [ 'DNS:' + id, # comment here ← This caused the original bug ] }}""" # Tests all major expression types: - {{ variable }} expressions - {% control %} blocks - Multiline expressions - Hash in strings: "string with # hash" - Escaped hashes: \#
Security: No Concerns ✅
- Test code poses no security risks
- Uses safe regex patterns without ReDoS vulnerabilities
- Properly handles file reading with exception handling
- No credential exposure or unsafe operations
Performance: Well Optimized
Efficient design:
- Selective file scanning with glob patterns vs brute force directory traversal
- Early termination on YAML parsing errors to avoid unnecessary processing
- Compiled regex reuse through
re.finditer()
- Smart skipping of non-Jinja2 files
Potential optimizations (minor):
- Consider caching compiled regex patterns if running frequently
- File content is read once per file (already optimal)
Documentation: Excellent
The CLAUDE.md addition on mutation testing is particularly valuable:
# Perfect example of mutation testing approach: 1. Write test for the bug 2. Temporarily introduce bug to verify test fails 3. Fix bug and verify test passes 4. Document what the test prevents
This guidance will help future contributors write more effective tests.
Technical Deep Dive
Jinja2 Expression Extraction (extract_jinja2_expressions()
):
- Uses proper regex with
re.DOTALL
for multiline support - Captures both
{{ }}
and{% %}
patterns - Returns structured data with position info for error reporting
Validation Logic (validate_jinja2_expression()
):
- Layered validation: Comment detection → Template compilation → Rendering
- Smart comment detection: Removes strings before checking for
#
to avoid false positives - Comprehensive Ansible mocking: 15+ filters including
version
,ternary
,b64encode
File Processing (validate_yaml_file()
):
- Graceful YAML error handling - skips malformed files rather than failing
- Categorizes errors - separates inline comment issues from other syntax errors
- Provides context - includes line numbers and expression snippets
Regression Prevention
This test would have caught the original bug in roles/strongswan/tasks/openssl.yml
where inline comments caused:
FAILED! => {"msg": "Unexpected templating type error occurred on ({{ [ ... # Per-deployment UUID... ] }}): unexpected char '#' at 87"}
The current file (lines 64-82) now correctly uses multiline YAML without inline comments in the Jinja2 expressions.
Minor Suggestions
- Consider adding a CLI option to run just the inline comment checks for faster CI feedback
- File pattern optimization: The current patterns are comprehensive but could add
group_vars/**/*.yml
for completeness - Error message enhancement: Could include the specific line content that triggered the error
Recommendation: Approve and Merge ✅
This PR represents excellent defensive programming practices:
- ✅ Comprehensive test coverage
- ✅ Clear documentation of testing approach
- ✅ Effective regression prevention
- ✅ No security concerns
- ✅ Performance optimized
- ✅ Follows project conventions
The mutation testing guidance added to CLAUDE.md is particularly valuable for the project's long-term code quality.
- Converted standalone script to proper pytest test functions - Replaced main() with individual test functions using pytest assertions - Added comprehensive edge case tests for inline comment detection: * Hash symbols in strings (should pass) * Escaped hashes (should pass) * Comments in control blocks (should fail) * Multi-line expressions with comments (should fail) * URL fragments and hex colors (should pass) - Test functions now properly integrate with pytest: * test_regression_openssl_inline_comments() - regression test * test_edge_cases_inline_comments() - comprehensive edge cases * test_yaml_files_no_inline_comments() - scan all YAML files * test_openssl_file_specifically() - test the originally buggy file This addresses the review feedback about pytest integration and adds the suggested test cases for better coverage. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fixed trailing whitespace issues (W293) - Applied ruff formatting for consistent code style - All tests still pass after formatting changes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Added a section on writing effective tests that emphasizes the importance of verifying that tests actually detect failure cases. This lightweight mutation testing approach ensures: - Tests catch the specific bugs they're designed to prevent - We avoid false confidence from tests that always pass - Test purposes are clear and documented - Both success and failure cases are validated The guidance includes a concrete example from our recent inline comment detection test, showing how to verify both the problematic pattern (should fail) and the fixed pattern (should pass). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
#
) within Jinja2 expressions in YAML task files{{ }}
blocks caused Ansible template errorsBackground
A user reported a bug where inline comments within Jinja2 expressions in YAML files caused Ansible to fail with "unexpected char '#'" errors. The bug was in
roles/strongswan/tasks/openssl.yml
where comments like# Per-deployment UUID...
were placed inside a Jinja2 expression.While the bug has already been fixed, our existing tests didn't catch this issue because they only validated
.j2
template files, not Jinja2 expressions embedded within YAML task files.What This PR Does
The new test (
tests/unit/test_yaml_jinja2_expressions.py
):.j2
templates{{ }}
and{% %}
blocks#
in strings, escaped hashes, and comments outside expressionsTest Results
Test Output
This test ensures that similar bugs won't be introduced in the future.
🤖 Generated with Claude Code