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

add mocks generation + add worker mocks #1239

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sapk
Copy link

@sapk sapk commented Mar 24, 2023

This PR add mock generation with latest version and add worker.Registry and worker.Worker mocks.

I want to be able to unit test registration of my worker so I need at least the registry mock. If I generate on my side it link to internal package which is forbidden. I can remediate this limitation on my side with writing a custom mock but I feel it maybe useful to other to have a clean generate one in this repository along side the others provided mocks.

I validated that I can use this in my repository by using a replace in the go.mod since we can link to internal in the same repository.

Risk should be low as it only update or add new mocks.

Some part of this PR are probably opinionated (ex go:generate comments) so let me know if it need any change.

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2023

CLA assistant check
All committers have signed the CLA.

@Groxx
Copy link
Contributor

Groxx commented Mar 28, 2023

tbh I'm planning on deleting these mocks entirely in a future version. mocks can be generated by users with the library of their choice, rather than being stuck with our code / tool versions / etc.

I haven't dug into them super closely, but AFAICT they aren't relying on any internal/ visibility rules, so they should be fully possible to build externally. or are there in fact problems with that?

@sapk
Copy link
Author

sapk commented Mar 28, 2023

Yes the main reason for the need for internal is that mockery translate the aliased options struct to internal ones. Like in the generated one (ex:worker) in this MR.

It can be work around by doing a manual version of the mock to not rely on the aliased internal options.

Maybe the real solution is to fix mockery to not uncover the alias.

Otherwise, I totally understand if you want to remove them. It was just to ease if someone else had the same need.

@Groxx
Copy link
Contributor

Groxx commented Mar 28, 2023

Yeah, we've been having that issue with mockery and gomock internally too.

Gomock at least has a "source" mode in the flags (as opposed to using reflection), which seems to work better.

Otherwise, I'm hoping that we'll have a v2 of this client library soon (a literal v2, so you can use both and migrate gradually), and that'll definitely be getting rid of those internal aliases. There's just no way to do so without making other breaking changes, so it has to wait for v2.

@Groxx
Copy link
Contributor

Groxx commented Mar 28, 2023

but in the meantime! if you're interested, I think this could work. I want to pin versions though, and bake them into the makefile - it's not too hard.

do you want to attempt that, or should I just prep a commit/? for you? I'm not entirely sure how to do that within github, but it's easy enough to just prep something to copy/paste.

@Groxx
Copy link
Contributor

Groxx commented Mar 29, 2023

eh, that took more fiddling than expected.
Groxx@ff949d2 <- if you want to add those changes, we can merge this :)

(there may be more to merge in and re-tidy soon, sorry about that if so)

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 this pull request may close these issues.

3 participants