Migrate core test to insta part 3 #16978
Conversation
Chen-Yuan-Lai
force-pushed
the
migrate_core_test_to_insta_part_3
branch
from
16959ba to
5cb1e98
Compare
…namic snapshot assertions
Chen-Yuan-Lai
force-pushed
the
migrate_core_test_to_insta_part_3
branch
from
5cb1e98 to
0e07645
Compare
There was a problem hiding this comment.
Thank you for this PR @Chen-Yuan-Lai -- unfortuately I am not likely going to be able to find the contiguous time needed to review a PR this large.
I wonder if you would be willing to help make it easier to review, by splitting it up into smaller PRs ?
Also I think the structure of some of these tests is pretty gnarly to begin with (e.g. macros with many parameters). Maybe we can work out a better pattern as part of this work if we are going to rewrite the whole thing anyways
Maybe we can try to port a few tests in one of the files to use iinsta to make sure we are good with the pattern before applying the pattern to the entire thing
For example, a more explicit Test style struct might make the options being tested more efficient.
I know this is a big ask, but I think getting these tests into better shape would really help the project
| /// when the flag `prefer_existing_sort` is `true` for bounded cases. | ||
| /// * `$PLAN`: The plan to optimize. | ||
| /// * `$SOURCE_UNBOUNDED`: Whether the given plan contains an unbounded source. | ||
| macro_rules! assert_optimized_in_all_boundedness_situations { |
There was a problem hiding this comment.
oh yeah, I remember this macro -- this is crazy
Sure! I will try it soon |
|
Thank you so much @Chen-Yuan-Lai |
|
Any idea if you will be able to split this PR into smaller pieces @Chen-Yuan-Lai ? I can try to get an AI agent to do it too if you prefer |
|
@alamb Sure, feel free to use an AI agent to split this PR! Sorry for the long delay - I've been short on time. Thanks for offering to help with this! |
# Conflicts: # datafusion/core/tests/physical_optimizer/enforce_distribution.rs
|
Resolved one minor conflict |
happy to do the split too! |
that would be great too -- thank you ! |
|
I ported part of the sort_enforcement tests here: |
# Conflicts: # datafusion/core/tests/physical_optimizer/enforce_sorting.rs # datafusion/core/tests/physical_optimizer/partition_statistics.rs
|
i'll chunk this into more prs |
😍 |
|
I'll prioritize getting them reviewed |
# Conflicts: # datafusion/core/tests/physical_optimizer/replace_with_order_preserving_variants.rs
3 participants
Which issue does this PR close?
coretests toinsta#15791 .Rationale for this change
Related #16324 #16617
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
No