Replace TempDir with TestArrayUri #102
Conversation
This removes our usage of the TempDir for tests and instead uses a new TestArrayUri trait. Eventually this will allow us to dynamically change whether tests run against a local file system or a remote cloud service instance. This does not yet support actually running against the cloud service as that will take more infra setup to get going. However, it should be as simple as creating a new TestCloudUri struct which then implements the TestArrayUri trait. Then the `test_util::get_uri_generator` can return either trait implementation at runtime based on an environment variable or whatever is easiest in CI.
| use crate::array::*; | ||
| use crate::config::Config; | ||
| use crate::query::{QueryBuilder, WriteBuilder}; | ||
| use crate::test_util::{self, TestArrayUri}; |
There was a problem hiding this comment.
Its the equivalent to use crate::test_util so that you can write test_util::get_uri_generator().
| self.base_dir() | ||
| } | ||
|
|
||
| fn with_path(&self, path: &str) -> TileDBResult<String> { |
There was a problem hiding this comment.
This resolves correctly?
It must, since you have a passing CI. I'm surprised though. Does it break if you use the trait?
There was a problem hiding this comment.
Not sure what resolution you're asking about here? The self.with_path(path) call and how its picked the TestDirectory implementation vs the trait-method-which-becomes-recursive issue? I contemplated if I'd need to cast things there but just went with the obvious first and it worked so I stopped thinking about it.
I think the "cast required to pick an implementation" only kicks in when you have multiple traits with duplicate names but I'd have to go read the spec closely to get to the root of it.
| } | ||
|
|
||
| fn with_path(&self, path: &str) -> TileDBResult<String> { | ||
| self.with_path(path) |
There was a problem hiding this comment.
Do you ever envision a different implementation? This could be a provided method
There was a problem hiding this comment.
Yeah, I waffled on that but had written the TestDirectory implementation before defining the trait. I'll go back and make it a provided method.
There was a problem hiding this comment.
Turns out the VFS tests (which can't run against the REST api) were using this so I had to keep the implementation on TestDirectory. Though its now provided so the impl TestArrayUri for TestDirectory doesn't have to specify it at least.
|
Tables repo needs something similar too now that I think about it |
This removes our usage of the TempDir for tests and instead uses a new TestArrayUri trait. Eventually this will allow us to dynamically change whether tests run against a local file system or a remote cloud service instance.
This does not yet support actually running against the cloud service as that will take more infra setup to get going. However, it should be as simple as creating a new TestCloudUri struct which then implements the TestArrayUri trait. Then the
test_util::get_uri_generatorcan return either trait implementation at runtime based on an environment variable or whatever is easiest in CI.