-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Split up monster test_window_partial_constant_and_set_monotonicity into smaller functions #17952
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
Conversation
8d1f211
to
6b1bdc3
Compare
use insta::assert_snapshot; | ||
use std::sync::{Arc, LazyLock}; | ||
|
||
// Function definition - Alias of the resulting column - Arguments of the function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote the harness to be a proper structure rather than macro, and then carefully ported each test
with the help of copilot magic auto complete.
I did not let copilot stick in the expected plan output -- instead I used the pre-existing plan output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified the above:
- manually checked some tests
- wrote a code to compare before and after: https://gist.github.com/blaginin/903ccb038360f89711a4ab1711a729c2
- manually changed some before/auto to confirm it's working
dc624c6
to
8267fb3
Compare
@blaginin I don't know if you are interested/willing to review another massive test refactor PR but this would really help me in my quest to update to arrow 57 |
of course! will check now |
I totally agree that this test needs to be changed - but just wanted let you know - if you run |
I totally agree that this test needs to be changed - but just wanted let you know - if you run cargo insta test instead of cargo test - it'll update everything at once 🙂 TIL |
BTW I think CI will fail due to unrelated reasons: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much cleaner and easier to debug! 🚀
have two more small prs to migrate core, hoping to do them today evening or tomorrow and that should be it |
if there's anything else to migrate feel free to throw for me too! |
Awesome -- thanks @blaginin -- I will stand by for reviews |
Which issue does this PR close?
test_window_partial_constant_and_set_monotonicity
from enforce_sorting #17903Rationale for this change
I am working on the arrow 57 upgrade and I found I had to update the output of
many tests. I broke some of the changes out into their own PR for easier review
This one in particular breaks the 1200+ line test
test_window_partial_constant_and_set_monotonicity
, including the 63 test cases in into individual tests so they are easier to update.Without this change, updating the 63 tests requires running the following commands in a loop (63 times):
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?