-
Notifications
You must be signed in to change notification settings - Fork 480
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
Move tests from parser.rs to appropriate parse_XX tests #845
Conversation
@@ -6912,40 +6912,6 @@ mod tests { | |||
}); | |||
} | |||
|
|||
#[test] | |||
fn test_parse_limit() { |
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.
I moved this code to sqlparser_common and sqlparser_mysql as appropriate, and used more concise functions to test these dialects
@@ -7402,24 +7368,6 @@ mod tests { | |||
); | |||
} | |||
|
|||
#[test] |
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 is postgres specific so moved to sqlparser_postgres
@@ -7458,11 +7406,4 @@ mod tests { | |||
)) | |||
); | |||
} | |||
|
|||
#[test] | |||
fn test_update_in_with_subquery() { |
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.
moved to sqlparser_postgres
@@ -1559,65 +1564,6 @@ fn parse_select_group_by() { | |||
); | |||
} | |||
|
|||
#[test] | |||
fn parse_select_group_by_grouping_sets() { |
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.
these tests are postgres specific (test with generic dialect and postgres) so moved to sqlparser_postgres
Pull Request Test Coverage Report for Build 4650441892
💛 - Coveralls |
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.
lgtm pending one question!
assert_eq!(ast.to_string(), sql.to_string()); | ||
}); | ||
|
||
let sql = "SELECT * FROM user LIMIT $1 OFFSET $2"; |
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.
Did we lose these tests? I see a mysql dialect test for offset but not one that touches the other dialects. Apologies if I missed it!
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.
Good spot. I should have called this out explicitly -- this test was replicated and therefore redundant.
The pre-existing test in sqlparser_common.rs is almost identical:
sqlparser-rs/tests/sqlparser_common.rs
Lines 6092 to 6104 in 784a191
let sql = "SELECT * FROM student LIMIT $1 OFFSET $2"; | |
let ast = dialects.verified_query(sql); | |
assert_eq!( | |
ast.limit, | |
Some(Expr::Value(Value::Placeholder("$1".into()))) | |
); | |
assert_eq!( | |
ast.offset, | |
Some(Offset { | |
value: Expr::Value(Value::Placeholder("$2".into())), | |
rows: OffsetRows::None, | |
}), | |
); |
Thanks for the review @ankrgyl |
Follow on to @ankrgyl 's suggestion in https://github.com/sqlparser-rs/sqlparser-rs/pull/842/files#r1149575835
I tried to clean up the tests a little both to make the current code a little nicer but also to help future contributors find the right place (and the right pattern)