Skip to content
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

feat: match multiline error #200

Merged
merged 9 commits into from
Nov 9, 2023

Conversation

BugenZhao
Copy link
Collaborator

@BugenZhao BugenZhao commented Nov 8, 2023

Support matching multiline error message under ---- for both statement error and query error.

query error
SELECT 1/0;
----
db error: ERROR: Failed to execute query

Caused by these errors:
  1: Failed to evaluate expression: 1/0
  2: Division by zero

The output error message must be the exact match of the expected one to pass the test, except for the leading and trailing whitespaces. Users may use --override to let the runner update the test files with the actual output.

Empty lines are allowed in the expected error message. As a result, the message must end with two consecutive empty lines.

Breaking changes in the parser:

  • Add new variants to ParseErrorKind. Mark it as #[non_exhaustive].
  • Change the type of expected_error from Regex to ExpectedError, which is either a inline Regex or multiline String.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao BugenZhao marked this pull request as ready for review November 8, 2023 09:26
@BugenZhao BugenZhao changed the title feat: match multiline error under ---- feat: match multiline error Nov 8, 2023
@BugenZhao
Copy link
Collaborator Author

Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Haven't reviewed yet) @wangrunji0408 requested multi line mode for query last time (To test select jsonb_pretty). Could the work for these two features be shared?


forget it

@@ -102,8 +101,7 @@ pub enum Record<T: ColumnType> {
label: Option<String>,
/// The SQL command is expected to fail with an error messages that matches the given
/// regex. If the regex is an empty string, any error message is accepted.
#[educe(PartialEq(method = "cmp_regex"))]
expected_error: Option<Regex>,
expected_error: Option<ExpectedError>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First I'm thinking whether it's necessary to introduce the error enum. Why not put it in expected_results. But it should be problematic.

I think we've messed the Record type. 🤣 To be more correct, it should be like enum of error or results. Some fields are common, and some belong only to query.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm. I'm happy to live with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same opinion with you. 😕 It's a sum type instead of a product type. We may refactor them in future PRs.

sqllogictest/src/parser.rs Outdated Show resolved Hide resolved
sqllogictest/src/parser.rs Outdated Show resolved Hide resolved
}

/// Returns whether the given error message matches the expected one.
pub fn is_match(&self, err: &str) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn is_match(&self, err: &str) -> bool {
pub fn matches(&self, err: &str) -> bool {

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how regex names. 😕 Just to keep interface unchanged.

sqllogictest/src/parser.rs Outdated Show resolved Hide resolved
sqllogictest/src/parser.rs Outdated Show resolved Hide resolved
sqllogictest/src/parser.rs Show resolved Hide resolved
sqllogictest/src/parser.rs Outdated Show resolved Hide resolved
Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just some minor points

@xxchan
Copy link
Member

xxchan commented Nov 8, 2023

@skyzh @wangrunji0408 can we add @BugenZhao as a colaborator.

@skyzh
Copy link
Member

skyzh commented Nov 9, 2023

@skyzh @wangrunji0408 can we add @BugenZhao as a colaborator.

Yep let's do that!

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao
Copy link
Collaborator Author

BugenZhao commented Nov 9, 2023

Wow, semver-check gets really slow (up to 30min).

@BugenZhao BugenZhao merged commit 7ee44cd into risinglightdb:main Nov 9, 2023
4 checks passed
@BugenZhao BugenZhao deleted the bz/multiline-error branch November 9, 2023 06:38
@BugenZhao
Copy link
Collaborator Author

@xxchan Would you please release a new version for this? 🥰

@xxchan
Copy link
Member

xxchan commented Nov 9, 2023

@BugenZhao Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants