Import cycle when using the fake in the same package as the interface

[nkovacs]

This causes an import cycle:

package counterfeit

//go:generate counterfeiter . Fooer

type Fooer interface {
	Foo() bool
}

func Bar(f Fooer) string {
	if f.Foo() {
		return "foo"
	} else {
		return "bar"
	}
}

package counterfeit

import (
	"temp/counterfeit/counterfeitfakes"
	"testing"
)

func TestBar(t *testing.T) {
	fake := &counterfeitfakes.FakeFooer{}

	Bar(fake)
}

This is caused by the interface check at the bottom of fake_fooer.go:

var _ counterfeit.Fooer = new(FakeFooer)

One solution is to change the test file's package to counterfeit_tests, but that doesn't work if I want to test unexported functions.

#8 is similar, but that one is solvable by renaming the import. This isn't.

Is this check really necessary though? If you try to use the fake in a test, and it no longer satisfies the interface, it'll fail to compile anyway (well, except if you're doing something tricky with interface{}).
Could you maybe add an option to disable this check?

[tjarratt]

Sorry this caused you some pain @nkovacs. Unfortunately, I'm not really amenable to changing it. For one, I feel this adds some clarity to the generated code that it actually does implement the desired interface - if you changed the interface, but did not change your generated code, you would see some compile errors in your tests - which is not at all where the problem is. Leaving this assignment here gives you a single compile error in the implementation that is closer to the source of the problem (namely, the fake doesn't implement the interface).

In general, I am strongly against the idea of using a generated fake within the same package. That seems to me to be opposed to the philosophy of go, which is to have separate packages for separate concepts. With interfaces, I can't imagine a strong compelling reason to have the definitions for two interfaces arising from the same package, but wanting to mock them between their tests. Perhaps that's just your tests providing feedback on your implementation that those should be separate. Or perhaps they shouldn't be mocked that way.

If you really want to do this, I suggest you write a small script that removes the assignment at the end of the file. Or you could fork counterfeiter.

[nkovacs]

Here's an example of where I ran into this: https://github.com/qiangxue/golang-restful-starter-kit/blob/master/services/artist.go#L9 and https://github.com/qiangxue/golang-restful-starter-kit/blob/master/services/artist_test.go#L106

[tjarratt]

Sorry @nkovacs, I don't really follow. Is the precise problem here that the artist unit test cannot use a counterfeiter double?

The bigger issue there, as I see it, is that the unit test is in the services package, instead of in the services_test package. Granted, there's not always a lot of alignment in the Go community here about when to have unit tests in the same package, and when to use the _test package, but as a maintainer of Counterfeiter, I really want to take a hard line here.

Go, as a language and tool, does not ensure that one writes good tests. Nor does it ensure that one's tests are testing the right things. One nice thing about open source projects is that they can make decisions that help guide users to do the right thing. In my opinion, it's correct to have tests in the _test package 99% of the time to ensure that you are testing your public interface and that you observe behavior and side effects, not implementation details. Hence counterfeiter should support that testing pattern primarily.

For the other 1% of cases, they should be so exceptional, that it shouldn't be quite so easy to do, in order to encourage users to test their interfaces "properly".

Granted, this is subjective, and not an absolute truth.

[nkovacs]

Yes, the problem is that I cannot generate a fake for the artistDAO interface for use in the tests of the services package.

And yes, using a _test package works in this case, but I don't agree that having tests in the same package (i.e. white-box testing) is inherently a bad thing.

Actually I'd prefer having the fakes generated into the same package (that would also fix #8), like in the original, hand-crafted code. These fakes are really only useful for the tests that use them, so I don't see why they need to be in a separate package. In a dynamic language, you'd be creating those fakes in the tests, at runtime. You wouldn't have them as a separate class, ready to import in all the tests that need them.

In this comment you brought up net/http and net/http/httptest as an example, but I don't think that's a really good example. net/http/httptest is useful for some special cases (testing http handlers and http servers), but if you look at the http package, its tests also have hand-crafted fakes, e.g.: https://github.com/golang/go/blob/master/src/net/http/fs_test.go#L620

[nkovacs]

What if I want to test unexported stuff?

Anyway, I solved this by making it possible to generate the fake into the same package, since that's actually what I need.

[ianaz]

Hi @nkovacs , I have the same issue. Are you still using your fork?

[benmoss]

If you can make the interface private it won't add the interface assertion to the bottom of your fake

[subbuqq]

i don't see the -pkg flag in the main build. is the fix not merged ?