Add test coverage for --recursive option

[kompre]

Summary

Adds comprehensive test coverage for the --recursive option added in #4.

Changes

• ✅ Add 4 new tests for the --recursive option
  • Test recursive directory traversal
  • Test non-recursive mode behavior
  • Test recursive with match patterns
  • Test directory structure preservation

Notes

The CI workflow file needs to be added manually as described in the issue comment.

Closes #5

Generated with Claude Code

[claude]

Pull Request Review: Add test coverage for --recursive option

Summary

This PR adds comprehensive test coverage for the --recursive option and introduces a CI workflow. Overall, this is a valuable addition that improves test coverage. Below are my findings across several categories.

---

✅ Strengths

1. Comprehensive Test Coverage: The four new tests thoroughly cover the recursive functionality:

   • Basic recursive traversal
   • Non-recursive mode verification
   • Recursive with match patterns
   • Directory structure preservation

2. Good Test Isolation: Each test uses its own output directory and includes proper cleanup in try/finally blocks

3. CI Workflow: The addition of .github/workflows/test.yml is excellent for maintaining code quality

---

🔍 Code Quality & Best Practices

Tests: tests/test_quarto_batch_convert.py

Issues Found:

1. Inconsistent Whitespace (Line 184) - Minor formatting issue

   • Line 184 has trailing whitespace that was removed
   • Line 187 also had trailing whitespace removed
   • This is good cleanup but worth noting

2. Test Isolation Concerns - The new tests do not use the existing setup_teardown_test_env fixture

   • Location: Lines 192-299
   • Issue: Tests directly reference tests/assets without creating isolated test data
   • Risk: Tests could interfere with each other or rely on external state
   • Recommendation: Consider using the existing fixture pattern or creating a dedicated fixture for recursive tests

3. Hardcoded Paths - All new tests use hardcoded string literals

   • Example: "tests/assets", "temp_recursive_test_output"
   • Risk: Tests may fail if run from different working directories
   • Recommendation: Use os.path.join() or pathlib.Path for better cross-platform compatibility

4. Fragile Output Assertions - Lines 216, 243, 270

   • Issue: The fallback "files" is too generic and could match error messages
   • Recommendation: Use more specific assertions or check the actual file count differently

5. Missing Edge Cases:

   • No test for deeply nested directories (3+ levels)
   • No test for empty subdirectories
   • No test for recursive mode with symbolic links (if supported)
   • No test for recursive mode combined with other options like --prefix

CI Workflow: .github/workflows/test.yml

Issues Found:

1. Missing Newline at EOF - Line 36

   • The file lacks a trailing newline, which is a common convention
   • Minor but worth fixing for consistency

2. Limited Test Coverage in CI:

   • Only runs pytest tests -v
   • Consider adding:
     • Code coverage reporting
     • Linting (ruff, black, mypy)
     • Multiple Python versions if supported

3. Security Consideration:

   • ✅ Good: Uses permissions: contents: read (principle of least privilege)
   • ✅ Good: Uses pinned action versions (@v4, @v5, @v6, @v2)

---

🐛 Potential Bugs

1. Race Conditions in Cleanup (Low severity)

   • Location: Lines 199, 228, 255, 282
   • Code: shutil.rmtree(output_dir, ignore_errors=True)
   • Issue: Using ignore_errors=True could mask real filesystem issues
   • Impact: Minor - primarily affects test debugging
   • Recommendation: Consider using a proper temp directory context manager like tempfile.TemporaryDirectory()

2. Assumption About File Count (Medium severity)

   • Location: Lines 215-216, 243, 270
   • Issue: Tests assume specific file counts in tests/assets without verifying preconditions
   • Risk: If someone adds/removes files in tests/assets, tests will break
   • Recommendation: Either document the required structure or create test fixtures that do not depend on shared assets

---

⚡ Performance Considerations

1. Test Execution Time:

   • Each test creates temporary directories and calls Quarto CLI
   • Running 4 additional tests will increase test suite time
   • Recommendation: This is acceptable for the value provided, but consider running tests in parallel if suite grows

2. Redundant Cleanup:

   • Cleanup happens in both try and finally blocks (lines 199 + 219)
   • The initial cleanup is defensive but could be removed if using temp directories

---

🔒 Security Concerns

No significant security issues found. ✅

• Tests do not execute untrusted input
• CI workflow follows security best practices
• No credentials or secrets involved

---

🧪 Test Coverage

Current Coverage:

✅ Well Covered:

• Recursive directory traversal
• Non-recursive mode
• Interaction with match patterns
• Directory structure preservation

⚠️ Missing Coverage:

• Recursive mode combined with:
  • --prefix option
  • --keep-extension option
  • --qmd-to-ipynb conversion
• Edge cases:
  • Empty directories
  • Very deep nesting (3+ levels)
  • Mixed file types in same directory
  • Unicode characters in directory names
• Error cases:
  • Insufficient permissions
  • Invalid directory paths with -r flag

---

📝 Specific Recommendations

High Priority:

1. Fix test isolation: Use fixtures or tempfile.TemporaryDirectory() instead of hardcoded paths
2. Improve output assertions: Replace generic fallback patterns with specific checks
3. Add trailing newline to .github/workflows/test.yml

Medium Priority:

4. Expand CI workflow: Add linting and coverage reporting
5. Add edge case tests: Deep nesting, combined options, error handling
6. Document test asset requirements: Add README explaining tests/assets structure

Low Priority:

7. Use pathlib: Consider migrating to pathlib.Path for better cross-platform support
8. Consolidate cleanup logic: Use context managers instead of manual try/finally

---

✅ Approval Recommendation

This PR is approved with suggestions for follow-up improvements.

The core functionality is sound and the tests successfully validate the --recursive option. The identified issues are mostly minor and do not block merging. I recommend:

1. Merge as-is to close issue 5 and get test coverage in place
2. Create follow-up issues for the test isolation and edge case coverage improvements
3. Consider the CI workflow enhancements for future PRs

Great work on adding both tests and CI infrastructure!

---

Review generated with Claude Code