Handle protected and internal property setters#627
Conversation
dtchepak
left a comment
There was a problem hiding this comment.
Hi @zvirja ! Looks good 👍
One question about breaking change -- how big a risk of people accidentally calling code they do not expect to run do you think this is? For example, if they were relying on internal not being updated?
I don't think it would be a common problem, but wanted to get your thoughts on it in case I am just lacking the imagination to see an issue.
tests/NSubstitute.Acceptance.Specs/FieldReports/Issue262_NonPublicSetterCall.cs
Outdated
Show resolved
Hide resolved
| public int PublicGetProtectedSetPropSetter | ||
| { | ||
| set => PublicGetProtectedSetProp = value; | ||
| } |
There was a problem hiding this comment.
I was playing around with this and added these members:
public virtual int OtherProp { protected get; set; }
public int GetOtherProp() => OtherProp;And this test:
[Test]
public void SetUpdatesProtectedGetter() {
var subs = Substitute.For<TestClass>();
subs.OtherProp = 21;
var result = subs.GetOtherProp();
Assert.That(result, Is.EqualTo(21));
}Not sure if that is a helpful example to include or not.
I did notice that if it was private get instead it did not work. I thought NonPublic would also match the private get?
There was a problem hiding this comment.
Thanks for the idea of Prop { protected get; set; } - added it as well 👍
public virtual int OtherProp { private get; set; }As for the property with private getter, we cannot override and intercept the getter here. When proxy is created, we intercept a setter only. Therefore, no matter what we assign - it will be never possible to read it.
The same actually happens with the property like this:
public virtual int OtherProp { get; private set; }Here you can override a getter only.
If you create a substitute, it will be very broken property functionality-wise, as it will "void" all the assigned values.
Notice that the scenarios above are only applicable to the partial mocks, which are not that frequent. For interface mocks which are way more popular those scenarios are not applicable.
b355445 to
f088fa3
Compare
As for the interfaces, I think there should be no breaking changes at all. Interfaces allow you to have As for the classes, more I think more about it, more I believe that we are safe. This PR does not change what is intercepted - whatever was captured by The only problematic case I can think of is the following: public virtual int Prop { get; private set; }
public virtual int Prop { private get; set; }
// Or if you don't have `InternalsVisibleTo` attribute
public virtual int Prop { get; internal set; }
public virtual int Prop { internal get; set; }If regular substitute was created (using Looks like our change only normalizes the expected property behavior and as for the weird cases - they were already broken before this change, so should be not a concern. It's just my analysis, I might overlook something as well 😊 |
dtchepak
left a comment
There was a problem hiding this comment.
Thanks a lot for this. 👍
Happy for this to be merged if you are. 😄
|
@dtchepak Done! Consider dropping a new version if we don't plan fix anything else in the nearby future 😉 |
Fixes #626
This change adds support for the non-public property setters. The original issue was about internal properties and this change handles them as well.
I didn't add unit test for internal properties as I didn't want to set
assembly: InternalsVisibleTo()attribute for the test assembly to not affect the existing tests.Please kindly take a look and give your feedback! Thanks!