Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SetupAllProperties() and Mock.Of<T> regression in Moq 4.12.0 #870

Closed
mattzink opened this issue Aug 6, 2019 · 6 comments · Fixed by #871 or #878
Closed

SetupAllProperties() and Mock.Of<T> regression in Moq 4.12.0 #870

mattzink opened this issue Aug 6, 2019 · 6 comments · Fixed by #871 or #878
Assignees
Labels
Milestone

Comments

@mattzink
Copy link

mattzink commented Aug 6, 2019

The following tests pass with Moq 4.11.0 and fail with 4.12.0:

var httpContext = Mock.Of<Microsoft.AspNetCore.Http.HttpContext>();
httpContext.Items = new Dictionary<object, object>();
Assert.NotNull(httpContext.Items);
var httpContext = new Mock<Microsoft.AspNetCore.Http.HttpContext>()
    .SetupAllProperties();
httpContext.Object.Items = new Dictionary<object, object>();
Assert.NotNull(httpContext.Object.Items);

This seems to be very likely caused by #826
Perhaps this new code doesn't handle abstract types correctly?

Environment: Windows x64, .NET Core 2.2.1

@mattzink
Copy link
Author

mattzink commented Aug 6, 2019

This pattern also fails:

var httpContext = Mock.Of<HttpContext>(
    context => context.Items == new Dictionary<object, object>());
Assert.NotNull(httpContext.Items);

@stakx stakx self-assigned this Aug 6, 2019
@stakx stakx added the bug label Aug 6, 2019
@stakx stakx added this to the 4.13.0 milestone Aug 6, 2019
@stakx
Copy link
Contributor

stakx commented Aug 6, 2019

Thanks for reporting, @mattzink. I've found the regression. The problem lies here:

https://github.com/moq/moq4/blob/731567c12159d855658726479eda8d25626ae8b9/src/Moq/Extensions.cs#L51-L59

.StartsWith should be .Equals. I'm about to push a bug fix to master.

@mattzink
Copy link
Author

mattzink commented Aug 6, 2019

Ahh, so it's because the property name is "Items" that it thinks it's an indexer. Interesting!

@stakx
Copy link
Contributor

stakx commented Aug 6, 2019

Yes, SetupAllProperties does not set up indexers (and never has, AFAIK). Under the hood, indexers look like parameterized properties called Item. (Though the name can be customized, which Moq currently doesn't account for.)

Fixed in master. Watch out for an updated version of Moq later this month!

@kaan-kaya
Copy link

So even with this fix, this issue remains for properties called Item, then?

@stakx stakx reopened this Aug 8, 2019
@stakx
Copy link
Contributor

stakx commented Aug 8, 2019

@kaan-kaya, good point. I suppose it's time to finally fix the indexer recognition logic for good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants