rethinkdb / rethinkdb-go

Go language driver for RethinkDB
Apache License 2.0
1.65k stars 182 forks source link

Add ability to block cursors when testing with mocks #460

Closed arnodel closed 5 years ago

arnodel commented 5 years ago

If you want the cursor to block on some of the response values, you can pass in a value of type chan interface{} to MockQuery.Return() and the cursor will block until a value is available to read on the channel. Or you can pass in a function with signature func() interface{}: the cursor will call the function (which may block). Here is the example above adapted to use a channel.

CMogilko commented 5 years ago

I don't think that using hacks in a production code (cursor.go is a production code) is a good idea. This affects performance and also breaks isolation, because now production code depends on testing data types. The right way is to mock the whole Cursor, but it isn't an interface due to some historical decision made by original author of the driver. But I don't see any strong reasons to break back compatibility only for testing. May be in the future it can happens.

arnodel commented 5 years ago

I agree it's a hack, but so is the whole query mocking approach in this library (hacking a Cursor by populating its buffer and tweaking its state). The performance impact is negligible (there is an added type assertion, which amount to comparing two pointers) - also note that turning Cursor into an interface would also have a performance impact.

Having the ability to test concurrent behaviour (e.g. selecting from a number of channels, including one obtained from listening to a cursor) is very valuable to us, and sadly in order to achieve this I had to resort to this kind of hack. As you point out, it is not possible to mock the Cursor so I feel that my hands are tied. I would be happy to learn about another approach though!

For me it's a practical compromise that is worth it as I think it is very important to be able to test this kind of behaviour. Happy to keep it as a fork though :)

Another solution would be not to mock the whole Cursor but only how it fetches new data (i.e. its fetchMore method or part of it). That would require more significant modifications to that method and to the Mock.Query method though, and I was not prepared to do that for lack of time at this point. However that would keep backward compatibility and not "break isolation". Would that seem feasible?

CMogilko commented 5 years ago

I reviewed your suggestion more detailed and found a way to implement this by mocking connection not cursor