test: integration tests for `duckdb` #178

phdah · 2025-02-05T09:31:39Z

Description

This follows the style and structure of both the postgres and bigquery integration tests.

What's left?

Edit: There seems to be some issue with the adapter. Will investigate it. Resolved by commit 98bac14ceda3:

Add Archived state in GetResult to not run async

This is a fix provided by @MattiasMTS in:
http://github.com/kndndrj/nvim-dbee/pull/177/files#diff-5234895d59f82777fe5b7e9e89ec74f054246a480b63d168a00142551ef6c6bcR48

Seems to be a race condition for creating the table returning empty result of the GetResult() function.
The GetStructure() returns empty. Not sure if this is expected or not.
- Resolved by adding more depth to the driver's structure.
Want to look into if/how we can switch databases (catalog in DuckDB).
- Resolved by adding the current database to show, but not allowing switching

Status

    --- PASS: TestDuckDBTestSuite/TestListOnlyOneDatabase (0.00s)
    --- PASS: TestDuckDBTestSuite/TestShouldCancelQuery (0.10s)
    --- PASS: TestDuckDBTestSuite/TestShouldFailInvalidQuery (0.00s)
    --- PASS: TestDuckDBTestSuite/TestShouldReturnColumns (0.00s)
    --- PASS: TestDuckDBTestSuite/TestShouldReturnManyRows (0.10s)
    --- PASS: TestDuckDBTestSuite/TestShouldReturnOneRows (0.11s)
    --- PASS: TestDuckDBTestSuite/TestShouldReturnStructure (0.03s)

Fixes

Added schema to DuckDB driver:

Added catalog

phdah · 2025-02-14T13:25:36Z

There might be some overlap with this PR: #77

MattiasMTS

nice work! ✨ left some few comments suggestions. Love some tests! 🧪

dbee/tests/testhelpers/helper.go

MattiasMTS · 2025-02-14T16:27:39Z

dbee/tests/integration/duckdb_integration_test.go

+}
+
+// SetupSuite initializes an in-memory DuckDB instance.
+func (suite *DuckDBTestSuite) SetupSuite() {


wdyt about moving some of the ddl stuff to the NewDuckDBContainer construct, similar to e.g. what we did in #177 ?

Yes, would you suggest to also use a ddl seeds file for this, or just the initialization of it should go in New*Container methods?

let's make any sql statements part of a seed file for easier scalability 😄 IMHO makes it easier to read what is happening in terms of 'logic' 😄

so, you make a seed file for the sql file, then bootstrap that file as part of the NewDuckDBContainer construct :)

MattiasMTS · 2025-02-14T16:30:12Z

dbee/adapters/duck_driver.go

+// NOTE: (phdah) As of now, swapping catalogs is not enabled
+func (c *duckDriver) SelectDatabase(name string) error {
+	// no-op
+	return nil


let's return an error here of fmt.Errorf("SelectDabase method not implemented yet") or something similar. Otherwise, it might be easy to confuse the user that there is a no-op even tho I am selecting a new database 'succesfully'. wdyt? 😄

Sounds good, let me try it out and see 😃

So returning the error is kinda obstructing. Since the ListDatabases() only return the database you initially connected to, you can't really change anyway right now. I would suggest to keep the no-op. Thoughts?

aha, ok gotcha!
Hmm I am tempted to say we should wait implementing these methods in a separate PR, then to actually implement them with no-op. Given, we don't want to confuse a user with feature of seeing databases but they aren't seeing what they 'expect'. Then it make sense to show nothing at all (because the feature doesn't exist) 😄

Do you think we'll make a PR adding this before the v0.2.0 release? 😋 if so, we can add this code and update it accordingly

So after not to much looking in to this, one can attach to new/more databases. Then you can run use other_db and swap between them. But I would say that this is not really how one typically use DuckDB. But I liked the visual "feedback" of seeing what database I was attached to. But if it's possible to disable the select action only, that would maybe be a hybrid solution?

Regardless, I think you're right that this can be it's own PR after v0.2.0 😃 So, lets do that!

dbee/adapters/duck_driver.go

MattiasMTS · 2025-02-14T16:38:58Z

dbee/tests/testhelpers/duckdb.go

+}
+
+// NewDuckDBContainer creates a new in-memory DuckDB instance.
+func NewDuckDBContainer(params *core.ConnectionParams) (*DuckDBContainer, error) {


perhaps a bit overengineered but you could make use of the generic testcontainer and boot up duckdb in that see (https://golang.testcontainers.org/features/creating_container/#genericcontainer). Might be useful in the future to remove any remaining local duckdb files or so.

Just adding a comment for idea sharing, no need to move away from the code there is now 😄

Thanks, and good point. Not sure if/how duckdb stores in memory databases, but would assume they clean up after themselvs somehow. But in the future when we want to try connecting to persistent databases, perhaps this would be great!

This follows the style and structure of both the postgres and bigquery integration tests.

This is currently just to make the UI show what catalog you start your connection to. So no swapping is enabled. I did play around with `use my_catalog` etc, in the `SelectDatabase()` function, but didn't get it to properly work due to how the `Swap()` function works.

This actually showed issues with the helper function, so great!

phdah changed the title ~~Add integration tests for duckdb~~ test: integration tests for duckdb Feb 5, 2025

phdah changed the title ~~test: integration tests for duckdb~~ test: integration tests for duckdb Feb 5, 2025

phdah force-pushed the phdah/integration-tests-duckdb branch 2 times, most recently from 900bfc8 to 58e11b9 Compare February 5, 2025 10:34

phdah force-pushed the phdah/integration-tests-duckdb branch from 58e11b9 to 97e47c4 Compare February 13, 2025 10:31

phdah marked this pull request as ready for review February 13, 2025 12:50

phdah force-pushed the phdah/integration-tests-duckdb branch from 97e47c4 to a29b1dc Compare February 14, 2025 09:39

phdah mentioned this pull request Feb 14, 2025

chore: update Structure and Columns for duckdb driver #77

Closed

phdah force-pushed the phdah/integration-tests-duckdb branch from a29b1dc to 581f147 Compare February 14, 2025 15:51

MattiasMTS reviewed Feb 14, 2025

View reviewed changes

phdah added 4 commits February 14, 2025 17:52

Add schema to driver structure

5ed0f67

Add integration tests for duckdb

5fd30b9

This follows the style and structure of both the postgres and bigquery integration tests.

Add unit test for helper function

563a215

This actually showed issues with the helper function, so great!

phdah force-pushed the phdah/integration-tests-duckdb branch from 5b6f1f8 to 563a215 Compare February 14, 2025 16:52

phdah added 2 commits February 14, 2025 18:11

review: update struct name

6a25557

review: update database var name

0893cf6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: integration tests for `duckdb` #178

test: integration tests for `duckdb` #178

phdah commented Feb 5, 2025 •

edited

Loading

phdah commented Feb 14, 2025

MattiasMTS left a comment

MattiasMTS Feb 14, 2025

phdah Feb 14, 2025

MattiasMTS Feb 14, 2025

MattiasMTS Feb 14, 2025

MattiasMTS Feb 14, 2025

phdah Feb 14, 2025

phdah Feb 14, 2025

MattiasMTS Feb 14, 2025

phdah Feb 14, 2025

MattiasMTS Feb 14, 2025

phdah Feb 14, 2025

test: integration tests for duckdb #178

Are you sure you want to change the base?

test: integration tests for duckdb #178

Conversation

phdah commented Feb 5, 2025 • edited Loading

Description

What's left?

Status

Fixes

phdah commented Feb 14, 2025

MattiasMTS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

test: integration tests for `duckdb` #178

test: integration tests for `duckdb` #178

phdah commented Feb 5, 2025 •

edited

Loading