mhuxtable commented 4 years ago

Implemented an ExpectPing method to watch for calls to Ping on the driver.

Some complexity: Ping is used on open to drive internal sql library behaviour to open a connection. We don't want these pings to be treated as expectations so there's a mechanism to override this checking in some cases.

In order to neatly implement the behaviour without duplicating the Sqlmock interface, the ExpectPing method is installed in the mock in all Go versions, but does nothing in pre-1.8 instances.

codecov-io commented 4 years ago

Codecov Report

Merging #204 into master will increase coverage by 0.39%. The diff coverage is 96.61%.

@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
+ Coverage   91.11%   91.51%   +0.39%     
==========================================
  Files          15       16       +1     
  Lines         743      801      +58     
==========================================
+ Hits          677      733      +56     
- Misses         48       49       +1     
- Partials       18       19       +1

Impacted Files	Coverage Δ
options.go	`100% <100%> (ø)`	:arrow_up:
sqlmock_go18.go	`100% <100%> (ø)`	:arrow_up:
sqlmock.go	`93.89% <100%> (+0.05%)`	:arrow_up:
sqlmock_before_go18.go	`100% <100%> (ø)`
expectations.go	`85.13% <81.81%> (-0.27%)`	:arrow_down:

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 36d18c9...dd0fe2a. Read the comment docs.

l3pp4rd commented 4 years ago

Hi, this would be a backward incompatible change. since in some tests users currently have, they may call ping, but not expect it. This would fail their tests in upgraded version.

maybe it could be a separate option in sqlmock.New construction like one of [these] (https://github.com/DATA-DOG/go-sqlmock/blob/master/options.go#L17) to enable ping expectations, since we have to maintain the backward compatibility as much as we can. And this is not a breaking change. By default it should be ignored unless option is provided.

mhuxtable commented 4 years ago

this would be a backward incompatible change

That's a fair point. I was also concerned about potential backwards incompatibility around extending the Sqlmock interface with a new ExpectPing method, although I don't envisage anyone trying to implement their own instance of Sqlmock. Does this concern you?

mhuxtable commented 4 years ago

@l3pp4rd I've pushed some changes which avoid breaking backwards compatibility – thanks for pointing this out.

There's a decision around the API which doesn't have a clear answer to me. I've flip-flopped in my preferred approach in the commits above, as you can see:

ExpectPing is a new method on the Sqlmock interface, so no existing code could be calling this. When the user does not pass the MonitorPingsOption option to the mock, ExpectPing can either behave:

as a no-op, as if pings were not enabled. At this point, we can choose to return an empty ExpectedPings struct or a nil pointer. I favour returning a nil pointer to indicate an error to the user, but am not keen because the way the library is used suggests this won't be checked and it will lead to unclear/unhelpful nil pointer deref panics.
as it's a new method, treat the call as the user expecting Ping mocking to be enabled and enable it, as if they passed MonitorPingsOption. This is the behaviour I've now implemented in a subsequent commit. It shouldn't break backwards compatibility as anyone calling ExpectPing will hopefully update their code to suit.

I'm not really happy about this though. I think I actually prefer the first option of just ignoring all the calls unless the option is supplied. What are your thoughts?

mhuxtable commented 4 years ago

I've decided that having just one good way of doing anything is the better option, and I'll just require the option to be passed to enable the expect pings behaviour...

mhuxtable commented 4 years ago

I've decided that having just one good way of doing anything is the better option, and I'll just require the option to be passed to enable the expect pings behaviour...

I've done this now.

l3pp4rd commented 4 years ago

sqlmock interface should not cause any problems, it rather should be a struct from the start, but by mistake was implemented as interface, may change this later in the future. will review it a bit later, thanks ;)

l3pp4rd commented 4 years ago

looks good, thank you ! and apologies for the delay

DATA-DOG / go-sqlmock

Implement ExpectPings to watch for Ping attempts on the database #204

Codecov Report