fixes leaking datasets tests #2730
Conversation
…ne fixture in most places
✅ Deploy Preview for dlt-hub-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rudolfix
force-pushed
the
chores/fixes-leaking-datasets-tests
branch
from
549c5bb to
19b1139
Compare
| @@ -92,14 +94,12 @@ def open_connection(self) -> duckdb.DuckDBPyConnection: | |||
| if first_connection: | |||
| # TODO: we need to frontload the httpfs extension for abfss for some reason | |||
| if self.is_abfss: | |||
| self._conn.sql("INSTALL https; LOAD httpfs;") | |||
| self._conn.sql("INSTALL httpfs; LOAD httpfs;") | |||
There was a problem hiding this comment.
they were aliases it seems!
| @@ -167,15 +167,50 @@ def test_storage() -> FileStorage: | |||
|
|
|||
|
|
|||
| @pytest.fixture(autouse=True) | |||
| def autouse_test_storage() -> FileStorage: | |||
| return clean_test_storage() | |||
| def autouse_test_storage(request) -> Optional[FileStorage]: | |||
There was a problem hiding this comment.
this is maybe out of scope, but all of these fixtures need good names and docstrings and should be added to the docstring linter, I often look up what they do exactly in code if I need to disable something for testing. This one is a good example, if you never looked it up the name autouse_test_storage gives no real hint about what it does.
There was a problem hiding this comment.
right! we'll dedicate a week to refactor our utils and release them as OSS lib. version in dlt+ has nice docstrings btw.
| if "no_load" in request.keywords: | ||
| # always deactivate | ||
| Container()[PipelineContext].deactivate() | ||
| Container()[PipelineContext].clear_activation_history() |
There was a problem hiding this comment.
hm, maybe we want to keep the activation history here so we can clean up with drop_active_pipeline_data after a bunch of "no_load" tests have run without having to store the pipelines?
There was a problem hiding this comment.
I added this because most of no_load tests produce empty or fake pipelines ie. with destinations that are not instantiated. so they were leaking into subsequent tests and breaking them on cleanup. I'll keep it util we have a good case
| @dataclass | ||
| class WithLocalFiles(BaseConfiguration): | ||
| """Mixin to BaseConfiguration that shifts relative locations into `local_dir` and allows for a few special locations. | ||
| :pipeline: in the pipeline working folder |
There was a problem hiding this comment.
I know this was already like this, but why not "pipeline_working_dir"? This way it is clear what this variable means.
There was a problem hiding this comment.
hmmm you can add QoL ticket for that. I like :pipeline: because it is like :memory:
Description
local_dirin tests so local files are automatically in_storage(no more *duckdb) databases after testsmore in commits