🧪 Add tests for RecoverableError::allows_partial and is_retryable#120
🧪 Add tests for RecoverableError::allows_partial and is_retryable#120bashandbone wants to merge 4 commits intomainfrom
RecoverableError::allows_partial and is_retryable#120Conversation
…tial and is_retryable Adds a new `MockRecoverableError` struct in the `error.rs` tests module to instantiate and test the `RecoveryStrategy` logic for the default `RecoverableError::allows_partial` and `RecoverableError::is_retryable` methods. Ensures that `Partial` and `Skip` are correctly identified for `allows_partial()` and `Retry` is correctly identified for `is_retryable()`. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideAdds focused unit tests for the default RecoverableError::is_retryable and allows_partial behavior by introducing a simple mock RecoverableError implementation and exercising all RecoveryStrategy variants plus the None case. Class diagram for RecoverableError and MockRecoverableError testsclassDiagram
class RecoverableError {
+recovery_info() Option_ErrorRecovery
+is_retryable() bool
+allows_partial() bool
}
class MockRecoverableError {
-recovery Option_ErrorRecovery
+MockRecoverableError(recovery Option_ErrorRecovery)
+recovery_info() Option_ErrorRecovery
}
class ErrorRecovery {
+strategy RecoveryStrategy
+instructions String
+auto_recoverable bool
}
class RecoveryStrategy {
}
class RecoveryStrategy_Partial
class RecoveryStrategy_Skip
class RecoveryStrategy_Retry {
+max_attempts int
}
class RecoveryStrategy_Fallback {
+strategy String
}
class RecoveryStrategy_Abort
RecoverableError <|.. MockRecoverableError
ErrorRecovery --> RecoveryStrategy
MockRecoverableError --> ErrorRecovery
RecoveryStrategy <|-- RecoveryStrategy_Partial
RecoveryStrategy <|-- RecoveryStrategy_Skip
RecoveryStrategy <|-- RecoveryStrategy_Retry
RecoveryStrategy <|-- RecoveryStrategy_Fallback
RecoveryStrategy <|-- RecoveryStrategy_Abort
Flow diagram for is_retryable and allows_partial decision logicflowchart TD
A[Start: RecoverableError instance] --> B{Has recovery_info}
B -->|None| C_is[is_retryable returns false]
B -->|None| C_ap[allows_partial returns false]
B -->|Some ErrorRecovery| D{strategy}
D -->|Retry| E_is_true[is_retryable returns true]
D -->|Retry| E_ap_false[allows_partial returns false]
D -->|Partial| F_is_false_p[is_retryable returns false]
D -->|Partial| F_ap_true[allows_partial returns true]
D -->|Skip| G_is_false_s[is_retryable returns false]
D -->|Skip| G_ap_true[allows_partial returns true]
D -->|Fallback| H_is_false_f[is_retryable returns false]
D -->|Fallback| H_ap_false_f[allows_partial returns false]
D -->|Abort| I_is_false_a[is_retryable returns false]
D -->|Abort| I_ap_false_a[allows_partial returns false]
C_is --> Z[End]
C_ap --> Z
E_is_true --> Z
E_ap_false --> Z
F_is_false_p --> Z
F_ap_true --> Z
G_is_false_s --> Z
G_ap_true --> Z
H_is_false_f --> Z
H_ap_false_f --> Z
I_is_false_a --> Z
I_ap_false_a --> Z
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
test_is_retryableandtest_allows_partialcases are quite repetitive; consider using a table-driven approach (e.g., iterating over(strategy, expected)pairs) to make the tests more compact and easier to extend. - You can reuse common
ErrorRecoveryfields (likeinstructionsandauto_recoverable) via a single helper or constant to avoid repeating the same literals in eachcreate_mock_errorcall.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test_is_retryable` and `test_allows_partial` cases are quite repetitive; consider using a table-driven approach (e.g., iterating over `(strategy, expected)` pairs) to make the tests more compact and easier to extend.
- You can reuse common `ErrorRecovery` fields (like `instructions` and `auto_recoverable`) via a single helper or constant to avoid repeating the same literals in each `create_mock_error` call.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Adds unit tests to close a coverage gap around the default helper methods on the RecoverableError trait, ensuring recovery behavior is explicitly asserted and guarded against regressions.
Changes:
- Added a mock
RecoverableErrorimplementation for testing default trait methods. - Added comprehensive tests for
RecoverableError::is_retryableacross allRecoveryStrategyvariants plusNone. - Added comprehensive tests for
RecoverableError::allows_partialacross allRecoveryStrategyvariants plusNone.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes a compiler error `E0425: cannot find value file_name in this scope` caused by declaring `_file_name` but referencing `file_name` in `crates/language/src/lib.rs`. Also includes previous test improvements: test: add tests for RecoverableError default trait methods allows_partial and is_retryable Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
Updates several examples in thread-ast-engine rustdocs that broke when upstream ast-grep trait constraints (like Clone and Debug for Language, or Document changes) were modified. This fixes the CI 'Test' suite failures. Also implements `Clone` and `Debug` on `Tsx` and `JavaScript` test languages. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…rates Fixes additional doc tests across `crates/ast-engine/src/*` where trailing `}` or missing variable scope issues were introduced from previous fixes. Also fixes `crates/language/src/lib.rs` and `html.rs` tests failing due to missing trait imports like `thread_ast_engine::tree_sitter::LanguageExt` and `Doc` when calling `ast_grep`. Test pass rates across `thread-ast-engine` and `thread-language` crates should now succeed. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
🎯 What: The testing gap addressed: added tests for the default
allows_partialandis_retryablemethods in theRecoverableErrortrait.📊 Coverage: What scenarios are now tested:
allows_partialis explicitly verified against all variants ofRecoveryStrategy(Partial,Skip,Retry,Fallback,Abort) andNoneto ensure it only returns true forPartialandSkip. The same comprehensive enumeration is also tested foris_retryablechecking for theRetryvariant.✨ Result: The improvement in test coverage: ensuring error recovery behavior is explicitly asserted for future regressions.
PR created automatically by Jules for task 13292274821357028093 started by @bashandbone
Summary by Sourcery
Add unit tests to verify default
RecoverableErrorbehavior for retryable and partial recovery scenarios.Tests:
is_retryableacross allRecoveryStrategyvariants and absence of recovery info.allows_partialacross allRecoveryStrategyvariants and absence of recovery info.