Repository navigation
Docs: Assert exact parser errors in some negative tests - #2713
Open
s5dsn-eqee wants to merge 1 commit into
Open
s5dsn-eqee wants to merge 1 commit into
s5dsn-eqee wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2713 +/- ##
==========================================
+ Coverage 81.95% 81.97% +0.02%
==========================================
Files 42 42
Lines 34702 35011 +309
Branches 34702 35011 +309
==========================================
+ Hits 28440 28702 +262
- Misses 2735 2744 +9
- Partials 3527 3565 +38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
LucaCappelletti94
requested changes
Oct 24, 2026
| - New unit tests should be added to the `tests` module in the corresponding dialect file (e.g., `tests/sqlparser_redshift.rs` for Redshift), and should be placed at the end of the file. | ||
| - If the new functionality is gated using a dialect function, and the SQL is likely relevant in most dialects, tests should be placed under `tests/sqlparser_common.rs`. | ||
| - Cover both positive and negative cases: valid input parses and round-trips, invalid input fails with the expected error. | ||
| - Cover both positive and negative cases: valid input parses and round-trips, invalid input fails with the expected error. For some of them, assert the exact error text on a full statement, not just `is_err()`. |
Contributor
There was a problem hiding this comment.
I thought this was already blocked by the clippy lint now?
Contributor
Author
There was a problem hiding this comment.
Not fully, as far as I can see. The lint only fires on a stored result like
assert!(res.is_err()). A direct call still passes clippy, for example:
Contributor
Author
There was a problem hiding this comment.
s5dsn-eqee
force-pushed
the
agents-exact-error-tests
branch
from
October 24, 2026 13:50
9813534 to
6a04207
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docs: Assert exact parser errors in some negative tests
Motivation: when a negative test asserts the exact error, a change to the
parser branch that rejects the input shows up as a failing test. That helps
a developer or an agent see which branches a PR changed, straight from the
test diff. A bare
is_err()passes no matter which branch rejected the input.The usual objection is maintenance: when error messages change, updating
many exact assertions by hand is slow and tedious. With an LLM, updating
them is now a few minutes of work, so the extra signal is worth the cost.