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

If one interface implements another interface and both contains the same property the property is not set on the original object that implements the first interface #31

Closed
imshz opened this issue Dec 17, 2012 · 7 comments

Comments

@imshz
Copy link

imshz commented Dec 17, 2012

What steps will reproduce the problem?

  1. See code below
  2. var myFake = A.Fake<IMyInterface>();
  3. Send to SetValues(myFake) on SomeClass
  4. check "Number"-value on myFake, it is null
  5. cast myFake to IYourInterface, and check "Number"-value the value is 0.

What is the expected output? What do you see instead?
That the value on MyFake to be 0 not null.

What version of the product are you using? On what operating system?
Tried on version 1.7.4257.42 (other: c# / windows 7 / visual studio 2010 ultimate)

Please provide any additional information below.

Code:

public interface IMyInterface : IYourInterface
{
   decimal? Number { get; set; }
}

public interface IYourInterface
{
   decimal? Number { get; set; }
}

public class SomeClass
{
   public void SetValues(IYourInterface yi)
   {
      yi.Number = 0;
   }
}
@philippdolder
Copy link
Member

Can you please also provide your test code? (The part you are describing as step 2 to 5)

@imshz
Copy link
Author

imshz commented Dec 21, 2012

Hi, Okey. Here is the code:

TestShouldWork do fail. Im using NUnit.

[TestFixture]
public class FakeItTests
{
    [Test]
    public void TestShouldWork()
    {
        var someClass = new SomeClass();

        var fake = A.Fake<IMyInterface>();

        someClass.SetValues(fake);

        Assert.IsNotNull(fake.Number);
        Assert.IsTrue(fake.Number == 5);
    }

    [Test]
    public void TestWorks()
    {
        var someClass = new SomeClass();

        var fake = A.Fake<IMyInterface>();

        someClass.SetValues(fake);

        Assert.IsNotNull(((IYourInterface)fake).Number);
        Assert.IsTrue(((IYourInterface)fake).Number == 5);
    }
}

public interface IMyInterface : IYourInterface
{
    decimal? Number { get; set; }
}

public interface IYourInterface
{
    decimal? Number { get; set; }
}

public class SomeClass
{
    public void SetValues(IYourInterface yi)
    {
        yi.Number = 5;
    }
}

@philippdolder
Copy link
Member

I had a look into your issue just now.
I'd like to explain why FakeItEasy behaves the way it does.

The following code (manual fake implementation) shows the behavior of FakeItEasy

public class MyFake : IMyInterface
{
    int? IYourInterface.Number { get; set; }
    int? IMyInterface.Number { get; set; }
}

As you see, it implements the interfaces explicitly. Therefore the behavior differs depending on how you "look" at the object.

So far there is no way (as far as I know) to tell FakeItEasy to not implement the interfaces explicitly when there are matching methods or properties. Probably there should be a FakeOption that allows to change the implementation to explicit or non-explicit. But that's not just 2 hours of work.

For me the question in your case is: Are these your own interfaces you have to fake? Or are these 3rd party ones, that you can't change. To me the interface declaration looks like a design smell. Maybe you can change that?

Looking forward to your feedback.

@imshz
Copy link
Author

imshz commented Jan 3, 2013

Thank you for your feedback.

I agree that it looks like a design smell, but it is not :)

I have tried to think of other ways to reimplement my code, but the issues is this code i autogenerated. Its several classes that represents business objects and their interfaces contains these properties.
I have one interface that is to combine all common properties, and its implemented by the other interfaces.
And it is not possible for me to remove the properties from autogenerated interfaces.

I work on a large project where we are considering to change our mocking framework from RhinoMocks to something else. This is the only thing we have discovered fakeiteasy does not support that we need.

Please let me know if you decide to implement this fix in the future, we will propobly hold on to Rhino until we find something that supports every scenario with existing tests.

(sorry for my poor english)

@philippdolder
Copy link
Member

Hi,

To have this feature in FakeItEasy we would need to introduce an additional Configuration possibility to let you decide if your Fake should implement the interface(s) explicitly or implicitly. In theory that could resolve your issue. But, with my current knowledge of Castle.Core (which we use to generate the dynamic proxies) and FakeItEasy internals I don't know how I could achieve a proxy that implements an interface implicitly. So, for the moment I don't think it is foreseeable that we can support this feature in the next 2 or 3 months.

But, I like to try helping you get this working with the current capabilities of FakeItEasy.

I'm still convinced that it is a design smell, but possibly it's the fault of the generated code which you probably cannot influence much.

Let me ask a couple more question regarding the design to understand your issue completely:
IMyInterface and IYourInterface are both generated code?
What tool/component are you using that generates this code?

In case IMyInterface is your code, then just don't redeclare the properties and simply write:
public interface IMyInterface : IYourInterface { }

@adamralph
Copy link
Contributor

I agree with Philipp. This is incorrect design. If an interface inherits from another interface it should not re-declare members from the parent interface. It doesn't matter whether the code is auto generated or manually written. The code as it stands is all that counts. If this is the result of auto-generation then the auto-generation needs to be fixed.

@adamralph
Copy link
Contributor

@imshz we'll leave this issue open for a while to allow you to comment.

Unless we see a compelling case for FakeItEasy to support this pattern we'll close this issue as a wontfix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants