Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

MOCK-429: add support for assignable types to Eq matcher #481

Merged
merged 1 commit into from
Oct 23, 2020

Conversation

GrigoriyMikhalkin
Copy link
Contributor

Solution to #429. Adds support for assignable types to Eq matcher.

@cvgw
Copy link
Collaborator

cvgw commented Oct 16, 2020

Hi @GrigoriyMikhalkin, is this PR ready for review?

@GrigoriyMikhalkin
Copy link
Contributor Author

@cvgw Hi! Yep.

x1Val := reflect.ValueOf(e.x)
x2Val := reflect.ValueOf(x)

if x1Val.Type().AssignableTo(x2Val.Type()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for aliased types (type A []string) it doesn't matter, but if we are talking about interfaces I think there may be an issue here.

type Foo interface {
  Meow() string
}

type Baz struct {}

func (b Baz) Meow() string {
  return "meow"
}

type Boomer interface {
  Boom(Foo)
}

func TestBoom(t *testing.T) {
  // omitted for brevity
  var bazzer Baz = newBazzer()
  mockBoomer.EXPECT().Boom(gomock.Eq(bazzer)).Times(1)
  var fooer Foo = Foo(bazzer)
  mockBoomer.Boom(fooer) // Foo not assignable to Baz
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But interface isn't a type(it has underlying type instead). Here is script in playground https://play.golang.org/p/AvDmLq8arUK -- hope i correctly understood your concern.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I think I understand. In the case of an interface the object will be resolved to its underlying type which must be assignable to the expected type. Thanks for the demo!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants