Test for parse(unparse(parse()) is identity #129

alamb · 2022-12-10T20:49:38Z

Follow on to #127

Here is a proposed method for testing that parsing the results of unparsing are the same records.

alamb · 2022-12-10T20:50:12Z

sqllogictest/src/parser.rs

@@ -137,6 +137,35 @@ pub enum Record {
    Injected(Injected),
 }

+/// Newtype that allows comparing a RegEx


I am not a super fan of this, but I can't think of any better way to implement PartialEq in a more maintainable way.

What about https://mcarton.github.io/rust-derivative/latest/cmp.html#compare-with

That seems to have worked well 👍

alamb · 2022-12-10T20:50:39Z

sqllogictest/src/parser.rs

+    /// Replaces the actual filename in all Records with
+    /// "__FILENAME__" so different files with the same contents can
+    /// compare equal
+    fn normalize_filename(records: Vec<Record>) -> Vec<Record> {


I am also not a super fan of this patter either, but it is what I can come up with

What about using derivative crate to ignore a field when comparing? https://mcarton.github.io/rust-derivative/latest/cmp.html#ignoring-a-field

I'll give it a try 👍

alamb · 2022-12-10T21:14:32Z

@xxchan if you are OK with this approach, I'll add tests for the other example files as a follow on PR

xxchan · 2022-12-10T21:37:01Z

sqllogictest/src/parser.rs

@@ -679,6 +712,34 @@ mod tests {
            UsefulDiffDisplay(&changeset),
            output_contents,
        );
+


IMHO if unparse(parse()) is identity (i.e., the above assert passed), then parse(unparse(parse())) isn't necessary. Should we remove the assert above? 🤔

🤔 I really like it :) But I can remove it if you prefer

I don't dislike it, but I proposed parse(unparse(parse())) just becasue parse_roundtrip can fail in such cases.

// Technically this will not always be the same due to some whitespace normalization // // query III // select * from foo; // ---- // 1 2 // // Will print out collaposting the spaces between `query` // // query III // select * from foo; // ---- // 1 2

If we don't care or test that case, I think we don't even need to test parse(unparse(parse())) at all. The old one is enough.. 🤪 How do you think?

For my usecase downstream apache/datafusion#4570, just need the parse(unparse(parse())) guarantee.

I don't really need the parse_roundtrip guarantee. It would be nice if the results of printing a Record will look pretty close to its input so that the "test script completion mode" is minimally disruptive to the rest of the records

Thus I think I will try add specific tests for reprinting certain records and change the tests for parsing the example files to ensure that the files survive roundtrip

alamb · 2022-12-11T10:14:26Z

sqllogictest/Cargo.toml

@@ -10,6 +10,7 @@ description = "Sqllogictest parser and runner."

 [dependencies]
 async-trait = "0.1"
+derivative = "2.2.0"


Use derivative as suggested by @xxchan

alamb · 2022-12-11T10:14:40Z

sqllogictest/src/parser.rs

@@ -81,6 +81,7 @@ pub enum Record {
        conditions: Vec<Condition>,
        /// 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.
+        #[derivative(PartialEq(compare_with = "cmp_regex"))]


👍 much nicer

alamb · 2022-12-11T10:15:12Z

sqllogictest/src/parser.rs

+        parse_roundtrip("../examples/validator/validator.slt")
+    }
+
+    /// Verifies Display impl is consistent with parsing by ensuring


Removed the "display roundtrip check" (I will try and propose a more specific test in a follow on PR)

Signed-off-by: Andrew Lamb <[email protected]>

alamb commented Dec 10, 2022

View reviewed changes

alamb force-pushed the alamb/test_reparse branch from 1a66f68 to b5d6008 Compare December 10, 2022 21:11

alamb mentioned this pull request Dec 10, 2022

Improve the ability to parse/unparse files #127

Merged

xxchan reviewed Dec 10, 2022

View reviewed changes

alamb force-pushed the alamb/test_reparse branch from 8fae6dc to 968de75 Compare December 11, 2022 10:16

alamb commented Dec 11, 2022

View reviewed changes

alamb added 4 commits December 11, 2022 05:16

Test for parse(unparse(parse()) is identity

94403fe

Signed-off-by: Andrew Lamb <[email protected]>

Simplify test logic

2127b45

Signed-off-by: Andrew Lamb <[email protected]>

Use derivate macro

5b324fb

Signed-off-by: Andrew Lamb <[email protected]>

Remove contents check, add rest of example files

4fa73cb

Signed-off-by: Andrew Lamb <[email protected]>

alamb force-pushed the alamb/test_reparse branch from 968de75 to 4fa73cb Compare December 11, 2022 10:16

This was referenced Dec 11, 2022

Add update_record_with_output function and tests #130

Merged

Make it easier to update sqltestlogic test expected output ("test script completion mode") apache/datafusion#4570

Closed

alamb requested a review from xxchan December 11, 2022 11:19

xxchan approved these changes Dec 11, 2022

View reviewed changes

xxchan merged commit 46752d1 into risinglightdb:main Dec 11, 2022

alamb deleted the alamb/test_reparse branch December 12, 2022 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test for parse(unparse(parse()) is identity #129

Test for parse(unparse(parse()) is identity #129

alamb commented Dec 10, 2022

alamb Dec 10, 2022

xxchan Dec 10, 2022

alamb Dec 11, 2022

alamb Dec 10, 2022

xxchan Dec 10, 2022

alamb Dec 11, 2022

alamb commented Dec 10, 2022

xxchan Dec 10, 2022

alamb Dec 10, 2022

xxchan Dec 10, 2022 •

edited

Loading

alamb Dec 11, 2022

alamb Dec 11, 2022

alamb Dec 11, 2022

alamb Dec 11, 2022 •

edited

Loading

Test for parse(unparse(parse()) is identity #129

Test for parse(unparse(parse()) is identity #129

Conversation

alamb commented Dec 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

xxchan Dec 10, 2022 •

edited

Loading

alamb Dec 11, 2022 •

edited

Loading