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

Change wording of error message for unset query return value #105

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MasterCarl
Copy link

I forgot to call .WillReturnRows(...) on my ExpectQuery statement, which gave me the following error message:

Query 'INSERT INTO "Message" (.+) VALUES ($1, $2, $3),($4, $5, $6);$' with args [{Name: Ordinal:1 Value:1}], must return a database/sql/driver.Rows, but it was not set for expectation *sqlmock.ExpectedQuery as ExpectedQuery => expecting Query, QueryContext or QueryRow which:
  - matches sql: 'INSERT .+'
  - is with arguments:
    0 - 1

This lead me to believe there was something wrong with the query regex, or the arguments. I changed the wording so the message would read:

No required return value of type database/sql/driver.Rows was set for expectation *sqlmock.ExpectedQuery as ExpectedQuery => expecting Query, QueryContext or QueryRow which:
  - matches sql: 'INSERT .+'
  - is with arguments:
    0 - 1

I removed the query that was sent to sqlmock from the error message since it has nothing to do with the failure.

@codecov-io
Copy link

codecov-io commented Jan 5, 2018

Codecov Report

Merging #105 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #105   +/-   ##
=======================================
  Coverage   89.66%   89.66%           
=======================================
  Files          12       12           
  Lines         648      648           
=======================================
  Hits          581      581           
  Misses         47       47           
  Partials       20       20
Impacted Files Coverage Δ
sqlmock.go 90.14% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7b3087...ebac755. Read the comment docs.

@l3pp4rd
Copy link
Member

l3pp4rd commented Feb 19, 2018

Hi, sorry for late reply, will check this when I can

@l3pp4rd
Copy link
Member

l3pp4rd commented Apr 22, 2018

well, I do not think it needs changes, the reason is available in the message. read your own comment, you were expecting an Query but instead, the SQL is for INSERT of course, rows will not be there. The reason why the SQL string is visible first in error is exactly for that reason, because people more often mix up ExpectExec with ExpectQuery.

In case if many more people will open issues with similar problems, I will not change it yet. There must be a proper reason, now to me it all looks like 1 user out of 1000 was confused, why should be 999 confused users instead of 1?

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.

None yet

3 participants