refactor(scan) move test utils to test_utils.rs - #3107
Conversation
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey for this pr!
There was a problem hiding this comment.
We already have a crate leve test_utils, my suggestion is to split the crate level test_utils into a dir mod, and put different test utils into different file mods.
There was a problem hiding this comment.
Happy to give it a go, I was following the pattern we have elsewhere for puffin, arrow etc
There was a problem hiding this comment.
Have done the first part, let me know what you think. Didn't want to start moving other test_utils around in this PR but happy to follow up if we're happy with the direction?
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey , yes it's the right direction.
| #[cfg(test)] | ||
| pub(crate) mod scan; |
There was a problem hiding this comment.
No, we should mark the whole test_utils mod available only in test configuration.. I know it will break pub api, but it's necessary, these are not api.
There was a problem hiding this comment.
The issue is then we can't use these across crate boundaries:
error[E0432]: unresolved import `iceberg::test_utils`
--> crates/catalog/rest/src/catalog.rs:1668:18
--> crates/integrations/datafusion/tests/integration_datafusion_test.rs:34:14
--> crates/integrations/datafusion/src/physical_plan/project.rs:193:18
--> crates/integrations/datafusion/src/physical_plan/repartition.rs:185:18
There was a problem hiding this comment.
I just realized thate we have a test_utils crate, we can move shared utils there.
2 participants
Which issue does this PR close?
What changes are included in this PR?
scan mod.rs was bloated with a lot of test fixtures that are better placed in a test_utils.rs
Are these changes tested?
AI Disclosure