search

matryer / moq

Interface mocking tool for go generate
http://bit.ly/meetmoq
MIT License
1.96k stars 126 forks source link

Internal registry for disambiguated imports, vars #141

Closed sudo-suhas closed 3 years ago

sudo-suhas commented 3 years ago 
- Move functionality in the moq package partially into 
  internal/{registry,template}.
- Leverage registry to assign unique package and variable/method
  parameter names. Use import aliases if present in interface source
  package.

BREAKING CHANGE: When the interface definition does not mention the 
parameter names, the field names in call info anonymous struct will be
different. 

The new field names are generated using the type info (string -> s, 
int -> n, chan int -> intCh, []MyType -> myTypes, map[string]int ->
stringToInt etc.).

For example, for a string parameter previously if the field name was 
'In1', the new field could be 'S' or 'S1' (depends on number of
string method parameters).

Closes #68, #94 Replaces and closes #63, #120, #121, #127

TODO:

@matryer this is a significant refactor and would be very good to get a review.

edit: Presentation deck - https://docs.google.com/presentation/d/1-HVOygCVpQQ_8ikE2_OYzlvuH1NYI3wT02PWn0Onpcc/edit?usp=sharing, which

breml commented 3 years ago

@sudo-suhas We have the situation described in #94. I did a test with this PR and I can confirm, that it worked.

Regarding the breaking change, could you provide an example, that illustrates when and how the breaking change could potentially affect existing (test) code.

sudo-suhas commented 3 years ago

@breml an example of the breaking change is present in one of the 'golden' files generated via tests - https://github.com/matryer/moq/pull/141/files#diff-f8cb62335f3c482f132a362ff53cecc339a89fabfdc2ed25539b283308509678R36-R39

matryer commented 3 years ago

@breml What do you think about this? I'm not familiar with the key differences between this PR and #94.

sudo-suhas commented 3 years ago

@matryer this PR fixes the issue reported in #94. There are other PRs which also attempt to do the same. The difference would be that this introduces some abstraction and splits the Mocker into more manageable pieces rather than trying to fit the disambiguation logic for imports and variables into the existing type.

breml commented 3 years ago

@matryer I spent some time to have a more in depth look at this PR.

TLDR: The result seems good, at least it worked for me. The PR as a whole is really difficult to review.

I have the following thoughts about this PR:

With all that said, to me the question is, are we willing to merge this PR to fix the before mentioned issues while taking the risk, that other things might be broken, due to the difficulty to properly review this PR and due to the missing tests/coverage for certain areas of the code.

sudo-suhas commented 3 years ago

@breml Thank you for taking the time to review the changes. I appreciate it very much. I do have some clarifications though.

I would like to have the tests and the changes to moq in a separate commit, such that I as a reviewer can easily follow the changes made and reason about the effects of the change... moving tests to a new format (explicit tests to golden files)_and so on.

The golden file tests were added some time back. While I was adding new tests, I moved most of the existing golden file tests into a single table-driven one. I have added a commit which refactors the existing tests in a separate commit.

This PR combines multiple things like refactoring the package/directory structure (introduction of internal package), adding the registry, introduce templates, introduce breaking changes to the generated mocks ...

I agree that the PR does a lot of changes. That's partly because I was (and am) not sure how to split some of the changes. For example, I am not sure how I can split the internal/registry+internal/template refactor because the template has to reference the registry types to reflect the changes made while resolving conflicts. While it is no way a justification, I think you would be able to understand that there was also a limit on the time I could dedicate for doing the changes.

A good example for this is pkg/moq/moq.go. There are so many changes to this single file, I do not have the time to really go through and understand each change.

With the introduction of internal/registry, I don't think the changes in pkg/moq/moq.go could have been avoided. That's not to downplay the difficulty of reviewing the changes. Instead, perhaps, it would be better to view only the resulting file. I do believe that the file now is a lot simpler to read since a lot of the heavy lifting is moved to the registry.

I am not really sure about the decision to use the type information for the generated variable names. To prefix variable names with a type-dependent prefix does look like code smell to me, especially in a statically typed language. I think there are other ways to address the name clash issue.

The generated names for method parameters is used only if there wasn't one defined in the interface definition (or was _) to begin with(see interface, moq). The type names are not prefixed to variable names otherwise. And with regards to the generated names when none were present, I would argue that they are still better than in1/in2/in3.... And name clashes with imported packages are resolved by appending MoqParam. Name clash with variables is resolved by a serial number suffix (myName1,myName2 etc.).

The new packages internal/registry and internal/template do not really have coverage by unit tests, so this makes it even more difficult to get a good feeling about the changes introduced by this PR.

This is mostly because I am not sure how I can test these. Consider registry.Var - how do I generate an instance of types.Var with the appropriate type, package etc.? I am not sure. If I were to try to use the testpackages for this, that would not be very different from the current tests in pkg/moq.

All that said, I am still open to listing the specific steps we could take to make the PR easier to review and also improve the confidence for merging in the changes. But I would need your help to figure out what those steps need to be.

breml commented 3 years ago

@sudo-suhas Thanks for your reply. As mentioned already in my first review comment, I think this PR brings a lot of improvement. I consider myself a regular user of moq and I contributed some smaller PR in the past. But I am not familiar with the whole code base of moq in depth. @matryer invited me to share my thoughts and this is what I did. No offence intended.

I spent again some time on this PR and I have the following additional input for you:

Generate an instance of types.Var:

This can be as follows:

import "go/types"

var x = types.NewVar(token.NoPos, nil, "variablename", types.Universe.Lookup("int").Type())

sync package edge case

For the following interface, the generation of the mock failed:

package syncimport

import (
    stdsync "sync"

    sync "github.com/matryer/moq/pkg/moq/testpackages/syncimport/sync"
)

type Syncer interface {
    Blah(s sync.Thing, wg *stdsync.WaitGroup)
}

Where package github.com/matryer/moq/pkg/moq/testpackages/syncimport/sync is:

package sync

type Thing string
sudo-suhas commented 3 years ago

Maybe I came across as defensive 🙈 but I was not offended in the least. I will find some time to incorporate the helpful input you have provided.

sudo-suhas commented 3 years ago

I made some improvements to the tests to further improve coverage:

image

Hope this is sufficiently high coverage for the internal/{registry, template} packages.

edit: Not sure if it was understood, the sync package edge case has been covered as well.

matryer commented 3 years ago

@sudo-suhas Would you be up for a quick 15 minute screenshare some time to go through this? It looks like you've done a lot of work here, and I want to make sure I understand the new design, and your thinking behind it.

sudo-suhas commented 3 years ago

Sure @matryer, can do. Should we chalk out the time and tool via Twitter DMs?

matryer commented 3 years ago

Yes, good idea. I’m @matryer on Twitter.

On 5 Jan 2021, at 10:43, Suhas Karanth notifications@github.com wrote:

Sure @matryer, can do. Should we chalk out the time and tool via Twitter DMs?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

breml commented 3 years ago

@matryer, @sudo-suhas Did it workout with your direct chat about this PR? What are the conclusions?

sudo-suhas commented 3 years ago

No, not yet. We will try to sort it out soon.

sudo-suhas commented 3 years ago

@matryer I have created a deck which

Please check it out - https://docs.google.com/presentation/d/1-HVOygCVpQQ_8ikE2_OYzlvuH1NYI3wT02PWn0Onpcc/edit?usp=sharing.

matryer commented 3 years ago

This is great work. And the overview by @sudo-suhas is outstanding. Thank you.

sudo-suhas commented 3 years ago

Thank you @matryer for your kind words 🙏🏼

With regards to the tagged release v0.1.6, since this was a breaking change, it should have been v0.2.0, CMIIW.

matryer commented 3 years ago

@sudo-suhas ahh yeah - that's a good point. Do you think it's worth removing the release and retagging? Or has that ship sailed now?

Technically, moq hasn't yet reached v1, but the last thing we want is to break things for those using this tool.

sudo-suhas commented 3 years ago

The correct way to handle it would be to release v0.1.7 which would be equivalent to v0.1.4 (what happened to v0.1.5? 🙈) and then release v0.2.0 which would be equivalent to v0.1.6.

matryer commented 3 years ago

What's the advantage of bumping the minor release? Is this respected by tools, or just a better signal to moq users?

matryer commented 3 years ago

v0.1.5 got borked and I didn't want to risk having a bad release. 😬

sudo-suhas commented 3 years ago

To the best of my understanding, for v0 semantic versioning, bumping the minor version indicates a breaking change while incrementing the patch version is expected to add new backwards-compatible features or bug fixes.

matryer commented 3 years ago

Oh right, in modules you mean? Yeah ok, it's worth doing then.

I'm away from my computer for a bit, do you have the right permissions to do a release?

sudo-suhas commented 3 years ago

Have created both releases as discussed.

Conclusion: Use moq v0.2.0 to get the changes made in this PR.

matryer commented 3 years ago

Thanks Suhas. 🎉

On Tue, 2 Feb 2021 at 07:30, Suhas Karanth notifications@github.com wrote:

Have created both releases as discussed.

Conclusion: Use moq v0.2.0 https://github.com/matryer/moq/releases/tag/v0.2.0 to get the changes made in this PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/matryer/moq/pull/141#issuecomment-771430390, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAY2G3DMYUTDVC5DZXK3VTS46SZ7ANCNFSM4UIOVAMQ .

breml commented 3 years ago

Thanks @sudo-suhas and @matryer for the great work, this is very much appreciated.