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

Setup property doesn't work as expected #1248

Closed
Naxemar opened this issue Mar 21, 2022 · 3 comments · Fixed by #1260
Closed

Setup property doesn't work as expected #1248

Naxemar opened this issue Mar 21, 2022 · 3 comments · Fixed by #1260
Assignees
Labels
Milestone

Comments

@Naxemar
Copy link

Naxemar commented Mar 21, 2022

Hi everyone, from time to time we are performing a regular nuget update, and currently part of our previously working tests are not working any more.

IDE: VS 2022
Test Runner: vstest
Test framework: mstest
.NET Version: 4.8

Steps to reproduce:

using Microsoft.VisualStudio.TestTools.UnitTesting;
using Moq;

namespace MoqIssueTestProject
{
    public interface ITest1Interface
    {
        bool IsDone { get; set; }
    }

    public interface ITest2Interface : ITest1Interface
    {
    }

    public abstract class AbstractClass
    {
        public virtual bool IsDone { get; set; }
    }

    public class InheritedClass : AbstractClass, ITest1Interface
    {
    }

    public class ClassToTest
    {
        private readonly ITest1Interface instance;

        public ClassToTest(ITest1Interface instance)
        {
            this.instance = instance;
        }

        public void DoSomething()
        {
            instance.IsDone = true;
        }
    }

    [TestClass]
    public class UnitTest1
    {
        [TestMethod]
        public void TestMethod1()
        {
            // Arrange
            var mockDependency = new Mock<InheritedClass>();
            var mockDependency2 = mockDependency.As<ITest2Interface>().SetupProperty(x => x.IsDone, false);
            var tc = new ClassToTest(mockDependency2.Object);

            // Act
            tc.DoSomething();

            // Verify
            mockDependency.VerifySet(x => x.IsDone = true, Times.Once()); // Fine
            mockDependency2.VerifySet(x => x.IsDone = true, Times.Once()); // Fine
            Assert.IsTrue(mockDependency2.Object.IsDone); // This fails
            Assert.IsTrue(mockDependency.Object.IsDone); // This fails
        }
    }
}

Last version where it was working as expected: 4.16.1

@Naxemar Naxemar changed the title Setup property regression Setup property doesn't work as expected Mar 23, 2022
@stakx
Copy link
Contributor

stakx commented Apr 17, 2022

@Naxemar, thanks for reporting. I'll look into it soon.

@stakx stakx added the bug label May 12, 2022
@stakx
Copy link
Contributor

stakx commented May 12, 2022

This is a bug caused by this bit of code inside StubbedPropertySetup, which compares methods in a way-too-simplistic fashion.

https://github.com/moq/moq4/blob/a56901655b5149c1dda38a1d843c0c6b51f9d950/src/Moq/StubbedPropertySetup.cs#L101-L105

This code decides whether or not the stubbed property setup will trigger for the call to mockDependency2.Object.IsDone = true. It doesn't correctly identify that the actual invocation's method (InheritedClass.set_IsDone) matches the one referred to in the setup (ITest1Interface.set_IsDone); but it should, because the former is the implementation of the latter.

@stakx
Copy link
Contributor

stakx commented May 12, 2022

Note to self: We might get away with simply comparing property names (as already happens in StubbedPropertiesSetup). This just might work since SetupProperty doesn't support indexers, so the property signature becomes irrelevant. The part I'm unsure about pertains to explicitly implemented properties: if a property is explicitly implemented due to redeclaration in a derived type, then its name isn't strictly unique. But this may be a rare edge case.

@stakx stakx self-assigned this May 12, 2022
@stakx stakx added this to the 4.18.1 milestone May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants