You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "blaginin (via GitHub)" <gi...@apache.org> on 2025/03/12 12:21:02 UTC
[I] [Epic] Add snapshot tests [datafusion]
blaginin opened a new issue, #15178:
URL: https://github.com/apache/datafusion/issues/15178
### Is your feature request related to a problem or challenge?
Follow up on https://github.com/apache/datafusion/pull/15165, related to https://github.com/apache/datafusion/issues/10373
We currently have a lot of inline assertions when something is tested against a large pre-defined string(s). For example:
https://github.com/apache/datafusion/blob/main/datafusion/core/tests/dataframe/mod.rs#L2472-L2496
The issue with this approach is that every time we update logic, each of those tests needs to be updated manually. To make updates faster and easier, we're switching to insta. This issue will be a collection of small good-first-issue tickets to complete the transition.
## Tickets to pick up
- [ ] todo
In addition to the previous list, you can manually find places to rewrite logic. Generally, everything that uses `assert_batches_sorted_eq`, `assert_batches_eq`, `assert_contains`, or just compares with a very big string is a good candidate.
### Describe the solution you'd like
We should switch those assertions to `insta` You can see one example of this transition in https://github.com/apache/datafusion/pull/15165
I made that change manually using IDE replace tools and a bit of regex. Initially, I just put `@"placeholder"` as a RHS and then ran `cargo insta test` to fill gaps and then confirmed it's the same as before via git diff. It didn't take a lot of time, but I think there are ways to make it faster:
- https://github.com/getgrit/gritql
- rust-analyzer Structured Search Replace
- Cursor / Other AI tool ☠️
### Describe alternatives you've considered
_No response_
### Additional context
_No response_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-3490387309
8 months of work, but we did it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "xudong963 (via GitHub)" <gi...@apache.org>.
xudong963 commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-2813142044
Places are still using old styles:
- [ ] core/tests/sql/explain_analyze.rs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-2730830528
> [@alamb](https://github.com/alamb) can I ask you to put "good first issue" on tickets in the list if you're happy with them? I don't think I have permission to do that
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-3180654273
we are so close
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-3180654895
We have only https://github.com/apache/datafusion/issues/15791 left
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "xudong963 (via GitHub)" <gi...@apache.org>.
xudong963 commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-2989610772
https://github.com/apache/datafusion/blob/5ca4ff02932eecdd203b1b90acaf4381c0d5cb5c/datafusion/proto/tests/cases/roundtrip_physical_plan.rs#L143 I think the roundtrip tests also can use insta?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "xudong963 (via GitHub)" <gi...@apache.org>.
xudong963 commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-3490360447
🎊
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-2813832937
for sure, will do! thank you!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-2726480816
Those places may be good to check
<img width="688" alt="Image" src="https://github.com/user-attachments/assets/1dfd4de9-131f-4244-8f08-8647fe3c2d13" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-2726487177
@alamb can I ask you to put "good first issue" on tickets in the list if you're happy with them? I don't think I have permission to do that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "shruti2522 (via GitHub)" <gi...@apache.org>.
shruti2522 commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-2723169995
take
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15178:
URL: https://github.com/apache/datafusion/issues/15178#issuecomment-2813827406
@blaginin any chance you could file some good first issue tickets to cover the places taht @xudong963 identified in https://github.com/apache/datafusion/issues/15178#issuecomment-2813142044 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] [Epic] Add snapshot tests (migrate to `insta` for tests) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #15178: [Epic] Add snapshot tests (migrate to `insta` for tests)
URL: https://github.com/apache/datafusion/issues/15178
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org