sqlmock panics when closing empty Rows object about go-sqlmock

[diegommm]

Hi @raxod502-plaid! Thank you for your report. You are right that it shouldn't panic, that's a bug. It is also true that Exec would be preferable for executing any SQL that will never return any rows, like TRUNCATE.

Quick facts:

• A Query/QueryRow is expected to always return at least one RowSet.
• An Exec is like a Query/QueryRow whose RowSets are discarded (but has other properties).
• The panic happens because it tries to look for the first RowSet and the code doesn't check that there is none. This is the bug you describe.

Correct usage:

• The recommendation for your code is to use Exec as you found out. That's not a workaround, your code will benefit from explicitly stating that you are not expecting any RowSets to be returned, and additionally you may get a sql.Result (with rows affected and last inserted ID) if your driver supports it and if it makes sense for the operation.
• For code that could potentially return rows (like a SELECT, INSERT .. RETURNING, etc.), the correct expectation to be set is WillReturnRows(mock.NewRows(colNames)). This tells the mock that one RowSet is expected.
• It is valid to use WillReturnRows(mock.NewRows(nil)), because it states that you will expect exactly one RowSet with no columns, which is equivalent to an Exec discarding the sql.Result.

Consider a more complex example:

const myQuery := `
    SELECT disabled FROM users WHERE id = ?;
    DELETE FROM logins WHERE created_at < ?;
`
tx.QueryContext(ctx, myQuery, theID, theTimestamp)

That's a query returning two RowSets, the first one with a single column, and the second with none. A test expectation for it would be like:

mock.ExpectQuery(myQuery).WithoutArgs().WillReturnRows(
    mock.NewRows([]string{"disabled"}),
    mock.NewRows(nil),
)

Conclusions:

sqlmock should not panic this way if WillReturnRows is not passed any arguments. Instead, it should provide a help message explaining that it expects at least one (non-nil) argument to WillReturnRows. It should also not panic if NextResultSet is called and there is none provided, but instead fail the test stating that a call to that method was not expected (didn't check if that's already in place, just saying).