db.Close() only sometimes satisfies mock.ExpectClose()

[alewgbl]

trafficstars

I've found a case where calling db.Close() in different ways only sometimes satisfies mock.ExpectClose(). For example, if I call sqlmock.New() and then call defer db.Close() on the DB handle that is returned, mock.ExpectClose() is not satisfied. However if I perform a nil check on the deferred result of db.Close() then mock.ExpectClose() is satisfied.

I would expect all cases of db.Close() to be consistent: either satisfy or don't satisfy mock.ExpectClose().

My goal is to catch non-nil errors from db.Close() and ensure they are logged.

Test cases showing the behaviour:

func Test_SQLMock_DBClose_Pass(t *testing.T) {
	db, mock, _ := sqlmock.New()
	defer db.Close()
	assert.Nil(t, mock.ExpectationsWereMet())
}

func Test_SQLMock_DBClose_AlsoPass(t *testing.T) {
	db, mock, _ := sqlmock.New()
	defer func() {
		_ = db.Close()
	}()
	assert.Nil(t, mock.ExpectationsWereMet())
}

func Test_SQLMock_DBClose_Fail(t *testing.T) {
	db, mock, _ := sqlmock.New()
	defer func() {
		err := db.Close()
		if err != nil {
			t.Fatal(err)
		}
	}()
	assert.Nil(t, mock.ExpectationsWereMet())
}

go version go1.12.7 linux/amd64 go-sqlmock v1.3.3

[l3pp4rd]

Hi, I think defer will defer the close to happen after the assertion right? instead it should be called before the assertion

[alewgbl]

@l3pp4rd It's true that the defer call will close the DB handle at the end of the function, however the behaviour is different depending on what's in that deferred function call.

Also, it doesn't make sense to me that I must close the DB handle after I make the assertion. What if some other error happens before the defer call is made? I'm not so sure I'd get to log an error from the db.Close() call at that point.

[l3pp4rd]

Well, I do not get why you are closing it in a test anyway. It is a mock database, it does not need to be closed. If the logic you are testing, is closing db, then you can make that expectation. Even when this method is deffered, you will call it usually from your test.

I actually do not know why it may close it before, maybe go garbage collector notices this in some way, or all conections were closed. But I do not see any problem here unless it is in the go db driver, maybe closing db has interesting behavior

[l3pp4rd]

have you investigated the behavior of close?

[mdaloia]

@l3pp4rd: I think @alewgbl is closing the db handle as per the example on the README.md

The thing going on here is:

• On the 3 cases the call to db.Close() is returning an error with the message "all expectations were already fulfilled, call to database Close was not expected" (debugging this line passes 3 times)
• The first 2 cases are ignoring it.
• The third one is using that error to fail the test.

On the 3 cases, at the point of the assertion, all the expectation are met but then a later call to db.Close() is executed and it doesn't know that we already asserted that expectations were met.

If you add a call to mock.ExpectClose() before the assertion all the 3 cases fails because no db.Close() was called before the assertion.

For what I understand, sqlmock is strict on the assertion of expectations. You must have a corresponding mock.ExpectXYZ() for every DB interaction, otherwise executing that interaction will fail with an error saying something like "call to ... was not expected".

[maraino]

I've been experiencing this also, this is working for me:

func mockDB(t *testing.T) (*sql.DB, sqlmock.Sqlmock) {
	t.Helper()

	db, mock, err := sqlmock.New()
	if err != nil {
		t.Fatalf("an error '%s' was not expected when opening a stub database connection", err)
	}
	t.Cleanup(func() {
		if err := mock.ExpectationsWereMet(); err != nil {
			t.Errorf("db expectations failed: %s", err)
		}
		// For some reason if we want to close the db, we also have to call
		// this, if not the previous method fails 🤷‍♂️
		mock.ExpectClose()
		if err := db.Close(); err != nil {
			t.Errorf("error closing the db: %w", err)
		}
	})
	return db, mock
}

If I remove the mock.ExpectClose() it fails randomly, and the same happens if I move this before the mock.ExpectationsWereMet.

mock.ExpectClose()
if err := db.Close(); err != nil {
	t.Errorf("error closing the db: %w", err)
}

[l3pp4rd]

Hi, very likely then that from some go version it now closes db in separate go routine and though synchronously we can no longer receive the close call to the driver. Which is something I would not expect since closing may give an error