-
Notifications
You must be signed in to change notification settings - Fork 299
Description
Some context here: #503 (comment)
As part of the MemoryCatalog implementation, I wrote ~1500 lines worth of tests to comprehensively exercise catalog behaviour. These tests are now being reused in the SqlCatalog implementation by copy-pasting.
We should consider a more DRY approach like creating a new crate with those tests that can be called by a function, maybe something like this:
fn test_catalog<C: Catalog>(catalog: &C) {
todo!()
}
Although it might be better to pass in a function that can setup a new catalog so that each test can work on a fresh instance e.g.
/// Takes a function that accepts a FileIO and an optional warehouse_location and returns a fresh Catalog instance
fn test_catalog<C: Catalog>(setup_catalog_fn: impl FnOnce(FileIO, Option<String>) -> C) {
todo!()
}
The main concern I have is that the existing catalogs don't all follow exactly the same behaviour, there are small, subtle differences in behaviour like checking for tables before dropping a namespace, error messaging, etc. It may or may not be worth aligning behaviours but we can discuss all of that when we get there.
The other concern I have is developer-experience. We should still strive for clear test failures. I'm not sure a function-based approach as illustrated above will offer a good DevX but it may still be worth the compromise. In other languages, this would just be an abstract class for me but I'm not sure what the Rust equivalent is at this moment in time.
This is currently blocked by: #503
Metadata
Metadata
Assignees
Labels
Type
Projects
Status