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

Verify evaluates the same object on reuse #1316

Closed
lund76 opened this issue Dec 30, 2022 · 3 comments
Closed

Verify evaluates the same object on reuse #1316

lund76 opened this issue Dec 30, 2022 · 3 comments

Comments

@lund76
Copy link

lund76 commented Dec 30, 2022

Hi

It was really hard to come up with a descriptive title, so right of the bat .. sorry for that.

But I've come across something that I'm not sure is a bug, or by design. But until I broke it down into a smaller example, I was quite sure it was a bug.

I've created a repo with democode:
https://github.com/lund76/MoqMyWords

My mockup has a method that clearly has an error (it's not supposed to update all 3) :

public class PriceService
{
    private readonly IInventoryService _inventoryService;

    public PriceService(IInventoryService inventoryService)
    {
        _inventoryService = inventoryService;
    }

    public async Task<Product> UpdateOnlyNewestPrice(List<Product> products)
    {
        foreach (var p in products.OrderByDescending(o => o.Created).Take(3))
        {
            await _inventoryService.UpdateAsync(p);
        }
        return products.FirstOrDefault() ?? new Product();
    }
}

And my test (verify a little funny formed to showcase this issue)

[Fact]
public async Task MoqMyWord_UpdateService_OnlyCalledOnce()
{

    var productList = new List<Product>
    {
        new() { Price = 1, Created = DateTime.UtcNow.AddDays(-1) },
        new() { Price = 2, Created = DateTime.UtcNow.AddDays(-2) },
        new() { Price = 3, Created = DateTime.UtcNow.AddDays(-3) }
    };

    Mock<IInventoryService> service = new Mock<IInventoryService>();

    var priceService = new PriceService(service.Object);

    await priceService.UpdateOnlyNewestPrice(productList);

    //For the sake of the argument a silly way to evaluate any price less than 3 is valid
    //This is only to make sure the test fails.
    service.Verify(u => u.UpdateAsync(It.Is<Product>(p => EvaluatePrice(p))),Times.Once);          
}
private bool EvaluatePrice(Product product)
{
    _outputHelper.WriteLine($"product price is {product.Price}");
    return product.Price < 3;
}

This test fails as expected, since the service is called 2 times:
image

If I then make a shitty change to the implementation, and only change a property of an existing object:
image

I would still expect it to fail with 2 calls to the service, but it fails with no calls to the service. Moq doesn't recognize the change in the object
image

@stakx
Copy link
Contributor

stakx commented Dec 30, 2022

Hi @lund76, no worries about the issue title, it's actually quite acurate. This question gets asked a lot (recently e.g. in #1309 and #1187). When Moq records invocations (for later verification), it simply captures the invocation arguments in an object[] array – it does not deep-clone every argument. This means that the usual .NET rules apply: reference types (such as your Product class) are captured by reference, and value types are captured by value (which essentially amounts to a shallow clone). Therefore if the same Product is captured on three invocations, and you change that Product instance over time, this will be reflected in the recorded invocations.

I'm a little early, but... have a happy new year! 🎆

@stakx stakx closed this as completed Dec 30, 2022
@lund76
Copy link
Author

lund76 commented Dec 30, 2022

Thanks for the reply. It what pretty sure it was something along those lines.
I've tried searching a bit for the issue, but must have given up to quickly - sorry for that :)

Have a happy new year as well :)

@stakx
Copy link
Contributor

stakx commented Dec 31, 2022

FYI @lund76, I'm likely going to merge #1319 which will provide a new way to solve this issue; see the last code example in the PR's description for an explanation of how this might help.

@devlooped devlooped locked and limited conversation to collaborators Sep 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants