Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add tests for query timeout #36

Merged
merged 6 commits into from
Aug 30, 2021
Merged

add tests for query timeout #36

merged 6 commits into from
Aug 30, 2021

Conversation

kminehart
Copy link
Collaborator

@kminehart kminehart commented Aug 27, 2021

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 kminehart linked an issue Aug 27, 2021 that may be closed by this pull request
Copy link
Collaborator

@sunker sunker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great to have some integration tests.

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.

mmh, sqlmock worked quite well for me in the past (even though I have never used anything super complex). I actually added it here at #30. Something I should be aware of?

query_integration_test.go Outdated Show resolved Hide resolved
@kminehart
Copy link
Collaborator Author

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.

@kminehart kminehart merged commit 42cb4ea into main Aug 30, 2021
@sasklacz sasklacz deleted the context-tests branch January 9, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test(s) for query timeout
3 participants