search

matryer / moq

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

[WIP] allow imported constraints for generics #198

Closed cbaker closed 1 year ago

cbaker commented 1 year ago

I'm not the most knowledgeable on the STDLIB given it's pretty big but this seems to work relatively well. 🤷‍♂️ If there is a way to accomplish the helper functions more optimally, I'd love to know. Thanks!

Example of the problem:

package myotherpackage

import (
 "os/fs"
)

type T interface {
  *fs.FileInfo
}
package mypackage

import (
  "github.com/matryer/moq/myotherpackage"
)

type MyInterface[T myotherpackage.T] interface {
  M(T) error
}

If we try to put this into mock as it stands it will attempt to generate something like:

var _ MyInterfaceMock[*os/fs.FileInfo] = .....

This is a problem and causes moq to fail in 2 ways: 1) Mocks will not generate because gofmt fails. 2) We will generate a bad mock if gofmt were to not fail.

The solution I have presented will add "os/fs" to the imports on the mock and generate:

var _ MyInterfaceMock[*fs.FileInfo] = ....
cbaker commented 1 year ago

Believe this will address: https://github.com/matryer/moq/issues/177 https://github.com/matryer/moq/pull/190

Maybe: https://github.com/matryer/moq/issues/173 ? Seems kinda "generic" ha

TheFellow commented 1 year ago

@cbaker (and whoever else comes across this) I just built from your fork and it does not appear to fix #177 sadly. I brought over the failing test from PR #190, fixed up the golden file and confirmed it does not generate the imported generic's package when generating the code.

I built from your branch and sanity checked on my production use case and sadly confirmed it there as well. Haven't dug into it yet to try and fix it but I really need to understand it so I can try and get it sorted out. It's causing a headache and ugly workarounds that I want to stop from growing.

cbaker commented 1 year ago

@cbaker (and whoever else comes across this) I just built from your fork and it does not appear to fix #177 sadly. I brought over the failing test from PR #190, fixed up the golden file and confirmed it does not generate the imported generic's package when generating the code.

I built from your branch and sanity checked on my production use case and sadly confirmed it there as well. Haven't dug into it yet to try and fix it but I really need to understand it so I can try and get it sorted out. It's causing a headache and ugly workarounds that I want to stop from growing.

Did you use the branch on my fork for this PR or main? This isn't merged into main on my fork yet. Have you checked out the test cases I wrote in this PR to see if this is relevant to what you're looking to fix? I've tested it on some code at my work that was previously failing and it successfully generated a mock. Send me a failing test case and I will assist in looking into it. Thanks!

TheFellow commented 1 year ago

@cbaker That'd be amazing!

I haven't looked through your tests yet, but I just forked your repo and added the test case I tried to your branch. Added a PR back to your branch here.

cbaker commented 1 year ago

@cbaker That'd be amazing!

I haven't looked through your tests yet, but I just forked your repo and added the test case I tried to your branch. Added a PR back to your branch here.

I think I see the problem. You're using a struct instead of an interface, hopefully simple to remedy, I'll check it out later tomorrow. Thanks.

cbaker commented 1 year ago

@TheFellow I have replicated the issue and began working a solution, just have not had a crazy amount of time recently.

TimVosch commented 1 year ago

Believe this will address: #177 #190

Maybe: #173 ? Seems kinda "generic" ha

I just opened PR #199 which fixes #177, might be relevant here.

cbaker commented 1 year ago

Believe this will address: #177 #190 Maybe: #173 ? Seems kinda "generic" ha

I just opened PR #199 which fixes #177, might be relevant here.

I believe what you did will more than likely fix it for everyone in a way more eloquent manner. Great job. Closing this PR.

TimVosch commented 1 year ago

Thanks,

But I am not sure if this covers all issues mentioned in this thread.

I am off work now, so I can check tomorrow.

Cheers,

On Tue, Jun 20, 2023, 17:14 Chad @.***> wrote:

Believe this will address: #177 https://github.com/matryer/moq/issues/177 #190 https://github.com/matryer/moq/pull/190 Maybe: #173 https://github.com/matryer/moq/issues/173 ? Seems kinda "generic" ha

I just opened PR #199 https://github.com/matryer/moq/pull/199 which fixes #177 https://github.com/matryer/moq/issues/177, might be relevant here.

I believe what you did will more than likely fix it for everyone in a way more eloquent manner. Great job.

— Reply to this email directly, view it on GitHub https://github.com/matryer/moq/pull/198#issuecomment-1598991975, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWX4MRRQ77R2D7HNFW4H33XMG45XANCNFSM6AAAAAAYHFEIJU . You are receiving this because you commented.Message ID: @.***>

TheFellow commented 1 year ago

@TimVosch I cloned your fork, built from your PR branch and tested it against our production scenario (a generic closing over an imported struct) and it worked perfectly! Thank you!

cc @cbaker