Add failing tests for #430: auto-fix fingerprint skip bug#432
Add failing tests for #430: auto-fix fingerprint skip bug#432gltanaka merged 8 commits intopromptdriven:mainfrom
Conversation
…ip bug This commit adds comprehensive test coverage for the bug where auto-fix success at sync_orchestration.py:1412 skips fingerprint save, causing incomplete metadata tracking. Tests added: - Unit test in test_sync_orchestration.py - E2E test in test_e2e_issue_430_autofix_fingerprint.py Both tests currently FAIL, reproducing the bug where: - operations_completed missing 'crash' entry - fingerprint not updated after auto-fix success - operation completion event not logged Tests will PASS once the fix is implemented. Related to promptdriven#430 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…c_orchestration.py:1350) Fixes promptdriven#430
The previous fixture used side_effect for code_generator_main which prevented individual tests from overriding the return value. This broke test_budget_exceeded which needs to set a custom cost value. Fixed by: - Reverting code_generator_main to use return_value in the fixture - Adding code file creation directly in test_auto_fix_success_saves_complete_metadata This allows test_budget_exceeded to work while maintaining the file creation needed for the auto-fix test.
The test was incorrectly checking for fields that don't exist in the Fingerprint dataclass. The Fingerprint only stores: - command (last operation) - file hashes - timestamp It does NOT store operations_completed, model, or cost. Changes: - Check 'command' field instead of 'operations_completed' - Remove assertions for non-existent 'model' and 'cost' fields - Fix filenames: basename_language.json (not _fingerprint.json) - Fix run report filename: basename_language_run.json (not _run_report.json) - Use atomic_state=None for E2E test (not dict)
- Remove emojis and excessive comments from E2E test - Remove unrelated calculator_example.py - Remove unrelated test_simple_verification.py
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #430 where the auto-fix success path in sync_orchestration.py skipped fingerprint saves and metadata tracking due to an early continue statement. The fix ensures that when auto-fix successfully resolves an import error, the operation is properly tracked and fingerprint metadata is saved before continuing to the next operation.
Changes:
- Added 3 lines before the continue statement at line 1415 to save fingerprint and track operation completion
- Added unit test to verify fingerprint saving and operations tracking for auto-fix success path
- Added E2E tests to validate complete metadata persistence after auto-fix
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_sync_orchestration.py | Added comprehensive unit test validating that auto-fix success saves fingerprint metadata and tracks 'crash' in operations_completed |
| tests/test_e2e_issue_430_autofix_fingerprint.py | Added E2E tests verifying fingerprint persistence and event logging for auto-fix scenarios |
| pdd/sync_orchestration.py | Fixed bug by adding fingerprint save and operation tracking before continue statement in auto-fix success path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review comment - extract call arguments into named variables for better readability and maintainability.
Serhan-Asad
left a comment
Serhan-Asad
left a comment
There was a problem hiding this comment.
Comment 1 (E2E test): The Fingerprint dataclass does not store 'model' or 'cost' fields. According to sync_determine_operation.py, Fingerprint only stores:
- command (last operation)
- pdd_version
- timestamp
- file hashes (prompt_hash, code_hash, example_hash, test_hash)
The model and cost are tracked separately in the workflow state, not persisted in the fingerprint file. The test correctly validates only the fields that exist.
Comment 2 (Unit test): Fixed in commit 575c6bf. Replaced magic index numbers with named variables for better readability.
The E2E test manually called _save_fingerprint_atomic() instead of exercising the sync orchestration flow where the bug occurs. This means it would pass even when the bug is present. The unit test in test_sync_orchestration.py is sufficient: - Exercises the actual buggy code path via sync_orchestration() - Verified to fail when bug is present (operations_completed missing 'crash') - Verified to pass when fix is applied - Properly tests the control flow bug Verified by temporarily reverting the fix and confirming the unit test catches the regression.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # We need to patch the internal functions that detect and fix import errors | ||
| with patch('pdd.sync_orchestration._run_example_with_error_detection') as mock_run_example, \ | ||
| patch('pdd.sync_orchestration._try_auto_fix_import_error') as mock_auto_fix, \ | ||
| patch('pdd.sync_orchestration._save_run_report_atomic') as mock_save_report: |
There was a problem hiding this comment.
The mock for _save_run_report_atomic is created but never verified in the assertions. Consider adding an assertion to verify it was called with the expected arguments to ensure the run_report saving behavior is properly tested.
| # the _save_fingerprint_atomic call at line 1716 | ||
| fingerprint_calls = [ | ||
| call for call in mock_save_fp.call_args_list | ||
| if len(call[0]) >= 3 and call[0][2] == 'crash' # arg[2] is operation |
There was a problem hiding this comment.
Using magic index [2] to access the operation argument is fragile. Consider using keyword argument matching or destructuring to make the test more maintainable and less prone to breakage if the function signature changes.
2 participants
Summary
Fixes the bug where auto-fix success skips fingerprint save, causing incomplete metadata tracking.
Changes
The Fix
Added 3 lines before the continue statement at line 1412 in sync_orchestration.py to ensure metadata is saved before early exit.
Root Cause
Control flow design oversight in the auto-fix success path. When auto-fix successfully resolves an import error, the continue statement at line 1412 immediately jumps to the next loop iteration, bypassing the critical metadata save logic.
Test Results
Fixes #430