search

duckdb / dbt-duckdb

dbt (http://getdbt.com) adapter for DuckDB (http://duckdb.org)
Apache License 2.0
895 stars 83 forks source link

Enable testing with file DB and fix bug in connection handling #131

Closed vlieven closed 1 year ago

vlieven commented 1 year ago

I noticed that the tests are only run against a :memory: database. This PR modifies the tests according to the dbt documentation to add testing profiles, allowing to switch between an in-memory and file-backed DuckDB backend (memory is kept as the default). The test can be run using a file by running pytest --profile file.

This change surfaces a bug in the Environment class, where the connection is prematurely closed due to the handles counting. I think that the Environment.close(cursor) behaviour is unwanted both for memory as file databases, which would make the handle counting unneeded as well. I simplified the implementation, fixing the issue, but I'm not sure if the handle counting served another purpose?

I suppose the tox.ini and github workflows should be updated as well to include the second testing path, but I'll let you review these changes first. Thanks!

jwills commented 1 year ago

Ah, this is so great-- I ran into this bug myself last night when I was working on my remote environment branch and I said to myself "hmm, probably time to add pytest profiles" right before I went to sleep. Thank you @vlieven!

vlieven commented 1 year ago

Happy to help! Should I update the tox and workflow, or how do you want to proceed?

jwills commented 1 year ago

@vlieven if you don't mind updating the workflow, I would be grateful-- also happy to do it myself (read: have ChatGPT do it for me)

jwills commented 1 year ago

Realizing it's pretty late in Belgium, gonna go the merge-and-chatgpt-it route