Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

support mocking vendored dependencies #28

Merged
merged 2 commits into from
Feb 11, 2018
Merged

support mocking vendored dependencies #28

merged 2 commits into from
Feb 11, 2018

Conversation

rgarcia
Copy link
Contributor

@rgarcia rgarcia commented Mar 14, 2016

This fixes #4

The files I used to manually test this are here: https://gist.github.com/rgarcia/cf553169c4832543a0a9

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@rgarcia
Copy link
Contributor Author

rgarcia commented Mar 14, 2016

I signed it! Company name: Clever.

@googlebot
Copy link

CLAs look good, thanks!

@ralfthewise
Copy link

If a destination is specified, it might be nice to create the temporary program in the directory tree of the destination so that it can also mock internal interfaces. See #29

@nizsheanez
Copy link

do we need to update something in this PR for approve it?

@@ -41,8 +41,13 @@ func Reflect(importPath string, symbols []string) (*model.Package, error) {

progPath := *execOnly
if *execOnly == "" {
// Put program in working directory to pick up packages in vendor/ during build
wDir, err := os.Getwd()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a non-starter for anyone using this where the source files might be read-only. We might need to resort to this only when needed (e.g. if there's vendoring involved).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsymonds re-reading the vendor spec I'm not sure there's an easy way to do that...the vendor directory could exist on a completely different subtree in the filesystem, so it's not as simple as looking for the existence of a vendor directory in the working directory.

dpb587-pivotal added a commit to cloudfoundry-attic/bosh-init that referenced this pull request May 6, 2016
Caused by golang/mock#28

Signed-off-by: Chris Brown <cbrown@pivotal.io>
@jmhodges
Copy link
Contributor

What's the next steps for this PR? Just ran into this again.

Looks like a new commit went up but can't tell immediately from the GitHub UI if that happened before or after the feedback.

mdelillo added a commit to vmware-archive/pcfdev-cli that referenced this pull request Aug 18, 2016
* Switch to fork that supports generating mocks for vendored dependencies (golang/mock#28)
* Install vendored mockgen before generating mocks
* Do not try to find files when no directories are found
@btomasini
Copy link

Any update on this?

@sugarjig
Copy link

Any updates? It looks like this also blocks the ability to vendorize mockgen. See #95.

@Soulou
Copy link

Soulou commented Nov 15, 2017

Any upgrade on this, using the following is not super healthy for my brain :D

mockgen $(PROJECT_ROOT)/<file path> <interface(s)> $@ \
&& sed -i '' s,$(PROJECT_VENDOR)/,, $@ \
&& goimports -w $<

@tyranron
Copy link

+1
Any updates?

@jsha
Copy link

jsha commented Dec 20, 2017

@rgarcia, looks like there are conflicts in mockgen/reflect.go. Would you be willing to bring this up to date with master and address any unaddressed review feedback? Thanks!

@mandazi
Copy link

mandazi commented Feb 1, 2018

Any update on this? I am also facing an issue that this PR most likely fixes.

@mandazi
Copy link

mandazi commented Feb 1, 2018

I don't believe @rgarcia is available since 2016 to work on this. Is it possible for someone else with write access to merge this with the latest master, resolve any conflicts and merge it. Reading the comments it appears many people have had this issue and this PR fixes it.

@rgarcia
Copy link
Contributor Author

rgarcia commented Feb 1, 2018

@mandazi another PR was merged that supports mocking vendored dependencies: #99

@rgarcia rgarcia closed this Feb 1, 2018
@mandazi
Copy link

mandazi commented Feb 1, 2018

Thanks @rgarcia! Didn't realize there was another PR for this.

@mandazi
Copy link

mandazi commented Feb 1, 2018

@rgarcia I am really sorry to bother you about this, but the PR (#99) you referenced did not resolve this issue. I just tested this PR (#28) and it did resolve my issue with the incorrect path to the vendor.

I humbly request that if it is possible to reopen this PR, review it and merge it.

Thank you.

This PR fixes this issue: #30

@kaedys
Copy link

kaedys commented Feb 1, 2018

#99 addresses mocking interfaces within vendored libraries. This PR addresses dependencies of mocks that are vendored, and the fact that the resulting mock code contains the illegal /vendor/ path portions in the import lines. The test added by #99 simply ensures that an interface located within a vendored library can be mocked. If you implement the following test instead, it fails:

/mockgen/tests/vendor_dep/doc.go:

package vendor_dep

//go:generate mockgen github.com/golang/mock/mockgen/tests/vendor_dep VendorsDep

/mockgen/tests/vendor_dep/vendor_dep.go:

package vendor_dep

import "a"

type VendorsDep interface {
	Foo() a.Ifc
}

/mockgen/tests/vendor_dep/vendor/a/a.go:

package a

type Ifc interface {
	A(string) string
	B(int) int
	C(chan int) chan int
	D(interface{})
	E(map[string]interface{})
	F([]float64)
}

Note that we're not mocking the vendored interface, we're mocking an interface that depends on a vendored type. The mockgen output has the following header:

[15:26:56] ~/go/src/github.com/golang/mock/mockgen/tests/vendor_dep $ go generate .
// Code generated by MockGen. DO NOT EDIT.
// Source: github.com/golang/mock/mockgen/tests/vendor_dep (interfaces: VendorsDep)

// Package mock_vendor_dep is a generated GoMock package.
package mock_vendor_dep

import (
	gomock "github.com/golang/mock/gomock"
	a "github.com/golang/mock/mockgen/tests/vendor_dep/vendor/a"
	reflect "reflect"
)

The a import line is illegal and should read simply a "a". This is with latest master, and thus includes the code from #99. This PR fixes an entirely separate issue than #99 does.

@mandazi
Copy link

mandazi commented Feb 1, 2018

Thank you @kaedys for the explanation between the two PRs.

I am not sure what the procedure is now that this PR is closed.

@rgarcia
Copy link
Contributor Author

rgarcia commented Feb 1, 2018

Sorry for the confusion, I can reopen.

@rgarcia rgarcia reopened this Feb 1, 2018
@mandazi
Copy link

mandazi commented Feb 1, 2018

@rgarcia Is it okay if I merge the latest master into this PR so it's up to date. I did it locally and there were no conflicts. Then we can see if it can be merged.

- Generate temporary program in current directory tree to pick up vendored dependencies during build
- Clean up the return of `PkgPath` according to the advice of rsc here golang/go#12019 (comment)
@rgarcia
Copy link
Contributor Author

rgarcia commented Feb 1, 2018

I went ahead and rebased

@mandazi
Copy link

mandazi commented Feb 1, 2018

@balshetzer Is it possible for you to take a look at this PR and see if it can be merged. This fixes this issue #30
I tested it for my projects and it does resolve the issue. Thank you.

@binodluitel
Copy link

I can confirm the fix works too

@mandazi
Copy link

mandazi commented Feb 2, 2018

@dsymonds Sorry to bother you. I was wondering if you could review this PR and merge it. It fixes #30 and also possibly #95 Much appreciated!

@balshetzer
Copy link
Collaborator

Thanks for bringing this PR up-to-date. I'll take a look shortly.

@rgarcia
Copy link
Contributor Author

rgarcia commented Feb 2, 2018

FWIW we get around this issue by using the -source flag and pointing to a file in vendor/. E.g.

mockgen -package mocks -source ./vendor/github.com/aws/aws-sdk-go/service/dynamodb/dynamodbiface/interface.go > mocks/dynamodb.go

@mandazi
Copy link

mandazi commented Feb 3, 2018

@rgarcia Interesting. Thanks! I am hoping this PR gets merged so I don’t have to manually update the generated mocks or do workarounds.

@balshetzer - Any update on the PR review and merge?

jpignata added a commit to awslabs/fargatecli that referenced this pull request Feb 5, 2018
Make setup and predictable builds easier for contributors by locking to
a set of dependencies. Some of the dependencies have been locked to
specific commits on master in order to accomodate projects that do not
ship releases often such as cobra and gomock (see
spf13/cobra#259 for details).

- Change mockgen commands to use a -source flag to explicitly point to
  the files in vendor for generating mocks. This, for some reason, fixes
  a bug in gomock whereby it uses the vendor path as the import path
  which breaks some dependencies.

  See golang/mock#28 for more details.
@kaedys
Copy link

kaedys commented Feb 5, 2018

FWIW we get around this issue by using the -source flag and pointing to a file in vendor/.

Doesn't the -source flag turn off the Reflection system and use simply file parsing instead? That has issues when the interface in question references types in the same package but different files, since those type names will be unqualified. Also, isn't that just addressing the same issue that #99 addressed, mocking a vendored interface, rather than mocking an interface with a vendored dependency?

@mandazi
Copy link

mandazi commented Feb 5, 2018

@balshetzer - Any update on this being merged? Thanks!

@balshetzer
Copy link
Collaborator

Would it be possible for you to add a test case to verify that this fixes the problem?

boxtown added a commit to boxtown/mock that referenced this pull request Feb 7, 2018
@mandazi
Copy link

mandazi commented Feb 7, 2018

@rgarcia could we use the test @kaedys mentioned in this comment: #28 (comment) to fulfill @balshetzer 's request?

@rgarcia
Copy link
Contributor Author

rgarcia commented Feb 7, 2018

added a test in 77f22c2

@mandazi
Copy link

mandazi commented Feb 9, 2018

@balshetzer A test was added by @rgarcia - Can this PR be reviewed and merged? Thanks!

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

Successfully merging this pull request may close these issues.

mocking dependencies located in a vendor directory