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!

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

Successfully merging this pull request may close these issues.

2 participants