go generate ./... is broken #7976

Activity

1. williammartin

   added

   tech-debtA chore that addresses technical debtA chore that addresses technical debt

   coreThis issue is not accepting PRs from outside contributorsThis issue is not accepting PRs from outside contributors

   on Sep 11, 2023
2. williammartin commented on May 7, 2024

   

   williammartin

   on May 7, 2024

   Last edited by williammartin

   MemberAuthor

   More actions

   Why is this happening?

   Well the problem is to do with dependencies and packages. Let's take a look at the Config interface which lives in the config package:

   cli/internal/config/config.go

   Lines 32 to 50 in 4896546

   	//go:generate moq -rm -out config_mock.go . Config
   	type Config interface {
   	GetOrDefault(string, string) (string, error)
   	Set(string, string, string)
   	Write() error
   	Migrate(Migration) error
   	CacheDir() string
   	Aliases() *AliasConfig
   	Authentication() *AuthConfig
   	Browser(string) string
   	Editor(string) string
   	GitProtocol(string) string
   	HTTPUnixSocket(string) string
   	Pager(string) string
   	Prompt(string) string
   	Version() string
   	}

   This Config interface is defined in the config package. The go generate comment also says to generate the ConfigMock struct in the same package. Finally, there is a stub config used in tests that also exists in the same package that references the concrete ConfigMock.

   With the -rm flag provided to moq via go generate, the config_mock.go file is deleted, then the package is loaded in order to generate the code from the interface. However, because stub.go references ConfigMock and that has been removed, the package is in a broken state.

   However, that's not all because changing the interface and regenerating exhibits the same problem. For example, changing a return type results in the ConfigMock and therefore the Stub from no longer satisfying the interface. Thus, loading the package to regenerate the interface also results in a broken state.

   Typically this isn't a problem because test specific code lives in _test packages but for whatever reason the CLI has mostly been built with tests living inside the implementation packages.
3. williammartin commented on May 7, 2024

   

   williammartin

   on May 7, 2024

   MemberAuthor

   More actions

   What do we do about this?

   At it's core the issue is that our dependency graph is all messed up. The usual thing to do here is to have the interface in a consumer package, put the mock into its own package mocks or foo_test and then to move the stubs as well so that the import graph makes sense. However we can't move the stub easily because it is reaching into the unexported config.cfg struct.

   I think the right thing to do is to then to move the interface to a shared location e.g. a gh package, then generate the mock in a nearby package. In this way the stub can stay in config and have access to the unexported struct.

   The other advantage to this is that it now starts to form a what I would call a "domain package", a place where you can look to see the shapes of the puzzle pieces that are required for the gh app to work.
4. williammartin

   closed this as completedin #9060on May 10, 2024
5. andyfeller

   assigned

   williammartin

   on May 13, 2024