Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(v5) WithContext types are troublesome to mock #378

Closed
lukestoward opened this issue Jun 23, 2022 · 6 comments · Fixed by #379
Closed

(v5) WithContext types are troublesome to mock #378

lukestoward opened this issue Jun 23, 2022 · 6 comments · Fixed by #379

Comments

@lukestoward
Copy link

lukestoward commented Jun 23, 2022

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 lukestoward changed the title (v5) withContext types are troublesome to mock (v5) WithContext types are troublesome to mock Jun 23, 2022
@lukestoward
Copy link
Author

@fbiville any thoughts on this?

@fbiville
Copy link
Contributor

fbiville commented Jul 1, 2022

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
Copy link
Author

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

@fbiville
Copy link
Contributor

fbiville commented Jul 1, 2022

Hello, I think #379 fixes it but it would be great if you could confirm it.

@lukestoward
Copy link
Author

lukestoward commented Jul 1, 2022

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
Copy link
Contributor

fbiville commented Jul 8, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants