fix: enable regression tests in AllTests.wl with proper generation and cleanup

[lwJi]

Summary

• Fix wolframscript -f test/AllTests.wl to properly run regression tests by generating output files before comparison
• Add cleanup logic to remove generated files after test completion
• Fix auto-run detection in compare_golden.wl to distinguish standalone execution from being loaded via Get[]

Test plan

• Run wolframscript -f test/AllTests.wl - all unit tests and 7/7 regression tests pass
• Verify exit code is 0 when tests pass
• Verify no .hxx or .c files remain in test directories after completion
• Verify standalone wolframscript -f test/regression/compare_golden.wl still works
• Verify Get["test/regression/compare_golden.wl"] in REPL loads without auto-running

[claude]

Code Review

Thanks for this PR! This is a solid improvement to the test infrastructure.

STRENGTHS:

1. Complete test workflow - properly implements regression test functionality with generation, comparison, and cleanup
2. Good code reuse - uses shared test_cases.txt consistently
3. Proper cleanup - removes generated files after tests
4. Smart auto-run detection - correctly distinguishes standalone execution from Get[] loading

CODE QUALITY:

• Good Module scoping and error checking
• Clear separation of concerns
• Proper exit code handling

POTENTIAL ISSUES:

1. Shell injection risk (AllTests.wl:72) - MEDIUM SEVERITY
   The Run command concatenates testName and backend without validation. If these contain special shell characters, could fail or pose security risks.
   Recommendation: Add validation or use RunProcess instead of Run

2. Silent file deletion failures (AllTests.wl:101-102)
   DeleteFile can fail but errors are not caught
   Recommendation: Wrap in Check or Quiet for graceful error handling

3. Error suppression (AllTests.wl:72)
   The 2>&1 redirect makes debugging harder

PERFORMANCE:

• Sequential test generation is appropriate
• Run command overhead acceptable for 7 tests

SECURITY:

• Medium risk: shell injection if malicious input in test_cases.txt
• Mitigated by version-controlled config file

TEST COVERAGE:

• Comprehensive testing per PR description
• All unit tests + 7/7 regression tests pass

VERDICT:
Well-implemented PR. Main concern is shell injection vulnerability.

Recommendation: APPROVE with changes requested

CHANGES REQUESTED:

1. Add input validation for testName and backend before shell execution
2. Add error handling for DeleteFile operations

OPTIONAL:

• Extract duplicate test case parsing to shared utility

[lwJi]

@claude could you implement the required 2 changes

[claude]

Claude finished @lwJi's task in 1m 34s —— View job

---

Implementation Complete ✓

I've successfully implemented the 2 required changes from the code review:

• Read current AllTests.wl to understand the code structure
• Add input validation for testName and backend before shell execution
• Add error handling for DeleteFile operations
• Commit and push changes to the branch

Changes Made

1. Input Validation (Shell Injection Prevention)

Added ValidShellInput function to validate backend and testName inputs before shell execution:

• Uses regex pattern ^[a-zA-Z0-9_./\-]+$ to allow only safe characters
• Validates both backend and testName before the Run command
• Sets $GenerationFailed = True and continues to next test if validation fails
• Location: test/AllTests.wl:61,74-78

2. Error Handling for DeleteFile

Wrapped DeleteFile calls with Check for graceful error handling:

• Catches and reports deletion failures with warning message
• Prevents silent failures during cleanup
• Location: test/AllTests.wl:112-115

Testing Recommendation

Please run wolframscript -f test/AllTests.wl to verify that:

• All tests still pass with the new validation
• No warnings appear during cleanup (unless actual file deletion issues exist)
• Invalid inputs would be properly rejected (you could test this by temporarily adding an invalid character to test_cases.txt)

---

Branch: fix/regression-test-generation |

[claude]

Code Review for PR 23

Thank you for this improvement to the test infrastructure! This PR successfully enables regression tests within AllTests.wl and adds proper cleanup.

Strengths

1. Well-Structured Solution - The PR correctly addresses the core issue by generating outputs before comparison, running comparisons via the module API, cleaning up generated files, and properly handling exit codes.

2. Security Considerations - Good addition of ValidShellInput validation to prevent shell injection attacks.

3. Smart Auto-Run Detection - The fix in compare_golden.wl properly distinguishes between direct execution and module loading.

Potential Issues

CRITICAL: Command Injection Vulnerability

Location: test/AllTests.wl:33

Problem: While you validate backend and testName, you do not validate TestDir. If TestDir contains special characters, it could lead to command injection.

Recommendation: Add validation for TestDir or use RunProcess instead of Run for safer command execution.

MEDIUM: Error Handling

Location: test/AllTests.wl:107-109

The warning message does not indicate what went wrong. Consider checking if file still exists after deletion attempt.

LOW: Code Duplication

The test case loading logic is duplicated between AllTests.wl and compare_golden.wl. Consider extracting this into a shared utility module.

Test Coverage

The test plan is excellent with all unit tests and 7 regression tests passing, exit code verification, and cleanup verification.

Recommendation: Consider adding a test case that intentionally fails to verify error handling.

Code Quality

Positive: Consistent formatting, clear variable naming, helpful progress messages, good use of Wolfram Language idioms.

Minor: The 2>&1 redirection may not work as expected with Run in Wolfram Language. Consider adding comments explaining the ScriptCommandLine check.

Security Assessment

Overall security posture is GOOD with input validation. Main recommendation is to extend validation to TestDir or use RunProcess.

Recommendation

APPROVE WITH MINOR FIXES

The PR successfully achieves its goals. The only blocking issue is the potential command injection via TestDir. Once addressed, this is ready to merge.

Action Items:

1. CRITICAL: Add TestDir validation or switch to RunProcess
2. OPTIONAL: Improve error message in cleanup section
3. OPTIONAL: Add failure test case to verify error handling