-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 some create/drop tests to ddl.slt
#4535
Conversation
324557d
to
1ffe19b
Compare
1ffe19b
to
9917a6c
Compare
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.
@@ -0,0 +1,2 @@ | |||
Jorge,2018-12-13T12:12:10.011Z |
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 files used to be generated programatically in Rust, instead I have checked them in for the tests
@@ -182,5 +182,234 @@ DROP VIEW foo, bar; | |||
statement ok | |||
DROP VIEW foo; | |||
|
|||
# Ok to drop |
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 think the SLT files are much easier to add / work with ❤️
} | ||
|
||
#[tokio::test] | ||
#[should_panic(expected = "doesn't exist")] |
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.
One difference is that .slt files don't seem to offer a way to expect a certain error message (seemingly by design). I am not sure if you have any thoughts / experience on this @xudong963
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 think it can be supported in sqllogictest-rs.
Currently, databend has written its own python version of the logictest framework, which is able to add error messages, such as statement error 1006
: 1006 is errorcode.
So I think it's a reasonable feature ~ ping @xxchan.
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.
We do support error messages:
statement error <regexp>
and query error <regexp>
example can be found here https://github.com/risinglightdb/sqllogictest-rs/blob/main/examples/basic/basic.slt#L20
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.
Sweet -- hank you @xxchan I will update these tests to use that syntax
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 added the expected messages in 6be3662 -- very cool! It was good enough that I see a few bugs
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.
Here is an improvement to fix the messages: #4572
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, the notes in the test are great too, @alamb you are as meticulous as ever 🧑🍳 !
Draft as it builds on #4530Which issue does this PR close?
Part of #4495
Rationale for this change
In #4495 we are porting tests from rust to sqllogictest for speed and consistency
What changes are included in this PR?
Port some rust based tests in
create_drop.rs
to sqllogictestsThere are a few tests I can't really port at the moment as they require programatic manipulation of the SessionContext (both information schema as well as table factories).
Are these changes tested?
Yes
Are there any user-facing changes?