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

Mock.As behavior change #1051

Closed
bluechrism opened this issue Aug 27, 2020 · 6 comments
Closed

Mock.As behavior change #1051

bluechrism opened this issue Aug 27, 2020 · 6 comments
Assignees

Comments

@bluechrism
Copy link

bluechrism commented Aug 27, 2020

We recently updated from Moq 4.7.8 to Moq 4.14.5 and while most of the breaking changes were easy enough to work out, a few things are becoming much more painful. One of those things seems to be a change in the behavior of some properties on a class that uses Mock.As to mock out various interfaces where the same property is present in each.

It may be a fairly convoluted interface hierarchy, but these things represent a value that can change between a defined set of positions of T, as long as the object is enabled. The version with index is something that changes by index of the target value rather than providing the actual value we want to use. The ReadOnly version is used if we don't want a consumer to be able to make changes. For now, I'm going to use the name Widget for these. We create these objects as concrete classes of Widget and pass them around by interface. There are tests that are attempting to verify a mocked Thing's value is updated, or where the change of value of a mocked Thing is a trigger for some other change. A mock that might be passed into a class might be as IWidget or IWidgetWithIndex, but then in a child method, or event handler used as IReadOnlyWidget, and so changes that are made need to be reflected in the mock object regardless which interface is being used.

Example interface hierarchy

public interface IReadonlyWidget<T>
{
   bool Enabled { get; }
   T Position { get; }
}

public interface IWidget<T> : IReadOnlyWidget
   bool Enabled { get; set; }
   T Position { get; set;}
}
public interface IWidgetWithIndex
{
   bool Enabled {get;}
   int Index {get; set;}
}
public interface IWidgetWithIndex<T>: IWidgetWithIndex, IWidget<T>
{
   bool Enabled {get; set;}
}

// The concrete class we are actually creating in code and that the Mocks are substituting for is like this: 
public class Widget<T> : IWidgetWithIndex<T>
{
    public bool Enabled { get; set; }
    public T position { get; set; }
    public int Index {get; set;}
}
There would be passed in a list of available values/positions and the Index property updates the position, and vice based on that list, and either only updates if Enabled is true:

Example extension class for testing - the two public extensions call the private one.

public static Mock<IWidgetWithIndex<TPosition>> SetupEnabledProperty<TPosition>(this Mock<IWidgetWithIndex<TPosition>> widgetMock, bool enabled)
{
    // widgetMock.As<IWidgetWithIndex>().SetupGet(a => a.Enabled).Returns(enabled); // Original 4.7.8
    widgetMock.SetupProperty(a => a.Enabled, enabled); 	// Update for Change for 14.4.5, as the initial position wasn't being retained        
    return widgetMock.SetupEnabledProperty<TPosition, IWidgetWithIndex<TPosition>>(enabled);
}

public static Mock<IWidget<TPosition>> SetupEnabledProperty<TPosition>(this Mock<IWidget<TPosition>> widgetMock, bool enabled)
{
	return widgetMock.SetupEnabledProperty<TPosition, IWidget<TPosition>>(enabled);
}

private static Mock<TWidget> SetupEnabledProperty<TPosition, TWidget>(this Mock<TWidget> widgetMock, bool enabled)
where TWidget : class, IWidget<TPosition>
{
     widgetMock.As<IReadOnlyWidget<TPosition>>().SetupGet(a => a.Enabled).Returns(() => widgetMock.Object.Enabled);
     return widgetMock.SetupProperty(a => a.Enabled, enabled);
}

After banging around at the real tests that expected the states of these Widgets to be changing and seeing them not change,
I ended up creating a test to test the Moq extensions for the Enabled property and it looks like this:

[TestMethod]
public void SetupEnabledProperty_ForWidgetWithIndex_GetAndSetWork()
{
	bool originalPosition = true;
	var mockWidget = new Mock<IWidgetWithIndex<int>>();
	mockWidget.SetupEnabledProperty(originalPosition);
	var cut = mockWidget.Object;

	Assert.AreEqual(originalPosition, cut.Enabled); // Failed here without update to extension method.
	Assert.AreEqual(originalPosition, (cut as IWidget<int>).Enabled);
	Assert.AreEqual(originalPosition, (cut as IReadOnlyWidget<int>).Enabled);
	Assert.AreEqual(originalPosition, (cut as IWidgetWithIndex<int>).Enabled);

	bool newPosition = false;
	cut.Enabled = newPosition;

	Assert.AreEqual(newPosition, cut.Enabled);  //Passes
	Assert.AreEqual(newPosition, (cut as IWidget<int>).Enabled); // Fails 
	Assert.AreEqual(newPosition, (cut as IReadOnlyWidget<int>).Enabled); // Fails
	Assert.AreEqual(newPosition, (cut as IWidgetWithIndex<int>).Enabled); // Passes
}

With Moq 4.7.8 this test passes.
After updating to 4.14.5, without the change motioned in the extensions class, it would now fail at the first checks to verify the original enabled value is set. Now it fails checking the updated state as IWidget or IReadOnlyWidget.
I can't figure out any version of the extension method to setup the setter which works for IWidgetWithIndexas well as IWidget and IReadOnlyWidget - if I get IWidgetWithIndex working, the other two break, and vice versa.

Note that a similar test for Mock does work (take the above and strip out the last of each set of asserts). Also similar tests for Position also work, but Position isn't on the IWidgetWithIndex interface.

@stakx
Copy link
Contributor

stakx commented Aug 29, 2020

Could you please provide a complete minimal code example that compiles without errrors?

@bluechrism
Copy link
Author

I have been able to fix my immediate issue but i'll still get a repro together for the example.
The fix involved using SetupProperty for the enabled property on one version of the mock and when using mock as use SetupGet or SetupSet to get or update the value from the first version of the mock, casted to the type it was when I used SetupProperty.

@bluechrism
Copy link
Author

I have been able to get my specific tests working but will continue with this issue so one of the maintainers of Moq can determine if this is an intentional impact or represents a bug in how moq now works.

I realize out interface hierarchy is perhaps a little convoluted. The attached Zip contains a minimal sample that represents our setup closely enough.

[
Mock_As_Behavior.zip.zip
](url)

In the WidgetTestExtensions class, there are two methods I want to mention:
SetupIsActivePropertyMock4_7_8
SetupIsActivePropertyMock4_14_5
These are the original and new versions of these methods. The project is SDK style (net472) so to switch between them just update the reference version number in the csproj. There are two corresponding tests in the WidgetExtensionsTests class that call each version. When running with Moq 4.14.5, the test using SetupIsActivePropertyMock4_7_8 fails.

The first is the version of the method that worked under Mock 4.7.8. The second is the version I have got working under Mock 4.14.5.

@stakx
Copy link
Contributor

stakx commented Sep 7, 2020

Thanks for the repro code @bluechrism. Will take a look sometime during the next few days.

@stakx stakx removed the needs-repro label Sep 7, 2020
@stakx stakx self-assigned this Sep 7, 2020
@stakx
Copy link
Contributor

stakx commented Sep 27, 2020

This change was introduced with 9f65584, between Moq versions 4.7.63 and 4.7.99. If memory serves, back then, Mock.As had several inconsistencies and bugs that were introduced to simplify mocking of Entity Framework contexts, at the cost of many other equally common scenarios. Those inconsistencies got fixed, thus the changes in behavior... it's very unlikely that we will (or should) go back to the previous behavior.

As to your particular scenario @bluechrism, yes, your type hierarchy is rather involved. Can you simplify your repro solution any further?

@stakx
Copy link
Contributor

stakx commented Oct 22, 2020

Closing this issue due to inactivity.

@stakx stakx closed this as completed Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants