Migrate user_defined tests to insta #15255

shruti2522 · 2025-03-15T21:24:58Z

Which issue does this PR close?

Closes Migrate user_defined tests to insta #15247 .

Rationale for this change

What changes are included in this PR?

migrate tests in datafusion/core/tests/user_defined to insta

Are these changes tested?

Are there any user-facing changes?

alamb · 2025-03-16T10:16:57Z

cc @blaginin

alamb · 2025-03-16T10:17:09Z

Thanks @shruti2522 !

blaginin · 2025-03-16T10:38:38Z

Hey! I’m a bit worried that now to understand the test you’ll need to look into two places: open the test file and also search for the snapshot file… that’s why in the example pr (#15165) I used inline snapshots.

do you think we should use the same approach here?

shruti2522 · 2025-03-16T13:43:05Z

Hey! I’m a bit worried that now to understand the test you’ll need to look into two places: open the test file and also search for the snapshot file… that’s why in the example pr (#15165) I used inline snapshots.

do you think we should use the same approach here?

Hey @blaginin, you are right about this, at first I tried implementing the inline snapshots only, but I ran into errors while running tests for run_and_compare_query in user_defined/user_defined_plan.rs, since multiple tests are using it ,only the first one works because insta doesn't allow duplicate inline snapshots by default. I tried allow_duplicates!(), but it gets a bit tricky with async functions. So, I used snapshot files instead because they make it much easier to run multiple tests using the same query fn, also it makes it much easier to update expected outputs in the future and eliminates the use of #[rustfmt::skip] in some places. Maybe we can discuss some alternative approach to implement inline snapshots 🤔

blaginin · 2025-03-17T12:57:56Z

I tried allow_duplicates!(), but it gets a bit tricky with async functions

Can you please explain more on this? I tried modifying async run_and_compare_query and this worked for me:

So, I used snapshot files instead because they make it much easier to run multiple tests using the same query fn, also it makes it much easier to update expected outputs in the future and eliminates the use of #[rustfmt::skip] in some places.

Just to give a bit of context, we had a discussion about inline / file snapshots here: #13672 (comment) and #13672 (comment).

To me personally, it feels like we can use snapshot files, but the snapshot itself should give enough information to understand the context. For example, in CLI, you can see program input and tests, and the snapshot file itself doesn't contain a lot of logic. Here I'm not sure if that's the case for ALL the test fixtures? Maybe some of them can be inline?

alamb

Thanks @blaginin and @shruti2522

I do think that inline snapshots would be better (and more consistent with the rest of the code). Can you please update the PR as suggested by @blaginin and mark the PR ready for review when done?

Thanks again for this (and all the other recent PRs)

shruti2522 · 2025-03-17T19:18:50Z

I tried allow_duplicates!(), but it gets a bit tricky with async functions

Can you please explain more on this? I tried modifying async run_and_compare_query and this worked for me:
> So, I used snapshot files instead because they make it much easier to run multiple tests using the same query fn, also it makes it much easier to update expected outputs in the future and eliminates the use of #[rustfmt::skip] in some places.
Just to give a bit of context, we had a discussion about inline / file snapshots here: #13672 (comment) and #13672 (comment).

To me personally, it feels like we can use snapshot files, but the snapshot itself should give enough information to understand the context. For example, in CLI, you can see program input and tests, and the snapshot file itself doesn't contain a lot of logic. Here I'm not sure if that's the case for ALL the test fixtures? Maybe some of them can be inline?

Yes, it worked. I was using insta::allow_duplicates! like a function call:

 insta::allow_duplicates!(|| {
        insta::with_settings!({
            description => description,
        }, {
            insta::assert_snapshot!(actual, @r###"
            +-------------+---------+
            | customer_id | revenue |
            +-------------+---------+
            | paul        | 300     |
            | jorge       | 200     |
            | andy        | 150     |
            +-------------+---------+
            "###);
        });
    })();

On changing it to this it worked

 insta::allow_duplicates! {
        insta::with_settings!({
            description => description,
        }, {
            insta::assert_snapshot!(actual, @r###"
            +-------------+---------+
            | customer_id | revenue |
            +-------------+---------+
            | paul        | 300     |
            | jorge       | 200     |
            | andy        | 150     |
            +-------------+---------+
        "###);
        });
    }

Thanks a lot @blaginin and @alamb ❤️

alamb

Thank you @shruti2522 and @blaginin -- thi slooks great to me!

blaginin · 2025-03-18T12:39:35Z

datafusion/core/tests/user_defined/expr_planner.rs

+fn fmt_batches(batches: &[RecordBatch]) -> String {
+    use arrow::util::pretty::pretty_format_batches;
+    match pretty_format_batches(batches) {
+        Ok(formatted) => formatted.to_string(),
+        Err(e) => format!("Error formatting record batches: {}", e),
+    }
+}


can we use batches_to_string for that?

blaginin · 2025-03-18T12:44:44Z

datafusion/core/tests/user_defined/user_defined_window_functions.rs

+fn fmt_batches(batches: &[RecordBatch]) -> String {
+    use arrow::util::pretty::pretty_format_batches;
+    match pretty_format_batches(batches) {
+        Ok(formatted) => formatted.to_string(),
+        Err(e) => format!("Error formatting record batches: {}", e),
+    }
+}
+


i think this can be removed as well

blaginin · 2025-03-18T12:45:10Z

datafusion/core/tests/user_defined/user_defined_table_functions.rs

+fn fmt_batches(batches: &[RecordBatch]) -> String {
+    use arrow::util::pretty::pretty_format_batches;
+    match pretty_format_batches(batches) {
+        Ok(formatted) => formatted.to_string(),
+        Err(e) => format!("Error formatting record batches: {}", e),
+    }
+}
+


This is a good call -- I took the liberty of pushing a commit to make the change.

blaginin

thank you!!

alamb · 2025-03-18T19:25:54Z

Thanks @shruti2522 and @blaginin 🙏

migrate user_defined tests to insta

1d7de54

github-actions bot added the core Core DataFusion crate label Mar 15, 2025

shruti2522 and others added 7 commits March 16, 2025 02:57

Merge branch 'main' into insta_user

b35d630

fix tests

b0d18df

test

46bb92f

fmt

40b9a12

update snaps

1e4d20f

add snaps

08ebef5

add snaps

d695e53

alamb reviewed Mar 17, 2025

View reviewed changes

alamb marked this pull request as draft March 17, 2025 16:08

shruti2522 added 3 commits March 18, 2025 00:21

add inline snapshots

259aedd

fix

fe1447c

remove snaps

5d4ada3

shruti2522 marked this pull request as ready for review March 17, 2025 19:26

alamb approved these changes Mar 18, 2025

View reviewed changes

blaginin reviewed Mar 18, 2025

View reviewed changes

alamb added 2 commits March 18, 2025 13:56

Reuse batches_to_stirng

c6cdabd

Merge remote-tracking branch 'apache/main' into insta_user

6f86fe5

blaginin approved these changes Mar 18, 2025

View reviewed changes

alamb merged commit 8c8b245 into apache:main Mar 18, 2025
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Migrate user_defined tests to insta #15255

Migrate user_defined tests to insta #15255

Uh oh!

shruti2522 commented Mar 15, 2025

Uh oh!

alamb commented Mar 16, 2025

Uh oh!

alamb commented Mar 16, 2025

Uh oh!

blaginin commented Mar 16, 2025

Uh oh!

shruti2522 commented Mar 16, 2025 •

edited

Loading

Uh oh!

blaginin commented Mar 17, 2025 •

edited

Loading

Uh oh!

alamb left a comment

Uh oh!

shruti2522 commented Mar 17, 2025

Uh oh!

alamb left a comment

Uh oh!

blaginin Mar 18, 2025

Uh oh!

blaginin Mar 18, 2025

Uh oh!

blaginin Mar 18, 2025

Uh oh!

alamb Mar 18, 2025

Uh oh!

blaginin left a comment

Uh oh!

alamb commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

Migrate user_defined tests to insta #15255

Migrate user_defined tests to insta #15255

Uh oh!

Conversation

shruti2522 commented Mar 15, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb commented Mar 16, 2025

Uh oh!

alamb commented Mar 16, 2025

Uh oh!

blaginin commented Mar 16, 2025

Uh oh!

shruti2522 commented Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blaginin commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

shruti2522 commented Mar 17, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

blaginin Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

blaginin Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

blaginin Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

blaginin left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

shruti2522 commented Mar 16, 2025 •

edited

Loading

blaginin commented Mar 17, 2025 •

edited

Loading