import cycle if i write a test for the faked interface in the package of the interface

[dionysius]

Related to #103, but with the difference, that i'm ok with having the fake in a sub-package (omitting -o flag)

package abc
//go:generate $GOPATH/src/github.com/maxbrunsfeld/counterfeiter/counterfeiter . Foo

// some struct which the implementer of the interface should return
type Stuff struct{}

type Foo interface {
	Bar() Stuff
}

I write a test file in the same package abc

package abc

import (
	"testing"
        "github.com/where/is/abc/abcfakes"
)

func Test_SomethingInteractingWithFoo(t *testing.T) {
  // fmt.Println("some stuff happens before")
  fakeFoo := &abcfakes.FakeFoo{}
  // fmt.Println("some stuff happens after")
}

once generated, abcfakes/fake_foo.go has these lines:

// Code generated by counterfeiter. DO NOT EDIT.
package abcfakes

import (
	"sync"

	"github.com/where/is/abc"
)

// ...

func (fake *FakeFoo) Bar() (abc.Stuff) {
	// and for any other method using a type from the package
	// ...
}

// ...

var _ abc.Foo = new(FakeFoo)

1. So, first problem is the same as in Interface fakes generated in the same package as the interface cause an import cycle #103. var _ abc.Foo = new(FakeFoo) forces the FakeFoo file to import github.com/where/is/abc which causes an import cycle, because coming from abc i'm importing github.com/where/is/abc/abcfakes.

2. Independent of the first problem, we have still another one: func (fake *FakeFoo) Bar() (abc.Stuff) { ... } forces the FakeFoo file to import github.com/where/is/abc too, which causes an import cycle for the same reason.

[dionysius]

1. Can be solved, if we could omit the last line. Do we need that?

var _ abc.Foo = new(FakeFoo)

2. Can only be solved, if the fake is generated in the same package. It will always be an import cycle otherwise - nothing counterfeiter could do, that's just how golang works. We already have the option -o to decide where the file goes, but it does not detect that it is landing in the same package and should write the fields differently:

// Code generated by counterfeiter. DO NOT EDIT.
- package abcfakes
+ package abc

import (
	"sync"
- 
- 	"github.com/where/is/abc"
)

// ...

- func (fake *FakeFoo) Bar() (abc.Stuff) {
+ func (fake *FakeFoo) Bar() (Stuff) {
	// and for any other method using a type from the package
	// ...
}

// ...

- var _ abc.Foo = new(FakeFoo)
+ // nothing
+ // or
+ var _ Foo = new(FakeFoo)

[alexkohler]

I would say we definitely should keep the last line, as it guarantees that FakeFoo is still meeting the abc.Foo interface, and will generate a compile-time error otherwise. Ideally, if we can detect where the package is going, we can determine whether or not to include the package qualifier.

[joefitzgerald]

I see four possible solutions (not mutually exclusive) to this problem

1. Put your test in the abc_test package; this allows you to import both the abc and the abcfakes package without an import cycle
2. Add a flag that omits the line that guarantees the fake satisfies the interface (var _ abc.Foo = new(FakeFoo))
3. Implement a fix for Interface fakes generated in the same package as the interface cause an import cycle #103 and allow you to generate the fake into the abc package
4. Implement Support external fake templates #98 and allow you to specify a different template to be used for fake generation

[LasTshaMAN]

Among possible solutions listed above only 1) or 3) are reasonable (in my opinion), because 2) and 4) propose to introduce a great deal of complexity in counterfeiter package itself (which is also against Go philosophy on being simple and having single opinionated approach to do things).

[dionysius]

I overall agree with all the points of @LasTshaMAN. Just my 2 cents:

1. Is in my opinion not reasonable enough. If you choose to put the tests into a separate package abc_test you can only test the exported fields of package abc. So this depends on the wished testing workflow. I have to look up what golang devs have expected us where to put which test in, but this is what I know so far.
2. a) that line itself is not the issue, as outlined in my OP: If you have an exported field (like abc.Stuff in my example), then you still have an issue with that. b) With this line, you guarantee on compile time that, without any implementation, that the interface is met. However, if you would leave it out, you would anyway get a compile time guarantee, once you really wrote a line of code or test correctly using and expecting the abc.Foo interface. c) third thought, why don't we put that line in a respective test file fake_foo_test.go? As I said already, this line is not the only issue, personally I found that line useful and I have put that into my test file for other own interface implementations.

I only see 3. as the best option.

Edit, addition to 2.: Imagine writing/generating a protobuf interface (e.g. like api.pb.go from api.proto).
You could end up with structs ...Request, ...Response for each of the Methods:
func (x *X) Method (ctx ..., req *MethodRequest, opts ...) (*MethodResponse, error) - depends on how you wrote your .proto file

[ryanrolds]

Been hitting this issue when trying to test private methods that need a fake. I'm just some random guy, but option 3 seems the most reasonable. I don't believe a fake needs to confirm that it satisfies the interface, using the fake in a test will do that.

[oleksmir]

Note, this issue is not presented if you do not export your interface. For instance,

type foo interface {
    Bar() string
}

[imsodin]

This project is absolute great. It allows to get rid of soooo much boilerplate, that I needed to update too frequently. And opens easy access to more detailed testing - thanks a lot for creating it!

To use a mocked interface within the same package (I need access to internals), I use the admittedly very hacky script below and go:generate like this:
//go:generate go run ../../script/prune_mocks.go -t mocked_test.go

// +build ignore

package main

import (
	"bytes"
	"flag"
	"log"
	"os"
	"os/exec"
	"path/filepath"
)

func main() {
	var path string
	flag.StringVar(&path, "t", "", "Name of file to prune")
	flag.Parse()

	filepath.Walk(path, func(path string, info os.FileInfo, err error) error {
		if err != nil {
			log.Fatal(err)
		}
		if !info.Mode().IsRegular() {
			return nil
		}
		err = pruneInterfaceCheck(path, info.Size())
		if err != nil {
			log.Fatal(err)
		}
		err = exec.Command("goimports", "-w", path).Run()
		if err != nil {
			log.Fatal(err)
		}
		return nil
	})
}
func pruneInterfaceCheck(path string, size int64) error {
	fd, err := os.OpenFile(path, os.O_RDWR, 0666)
	if err != nil {
		return err
	}
	defer fd.Close()

	var chunk int64 = 100
	buf := make([]byte, chunk)
	searched := []byte("var _ ")
	pos := size - chunk
	for {
		_, err = fd.ReadAt(buf, pos)
		if err != nil {
			return err
		}
		if i := bytes.LastIndex(buf, searched); i != -1 {
			pos += int64(i)
			break
		}
		pos -= chunk
	}
	return fd.Truncate(pos)
}

I understand that you are opposed to doing it, however I'd rather not do one package for every interface - the project would explode.
Would you consider accepting a contribution that adds a flag that does not add the interface check, e.g. -no-intf-check?
I'd rather do that than keep my hack :)