-
-
Notifications
You must be signed in to change notification settings - Fork 301
Improve error messages for ast-grep test failures based on failure type #2174
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2174 +/- ##
==========================================
+ Coverage 86.10% 86.19% +0.09%
==========================================
Files 107 107
Lines 16865 17012 +147
==========================================
+ Hits 14522 14664 +142
- Misses 2343 2348 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: HerringtonDarkholme <[email protected]>
ast-grep test failure
HerringtonDarkholme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address the code feedback
crates/cli/src/verify/reporter.rs
Outdated
| } | ||
| /// A hook function runs after tests completed. | ||
| fn after_report(&mut self, results: &[CaseResult]) -> Result<(bool, String)> { | ||
| fn after_report(&mut self, results: &[CaseResult]) -> Result<(bool, ErrorContext)> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not return (bool, ErrorContext). If the test succeed there is no error.
Please design a proper data type for test result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 988729f. Replaced the (bool, ErrorContext) return type with a proper TestResult enum that has Success and Failure variants. This eliminates the confusion between success and error states.
crates/cli/src/verify/reporter.rs
Outdated
| passed += 1; | ||
| } else { | ||
| failed += 1; | ||
| for status in &result.cases { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to result struct? Give it a name like is_snapshot_mismatch_only_failure also add comment why this method exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 988729f. Moved the logic to CaseResult::is_snapshot_mismatch_only_failure() method with documentation explaining its purpose for determining appropriate error messages when test failures occur.
| WriteFile(PathBuf), | ||
| // Test | ||
| TestFail(String), | ||
| TestFailSnapshotMismatch(String), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pleae change it to TestSnapshotMismatch for a shorter name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 988729f. Renamed TestFailSnapshotMismatch to TestSnapshotMismatch for a more concise name.
| DiagnosticError(_) => 1, | ||
| ProjectNotExist | LanguageNotSpecified | RuleNotSpecified | RuleNotFound(_) => 2, | ||
| TestFail(_) => 3, | ||
| TestFailSnapshotMismatch(_) => 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge this test to the line above using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 988729f. Merged the test cases using TestFail(_) | TestSnapshotMismatch(_) => 3 for cleaner code.
| ), | ||
| TestFailSnapshotMismatch(message) => Self::new( | ||
| message, | ||
| "It looks like your rule is working correctly but the snapshot test needs to be updated. You can run the test with `--update-all` to update all snapshots, or use `--interactive` to selectively update snapshots.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message is too verbose. Please change it in the same style as other error message. Stick to the philosophy mentioned in the comment.
// * Use consistent terminology
// * Be clear and concise
// * Provide context
// * Suggest a fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 988729f. Simplified the error message to "Run with --update-all to update all snapshots, or --interactive to update selectively." This follows the existing style guidelines: clear, concise, provides context, and suggests a fix.
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ssages - Replace (bool, ErrorContext) return type with proper TestResult enum - Move snapshot mismatch detection logic to CaseResult::is_snapshot_mismatch_only_failure method with documentation - Rename TestFailSnapshotMismatch to TestSnapshotMismatch for brevity - Merge test cases using | operator where appropriate - Simplify error message to be more concise and consistent with existing style - Add comprehensive unit tests for new functionality Co-authored-by: HerringtonDarkholme <[email protected]>
HerringtonDarkholme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address comment
| ), | ||
| TestSnapshotMismatch(message) => Self::new( | ||
| message, | ||
| "Run with `--update-all` to update all snapshots, or `--interactive` to update selectively.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add context like "test failed due to mismatching snapshots"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a009b97. Added contextual information "Test failed due to mismatching snapshots" to provide clear reason for the failure.
…messages Co-authored-by: HerringtonDarkholme <[email protected]>
HerringtonDarkholme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address comment
Co-authored-by: HerringtonDarkholme <[email protected]>
This PR improves the error messages shown when
ast-grep testfails by providing context-aware help based on the type of failures encountered.Problem
Previously, when
ast-grep testfailed, users would always see a generic message directing them to the playground:However, this wasn't helpful when the rule was actually working correctly but the snapshot tests just needed to be updated. Users would waste time debugging rules that didn't need fixing.
Solution
The implementation now analyzes the types of test failures and provides appropriate guidance:
When all failures are snapshot mismatches:
When there are mixed failures or non-snapshot failures:
Implementation Details
TestResultenum withSuccess,RuleFail, andMismatchSnapshotOnlyvariants with proper separation of concernsCaseResult::is_snapshot_mismatch_only_failure()to use a single loop instead of multiple iterator chains with allocationsTesting
The change includes comprehensive unit tests covering snapshot-only failures, mixed failures, and successful test scenarios. All existing tests continue to pass, ensuring backward compatibility.
Fixes #2173.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.