Prevent auto-stubbing failure due to inaccessible property accessors (d…

…evlooped#847)

1. Add regression tests for lazy `SetupAllProperties`

There are two groups of tests here:

 * Those that test `SetupAllProperties` (called either directly, or
   implicitly through `Mock.Of<T>`) followed by a property acces. This
   reflects `SetupAllProperties`' current implementation, which only
   sets up properties upon their first access (just-in-time).

 * Those that test `SetupAllProperties` itself, without any property
   access. Unsurprisingly, these currently pass, but the tests are
   still good to have in case we ever return `SetupAllProperties` to a
   ahead-of-time batch operation implementation.

2. Fix regression by checking accessor visibility

3. Update the changelog

CHANGELOG.md

@@ -19,6 +19,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1
 #### Fixed
 
 * `mock.SetupAllProperties()` now setups write-only properties for strict mocks, so that accessing such properties will not throw anymore. (@vanashimko, #836)
+* Regression: `mock.SetupAllProperties()` and `Mock.Of<T>` fail due to inaccessible property accessors (@Mexe13, #845)
 
 
 ## 4.11.0 (2019-05-28)

src/Moq/Interception/InterceptionAspects.cs

@@ -359,20 +359,36 @@ public static bool Handle(Invocation invocation, Mock mock)
 				if (property.CanRead)
 				{
 					var getter = property.GetGetMethod(true);
-					propertyValue = CreateInitialPropertyValue(mock, getter);
-					getterSetup = new AutoImplementedPropertyGetterSetup(expression, getter, () => propertyValue);
-					mock.Setups.Add(getterSetup);
+					if (ProxyFactory.Instance.IsMethodVisible(getter, out _))
+					{
+						propertyValue = CreateInitialPropertyValue(mock, getter);
+						getterSetup = new AutoImplementedPropertyGetterSetup(expression, getter, () => propertyValue);
+						mock.Setups.Add(getterSetup);
+					}
+
+					// If we wanted to optimise for speed, we'd probably be forgiven
+					// for removing the above `IsMethodVisible` guard, as it's rather
+					// unlikely to encounter non-public getters such as the following
+					// in real-world code:
+					//
+					//     public T Property { internal get; set; }
+					//
+					// Usually, it's only the setters that are made non-public. For
+					// now however, we prefer correctness.
 				}
 
 				Setup setterSetup = null;
 				if (property.CanWrite)
 				{
 					MethodInfo setter = property.GetSetMethod(nonPublic: true);
-					setterSetup = new AutoImplementedPropertySetterSetup(expression, setter, (newValue) =>
+					if (ProxyFactory.Instance.IsMethodVisible(setter, out _))
 					{
-						propertyValue = newValue;
-					});
-					mock.Setups.Add(setterSetup);
+						setterSetup = new AutoImplementedPropertySetterSetup(expression, setter, (newValue) =>
+						{
+							propertyValue = newValue;
+						});
+						mock.Setups.Add(setterSetup);
+					}
 				}
 
 				Setup setupToExecute = invocationMethod.IsPropertyGetter() ? getterSetup : setterSetup;

tests/Moq.Tests/Regressions/IssueReportsFixture.cs

@@ -2571,6 +2571,45 @@ public void Setting_up_indexer_with_SetupProperty_throws_with_error_message_poin
 
 		#endregion
 
+		#region 845
+
+		public class Issue845
+		{
+			public class Foo
+			{
+				public virtual object Bar { get; internal set; }
+			}
+
+			[Fact]
+			public void Mock_Of_can_deal_with_internal_setter()
+			{
+				_ = Mock.Of<Foo>();
+			}
+
+			[Fact]
+			public void Mock_Of_plus_property_access_can_deal_with_internal_setter()
+			{
+				var mocked = Mock.Of<Foo>();
+				_ = mocked.Bar;
+			}
+
+			[Fact]
+			public void SetupAllProperties_can_deal_with_internal_setter()
+			{
+				var mock = new Mock<Foo>();
+				mock.SetupAllProperties();
+			}
+
+			[Fact]
+			public void SetupAllProperties_plus_property_access_can_deal_with_internal_setter()
+			{
+				var mock = new Mock<Foo>();
+				mock.SetupAllProperties();
+				_ = mock.Object.Bar;
+			}
+		}
+		#endregion
+
 		// Old @ Google Code
 
 		#region #47