search

neo4j / neo4j-go-driver

Neo4j Bolt Driver for Go
Apache License 2.0
495 stars 70 forks source link

(v5) WithContext types are troublesome to mock #378

Closed lukestoward closed 2 years ago

lukestoward commented 2 years ago

I've been experimenting with the Context aware v5.0.0 preview and I've ran into some issues when trying to write unit tests.

In version 4.X of the Go package interface types such as Result, Session and Transaction have exported methods only. Which can easily be mocked using tools such as vektra/mockery. However, when using version 5.0.0-preview, the new context aware types ResultWithContext, SessionWithContext and TransactionWithContext contain new unexported methods such as legacy() which in some cases return unexported types such as *result.

Whilst mockery can still create mocks for the interfaces, I am unable to use the mock ResultWithContext as the legacy() method has an unexported return value *result.

I get the following error when building using the mock of ResultWithContext:

mocks/ResultWithContext.go:398:46: result not exported by package neo4j

Would it be possible to remove the unexported methods on those interfaces, or more importantly, remove the unexported return value *result.

If you would like to generate the mock yourself, run the following:

$ go install github.com/vektra/mockery/v2@latest
$ mockery --with-expecter --exported --dir $(go list -m -f '{{ .Dir }}' github.com/neo4j/neo4j-go-driver/v5)/neo4j --name ResultWithContext --output mocks
lukestoward commented 2 years ago

@fbiville any thoughts on this?

fbiville commented 2 years ago

Hi @lukestoward, first of all: thanks for trying out the preview version and providing feedbck!

The unexported legacy method is needed so that the legacy APIs can easily delegate to the new context-aware APIs. Let me try to see if I can at least change the return type to a public interface.

Note that the hypothetical fix would only applied to the 5.0 branch, which contains many more changes now.

lukestoward commented 2 years ago

Hi @fbiville, that shouldn't be a problem. Thanks for taking a look and let me know how you get on.

fbiville commented 2 years ago

Hello, I think https://github.com/neo4j/neo4j-go-driver/pull/379 fixes it but it would be great if you could confirm it.

lukestoward commented 2 years ago

Hi @fbiville, this does indeed look like it's fixed the issue.

One caveat is that I am still unable to use vektra/mockery as there is an issue with providing an implementation for an interface with unexported methods. This is more a limitation with mockery, since it is possible to satisfy an interface with unexported methods in a different package by embedding the interface directly in your type. That said it's a minor thing, but I will have to manually implement my own mocks. (unless I'm missing an option with the mock-gen tool 🤔)

Anyway, thank you for addressing this issue. 👍

fbiville commented 2 years ago

Back from sick leave 🐛 Unfortunately, I need these unexported methods on the interface, at least as long as the legacy "non-context.Context" API are around, so most likely for the whole lifetime of 5.x versions.