-
Notifications
You must be signed in to change notification settings - Fork 607
Add archive mode #633
base: main
Are you sure you want to change the base?
Add archive mode #633
Conversation
Can you rebase this PR with main to resolve the conflicts and restart the tests? |
"os" | ||
|
||
"github.com/golang/mock/mockgen/model" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this follows google's Golang style guide for grouping imports, i.e. std, local then third-party.
|
||
Archive mode generates mock interfaces from a package archive | ||
file (.a). It is enabled by using the -archive flag, the import | ||
path is also needed as a non-flag argument. No other flags are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can archive mode optionally take a list of interfaces, just like reflect mode? Mocking extra interfaces may introduce extra dependencies to the mock code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be, but it also means this mode will derive from the source mode. We will need to manually match the specified interfaces to the resolved types. IMHO, the generated code will not introduce dependencies that are not already transitively depended on (in theory), so it should be just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saying we have:
package foo
type SFoo struct {
}
type IBar Interface {
Method1() string
}
type IBaz interface {
Method2() SFoo
}
When we only mock IBar
, then the mock won't depend on package foo
, so it can be compiled into a different package foomock
and imported by the tests in package foo
. However, when we mock both IBar
and IBaz
, if the mock is still compiled into foomock
, then the mock would have to import package foo
because a method in IBaz
returns foo.SFoo
. Now the tests in foo
cannot import foomock
, because of circular dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in this case, I'll just put the generated mocks in foo, say mock_foo_test.go.
I'm never a big fan of exporting mocks for unit tests in external packages. The obvious reason is that you are actually testing against an imaginary counterpart, a passing test doesn't reflect the actual usage of the package being mocked.
If you need to provide something for external packages to test against, create a fake sharing the same logic.
This also follows https://github.com/golang/go/wiki/CodeReviewComments#interfaces.
Done. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @codyoss Can you merge?
This CL adds archive mode, which takes advantage of go/gcexportdata
and go/types packages to extract typing info from a package archive file.
Given the way go archive is designed, we don't need to worry about transitive
dependencies, as it already includes all transitive typing info. And, we don't
need to build or run anything, so even if cgo is involved, it will still work.
Fixes #424