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

mock: Add mockgen workflow #313

Merged
merged 2 commits into from
Nov 16, 2021
Merged

mock: Add mockgen workflow #313

merged 2 commits into from
Nov 16, 2021

Conversation

positiveblue
Copy link
Contributor

@positiveblue positiveblue commented Nov 8, 2021

  • make mock: generates all the mocks for testing.
  • mock-check: generates all the mocks + checks if any *.go file changed.
  • make gen: generates all the auto generated files (protos + mocks).
  • Add mock-check to our CI

Create mocks for sidecar/interfaces.go and internal/test/interfaces.go and use them for rewriting the TestRegisterSidecar.

@positiveblue positiveblue force-pushed the mock-gen branch 11 times, most recently from 0f821e1 to 6740c26 Compare November 9, 2021 06:28
@positiveblue positiveblue marked this pull request as ready for review November 9, 2021 07:15
@positiveblue positiveblue requested a review from guggero November 9, 2021 07:17
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool to see the gomock package in action!
I really like fact that we can generate very dynamic mocks on demand. And the way to use them also looks pretty easy to get started with.

This example use case also shows nicely why it's great to work with mocks: one can more easily test all possible branches as you demonstrated by adding two additional test cases.

IMO we should have a discussion about using gomock in lnd as well and take this PR as a base for the discussion.

sidecar_acceptor_test.go Outdated Show resolved Hide resolved
sidecar_acceptor_test.go Outdated Show resolved Hide resolved
sidecar_acceptor_test.go Outdated Show resolved Hide resolved
sidecar_acceptor_test.go Outdated Show resolved Hide resolved
Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see another mock in action!

Imo mock is a necessary component that's missing from our unit tests. It makes tests much easier and, in particular, since we have many goroutines flying around, I find it very helpful to validate whether a call does happen inside another goroutine.

As mentioned before, lnd and naut have already started using mocks (thanks to @bhandras!). The challenge I see for lnd is that it's missing interfaces, where it makes the mocking impossible. There is an ongoing effort made by @Crypt-iQ where we want to build more interfaces inside lnd's peer. And we def need more to make the mocking happen.

That being said, what's your opinion about the two tools, go/mock and testify/mock? Curious about your preference here. I chose testify/mock because we were already using testify inside lnd and from this article. Tbh I don't have that much experience using mocks inside go so that choice is more or less like, we just need A mock😂

sidecar_acceptor_test.go Show resolved Hide resolved
@Crypt-iQ
Copy link

As mentioned before, lnd and naut have already started using mocks (thanks to @bhandras!). The challenge I see for lnd is that it's missing interfaces, where it makes the mocking impossible. There is an ongoing effort made by @Crypt-iQ where we want to build more interfaces inside lnd's peer. And we def need more to make the mocking happen.

When we get native fuzzing in golang, mocks will be very helpful. Since I imagine the golang fuzzer is coverage-driven, it is good to separate components and make them "dumb" to not confuse the coverage signals (e.g. so they don't spawn goroutines, have nondeterministic behavior, etc). Also if you want to run a lot of fuzzers, you don't want something like an invoiceregistry or htlcswitch in lnd on each fuzzing instance if it's just testing round-trip parsing of a particular component, so mocking helps there too.

- `make mock`: generates all the mocks for testing.
- `mock-check`: generates all the mocks + checks if any *.go file changed.
- `make gen`: generates all the auto generated files (protos + mocks).
- Add `mock-check` to our CI
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few nits 🎉

Sorry if I'm being extra nit-picky, but if this is going to be our template for mock-usage, things should look extra tidy because they are going to be copy/pasted a lot 😅

sidecar_acceptor_test.go Outdated Show resolved Hide resolved
sidecar_acceptor_test.go Outdated Show resolved Hide resolved
sidecar_acceptor_test.go Show resolved Hide resolved
sidecar_acceptor_test.go Outdated Show resolved Hide resolved
sidecar_acceptor_test.go Show resolved Hide resolved
Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go mock go!

Create mocks for `sidecar/interfaces.go` and `internal/test/interfaces.go`
and use them for rewriting the `TestRegisterSidecar`.
@guggero
Copy link
Member

guggero commented Nov 11, 2021

Just waiting for @Roasbeef's opinion on using generated mocks, otherwise this is ready to be merged.

@positiveblue positiveblue merged commit dbd72b8 into master Nov 16, 2021
@guggero guggero deleted the mock-gen branch November 17, 2021 08:35
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.

4 participants