-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
tests: add postgres integration tests #8
Conversation
f105826
to
1311ddb
Compare
I've merged #7 so we can merge this once it's reviewed/approved. |
func testDeleteUsers(t *testing.T, rw dbw.Writer) { | ||
t.Helper() | ||
testCtx := context.Background() | ||
_, err := rw.Exec(testCtx, "delete from users", nil) | ||
require.NoError(t, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no tests for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in the tests which would fail if it wasn't working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything that calls it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the coverage report doesn't show it being called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove it. Ty!
PR #7 includes WithPgPlaceholder() option(fa247ec) and we will wait for it to be merged, before updating the mql dependency in this PR and merging.