search

grafana / sqlds

A package that assists writing SQL-driven datasources
Apache License 2.0
17 stars 12 forks source link

add tests for query timeout #36

Closed kminehart closed 3 years ago

kminehart commented 3 years ago

Before this PR i really couldn't think of a good way to write tests for a sql.DB or QueryContext without importing a package like sqlmock. I've had a lot of trouble with sqlmock in the past.

I figured accepting an interface instead of a *sql.DB makes this testing process a lot easier. Let me know if there's anything else I should test while I'm in here.

Arguably this test itself isn't super valuable but at the very least it ensures that if the context in db.QueryRowContext is canceled, the cancellation causes an error to be returned by the query function.

kminehart commented 3 years ago

Nice! One thing that came to my mind was testing context cancellation, but we can't really test that it's being honored here? It's needs to be implemented and tested on the driver.

Yeah exactly. It's safe enough to assume that if you're using QueryContext then the query function will return when the context is cancelled. That being said, github.com/lib/pq may actually not support this completely. It's possible that Snowflake might not either. It's not really our problem, but for the sake of compatibility we may want to keep using QueryContext but handle our own context cancellation instead.