Mocking conflict with internal packages.

[SimonRichardson]

When attempting to use gomock or similar mocking libraries with, the mocking libraries don't interpret type aliases correctly, which causes errors when using them for mocks. In essence, types that alias to an internal package become unmockable.

As a client library, it makes it hard to use.

Can we not consider putting the types (DialFunc, LogFunc, NodeInfo, NodeStore, NodeRole, etc) that are aliased in another package that isn't internal. Even if you want to keep the alias going, you could have the internal types alias to a package outside of internal.

[freeekanayaka]

Yes, I believe that would be fine. Care to submit a patch?

[SimonRichardson]

@freeekanayaka Wicked, will submit this week at some point

[stgraber]

@SimonRichardson hey, it's been a week and a bit over a year ;) Is that still something you intend to work on?

[SimonRichardson]

I will at some point, just trying to find time. If anybody else wants to do this before me, then be my guest.

[vianaddsv]

@stgraber and @freeekanayaka i'm interesting to contribute to resolve this task. I can do one suggestion of code ?

[stgraber]

Sure, we'd love a PR for this.

[vianaddsv]

@stgraber @freeekanayaka I created a branch with sugestion (https://github.com/vianaddsv/go-dqlite/tree/isolate-internal-package) but only applied to LogFunc and if ok i'm will apply to others. I create a adapter function to create one anticorruption layer to isolate the internal package functions to the client layer. The packages client, driver, app(call to the client layer) only know about LogFunc ( not internal package) and when need using the internal package only call adapter function to converter to internal package. sorry for the delay to answer, what do you think ?

[freeekanayaka]

I hoped the solution was more straightforward.

Just to take a step back, let me ask a couple of questions:

1. Why is mocking needed in the first place? If you want to unit-test your data-access-layer code (i.e. your SQL queries) you can easily factor your code to take a generic sql.DB object as parameter and use for example an in-memory SQLite driver for it (instead of go-dqlite). If you want to write higher-level unit tests involving also go-dqlite, it should be easy to just use go-dqlite for real (basically you only need a data-directory).

2. Isn't this alias problem something that mocking libraries can solve themselves?

I'm particularly interested in discussing 1., since it might turn out that it's actually a better alternative (incidentally that's the approach LXD takes for its unit tests).

[SimonRichardson]

Isn't this alias problem something that mocking libraries can solve themselves?

Unfortunately not, as the alias is not preserved in the AST, so there is no way to access it from a pkg level. That means that your code in the mocks will attempt to import something with an internal namespace. I guess you could run a post mock command to work around the dqlite code layout, but that's a hack and I wouldn't want to do that. At the time, I just shimmed dqlite library out to the edge and just translated the code where appropriate, even if this required double translating :disappointed:

I'm no longer working on Juju, so not my problem anymore :smile:

[vianaddsv]

1. Why is mocking needed in the first place? If you want to unit-test your data-access-layer code (i.e. your SQL queries) you can easily factor your code to take a generic sql.DB object as parameter and use for example an in-memory SQLite driver for it (instead of go-dqlite). If you want to write higher-level unit tests involving also go-dqlite, it should be easy to just use go-dqlite for real (basically you only need a data-directory).

I think that no problem only of mock , but a design of code. You never expose internal package to client layer and when create type alias to loggin.Func do you translate problem to other layers and impact to mock or in future when expose one struct or function important to client realize one unit test. So,with approach you even double translating.

Second sugestion do you move logging internal package out internal package and this can use by client and internal package, this is same approach in package net/http (https://github.com/golang/go/blob/master/src/net/http/internal/chunked.go) package internal contains HTTP internals shared by only net/http and net/http/httputil

What do you think about move logging and other functions out of internal package ?

[freeekanayaka]

1. Why is mocking needed in the first place? If you want to unit-test your data-access-layer code (i.e. your SQL queries) you can easily factor your code to take a generic sql.DB object as parameter and use for example an in-memory SQLite driver for it (instead of go-dqlite). If you want to write higher-level unit tests involving also go-dqlite, it should be easy to just use go-dqlite for real (basically you only need a data-directory).

I think that no problem only of mock , but a design of code. You never expose internal package to client layer and when create type alias to loggin.Func do you translate problem to other layers and impact to mock or in future when expose one struct or function important to client realize one unit test. So,with approach you even double translating.

Second sugestion do you move logging internal package out internal package and this can use by client and internal package, this is same approach in package net/http (https://github.com/golang/go/blob/master/src/net/http/internal/chunked.go) package internal contains HTTP internals shared by only net/http and net/http/httputil

What do you think about move logging and other functions out of internal package ?

Making the logging package public sounds ok. That would mean moving it from go-dqlite/internal/logging to go-dqlite/logging, keeping the type aliases declared in go-dqlite/logging for convenience and backward-compatibility.

About the other functions I'm not sure, we'd need to check on a case-by-case basis. Perhaps making a list of these function would be a good first step.

[vianaddsv]

1. Why is mocking needed in the first place? If you want to unit-test your data-access-layer code (i.e. your SQL queries) you can easily factor your code to take a generic sql.DB object as parameter and use for example an in-memory SQLite driver for it (instead of go-dqlite). If you want to write higher-level unit tests involving also go-dqlite, it should be easy to just use go-dqlite for real (basically you only need a data-directory).

I think that no problem only of mock , but a design of code. You never expose internal package to client layer and when create type alias to loggin.Func you translate problem to other layers and impact to mock or in future when expose one struct or function important to client perform one unit test. So,with approach you even double translating. Second suggestion do you move logging internal package out internal package and this can use by client and internal package, this is the same approach in package net/http (https://github.com/golang/go/blob/master/src /net/http/internal/chunked.go) package internal contains HTTP internals shared by only net/http and net/http/httputil What do you think about move logging and other functions out of internal package ?

Making the logging package public sounds ok. That would mean moving it from go-dqlite/internal/logging to go-dqlite/logging, keeping the type aliases declared in go-dqlite/logging for convenience and backward-compatibility.

I would like create a PR to move go-dqlite/internal/logging to go-dqlite/logging, can i create?

About the other functions I'm not sure, we'd need to check on a case-by-case basis. Perhaps making a list of these function would be a good first step.

List of external dependencies of internal package

package: protocol (go-dqlite/internal/protocol)

Functions: - protocol.DialFunc (using in go-dqlite/client/client.go) - protocol.NewInmemNodeStore (using in go-dqlite/client/store.go)

Interfaces:

• protocol.NodeStore (using in go-dqlite/client/store.go)

Structs:

• protocol.NodeInfo (using in go-dqlite/client/store.go)
• protocol.InmemNodeStore (using in go-dqlite/client/store.go)

Enumerations Types:

• protocol.NodeRole (using in go-dqlite/client/store.go)

Dependencies without problems:

package: driver (go-dqlite/driver)

• type Error = protocol.Error * Error implements error interface, so other functions or struct do use error native interface of go.

package: client (go-dqlite/client)

• const Voter, Standby and Spare * in file constants.go do you have const with reference internal package, but expose with primite int to the client.

Dependencies with questions: package: driver (go-dqlite/driver)

• var ErrNoAvailableLeader = protocol.ErrNoAvailableLeader - This is a public variable and expose out of package, but client should not use, suggestion this error is just called by internal function Open in App struct (app.go), but why you don't check error direct of internal package ?

if cause != protocol.ErrNoAvailableLeader {
  return nil, err
}

can we check this list , what do you think ?

[freeekanayaka]

1. Why is mocking needed in the first place? If you want to unit-test your data-access-layer code (i.e. your SQL queries) you can easily factor your code to take a generic sql.DB object as parameter and use for example an in-memory SQLite driver for it (instead of go-dqlite). If you want to write higher-level unit tests involving also go-dqlite, it should be easy to just use go-dqlite for real (basically you only need a data-directory).

I think that no problem only of mock , but a design of code. You never expose internal package to client layer and when create type alias to loggin.Func you translate problem to other layers and impact to mock or in future when expose one struct or function important to client perform one unit test. So,with approach you even double translating. Second suggestion do you move logging internal package out internal package and this can use by client and internal package, this is the same approach in package net/http (https://github.com/golang/go/blob/master/src /net/http/internal/chunked.go) package internal contains HTTP internals shared by only net/http and net/http/httputil What do you think about move logging and other functions out of internal package ?

Making the logging package public sounds ok. That would mean moving it from go-dqlite/internal/logging to go-dqlite/logging, keeping the type aliases declared in go-dqlite/logging for convenience and backward-compatibility.

I would like create a PR to move go-dqlite/internal/logging to go-dqlite/logging, can i create?

About the other functions I'm not sure, we'd need to check on a case-by-case basis. Perhaps making a list of these function would be a good first step.

List of external dependencies of internal package

package: protocol (go-dqlite/internal/protocol)

Functions: - protocol.DialFunc (using in go-dqlite/client/client.go) - protocol.NewInmemNodeStore (using in go-dqlite/client/store.go)

Interfaces:

• protocol.NodeStore (using in go-dqlite/client/store.go)

Structs:

• protocol.NodeInfo (using in go-dqlite/client/store.go)
• protocol.InmemNodeStore (using in go-dqlite/client/store.go)

Enumerations Types:

• protocol.NodeRole (using in go-dqlite/client/store.go)

Dependencies without problems:

package: driver (go-dqlite/driver)

• type Error = protocol.Error * Error implements error interface, so other functions or struct do use error native interface of go.

package: client (go-dqlite/client)

• const Voter, Standby and Spare * in file constants.go do you have const with reference internal package, but expose with primite int to the client.

Dependencies with questions: package: driver (go-dqlite/driver)

• var ErrNoAvailableLeader = protocol.ErrNoAvailableLeader - This is a public variable and expose out of package, but client should not use, suggestion this error is just called by internal function Open in App struct (app.go), but why you don't check error direct of internal package ?

if cause != protocol.ErrNoAvailableLeader {
  return nil, err
}

can we check this list , what do you think ?

Thanks for the detailed information. I trust that this list is mostly complete.

If you wish to work on this, I'd suggest to do it one step at a time with a series of Pull Requests. For example the first PR could move go-dqlite/internal/logging to go-dqlite/logging. The second PR could move just the necessary parts of go-dqlite/internal/protocol to go-dqlite/protocol. And so on.

Doing all this little by little makes it easier for us to review and for you to not go too far without feedback, with the risk of wasting time.

[vianaddsv]

@freeekanayaka i created a PR with part of solutions (only move logging package in this moment), i will create a second to other packages. I created a suggestion of code in another branch but it is a breaking change, because all client would need change import to go-dqlite/logging to me is sounds best solutions, but its a breaking change =/ ( https://github .com/vianaddsv/go-dqlite/tree/logging-out-of-internal-package-breaking-change)

[SimonRichardson]

@vianaddsv why not create an alias type in client package that points to the new logging package? That way you wouldn't need to change as much code?

[freeekanayaka]

@vianaddsv why not create an alias type in client package that points to the new logging package? That way you wouldn't need to change as much code?

I think that's effectively what is already in place in his branch. The alias types in the client package were aliasing the internal version of the package, and they are now aliasing the public one. I don't see a breakage in backward compatibility.