Add CallsBaseMethods option to fake creation #192

Closed
Alxandr opened this Issue Nov 7, 2013 · 15 comments

Projects

None yet

5 participants

@Alxandr
Alxandr commented Nov 7, 2013

Currently I find myself adding extension-methods like these:

namespace FakeItEasy
{
    static class FakeUtil
    {
        public static IFakeOptionsBuilder<TFaked> CallBase<TFaked>(this IFakeOptionsBuilder<TFaked> config)
        {
            return config.Strict()
                .OnFakeCreated(fake =>
                    A.CallTo(fake).Where(c => !c.Method.IsFinal && c.Method.GetBaseDefinition() != null)
                        .CallsBaseMethod());
        }
    }
}

However, as far as I can tell, this has two drawbacks. First, it's not actually part of FakeItEasy, and second (which is more important, but also caused by the first) it doesn't work optimally. What I want is to be able to say that if you don't find a configuration for the current method-call AND the method has a base method.

Note, if this is possible today in some way I don't know about, please let me know.

@adamralph
Member

@Alxandr thank you for raising this - we'll look into it ASAP.

@Alxandr
Alxandr commented Nov 8, 2013

It should probably be simple to implement. The same place you fallback to throw an exception in strict, one could fall back to calling base (if it exists).

@Alxandr
Alxandr commented Nov 22, 2013

Just thought I'd notify that I found a logic error in my previous sample. The following seems to work consistently though:

namespace FakeItEasy
{
    internal static class FakeUtil
    {
        public static IFakeOptionsBuilder<TFaked> CallBase<TFaked>(this IFakeOptionsBuilder<TFaked> config)
        {
            return config.Strict()
                .OnFakeCreated(fake =>
                    A.CallTo(fake).Where(c => !c.Method.IsFinal && !c.Method.IsAbstract)
                        .CallsBaseMethod());
        }
    }
}

Though I'm still worried about which order the configurations are used in. For instance, if I do something like create a fake object like this:

var fake = A.Fake<object>(conf => conf.CallBase());
A.CallTo(() => fake.ToString()).Returns("This is a fake");

I need to be absolutely sure that fake.ToString() returns my custom string, and doesn't call the base method. And as far as I know, with FakeItEasy today is just up to "it just happened to be made that way", and can at any point be refactored. If this is to be supported, some tests would definitely be in place.

@blairconrad
Member

Hi, @Alxandr.

I'm not a sophisticated IFakeOptionsBuilder- or Strict-user, so can you walk me through exactly what you're proposing?

I don't understand what "per-default calling" means, but it looks like your extension method provides an easy way for you to configure new fakes so that

  • they're Strict, and
  • any call to the fake, where the method is implemented on the faked type and is overridable, call the "base method" on the faked type

So far so good?

If so (and if not, please correct me!), can you explain to me what the benefit is? It seems to me that having most methods (that is non-abstract ones) default to the base method negates the value of making the fake Strict. This isn't a challenge, but something I'm curious about: why fallback to the implemented (non-abstract) methods? Is it because you typically fake interfaces but want the original methods on object (ToString, GetHash, Equals) to be called? Or do you commonly fake classes, and segregate the methods into abstract and non-abstract according to some scheme?

Finally (sorry, maybe I'm slow, or maybe just not putting enough effort into this), you're suggesting that your extension method be moved into the framework to live alongside Strict, yes? I don't think there's any efficiency benefit to be gained by bringing the extension into the framework, nor do I think the logic can be greatly simplified (which aren't reasons to not bring it in, I'm just trying to allay some or your voiced concerns).

One last point (something I know!): your example where you call CallBase and then explicitly configure fake.ToString will work, and work consistently. Later rules made using A.CallTo will override earlier ones, so ToString should always return "This is a fake". You say

with FakeItEasy today is just up to "it just happened to be made that way", and can at any point be refactored

but there's nothing to worry about here: subsequent A.CallTos overriding earlier ones is the intended behaviour and is promised in the documentation; there's no plan to change the behaviour, and we'd work to avoid such a surprise.

@Alxandr
Alxandr commented Aug 12, 2014

Hi.

There's one thing you're mistaken about with regards to the point of my extension method. The point is not to make a strict object that calls base whenever possible. The only reason I made it strict, is that since the framework provides me no way of configuring fakes to call base method "by default" (get back to that later), so by making it strict, I was sure that no methods accidentally didn't do just that (ie. I had a bug in the logic of what methods to call base on).

When you say that "later configurations always trumps older ones" that means that it's actually doable with the extension-method I have today, but I think this should be a deeper integration though. Now, I haven't read the source of FakeItEasy, so I'm making some assumptions as to how things work underneath, but it should help show what it is I'm asking for.

This is (simplified) how I imagine a call to a "faked" method goes:

  1. The method Foo() is called.
  2. Check if it's configured, if it is, call the configured callback or similar, and return, else continue downwards.
  3. Special casing of certain return types like Task.
  4. Return default(ReturnType).

What I propose is that a bool is added to fake objects, specifying that if it is set a method-call should by default run the base method, so it would turn into something like this:

  1. The method Foo() is called.
  2. Check if it's configured, if it is, call the configured callback or similar, and return, else continue downwards.
  3. Check if callBase is set. If it is, and a non-abstract base-method exist, call that and return, else continue downwards.
  4. Special casing of certain return types like Task.
  5. Return default(ReturnType).

Did that make sense?

@blairconrad
Member

Thanks for the response, @Alxandr. That's cleared things up.

Your guess about how FakeItEasy handles a call to a faked method is close. A little closer is

  1. Of all the rules that are defined on the fake, find the first one that matches the call and has not been called too many times already. n.b.: All fakes come with 5 rules out of the box:
    • one, which is considered before user-supplied rules, that handles events, and
    • four, which are considered after user-supplied rules, that handle:
      • object methods such as ToString
      • auto-faking of properties
      • property setters
      • supplying a default return value (this one always matches, so will be called if nothing else was), which will return a Dummy, and that's what handles things such as Task: one of the builtin Dummy rules is for Tasks.
  2. Apply the rule

Hm. That list wasn't was exciting as I thought it would be. Anyhow, you can see that FakeItEasy has no concept of an "unconfigured method". When it's executing a method, FakeItEasy just looks through the list of rules until one applies. So, an equivalent version of your suggestion would be to insert a rule into the list so that it fires before one of the "built-in" rules. This is exactly what your extension method does.

You don't actually care about making the fake Strict. This means that, if your guard logic were correct, you could use

public static IFakeOptionsBuilder<TFaked> CallBase<TFaked>(this IFakeOptionsBuilder<TFaked> config)
{
    return config.OnFakeCreated(fake =>
        A.CallTo(fake).Where(c => !c.Method.IsFinal && !c.Method.IsAbstract)
            .CallsBaseMethod());
}

or perhaps even

public static IFakeOptionsBuilder<TFaked> CallBase<TFaked>(this IFakeOptionsBuilder<TFaked> config)
{
    return config.OnFakeCreated(fake =>
        A.CallTo(fake).Where(c => !c.Method.IsAbstract)
            .CallsBaseMethod());
}

since, as I understand things, FakeItEasy shouldn't even see the call to a method that IsFinal.

And so, it seems your request comes down to moving one that latest extension method onto FakeOptionsBuilderExtensions, to sit alongside Strict.

Have I got it?

@Alxandr
Alxandr commented Aug 12, 2014

Yeah. That seems correct. The reason I came to this question (if I haven't already written so in an earlier post) was that I was converting a test-base from one test framework to another (and figured I'd at the same time convert the mocking framework too from Moq to FakeItEasy). And Moq has a notion of creating a new Moq that has CallsBase = true, for which I found no equivalent.

@blairconrad
Member

Excellent. Thanks for clearing things up. Me, I'm a fan of "fewer features is more", but I can see how this may be of use to people (I mean besides you), so I'm inclined to add it. I think the name would be better changed from CallBase to CallsBaseMethods to align with RuleBuilder.CallsBaseMethod. Then we'd have:

fake = A.Fake<IMyInterface>(options => options.CallsBaseMethods());

@FakeItEasy/owners, how do you stand on the feature?

@blairconrad blairconrad added enhancement and removed question labels Aug 12, 2014
@philippdolder
Member

@blairconrad In general, if we can increase the number of happy users of FakeItEasy with this kind of features I'm in strong favor of doing these features.

So, yes, do it :)

@blairconrad
Member

@philippdolder, that's good enough for me. Thanks.

@Alxandr, are you interested in implementing, or would you prefer me to? It is all the same to me. At this point, it's just copying the code and writing tests, which will end up dwarfing the implementation. :)

@blairconrad blairconrad changed the title from Allow per-default calling to base-methods to Add CallsBaseMethods option to fake creation Aug 13, 2014
@blairconrad blairconrad added 1 - Ready and removed 0 - Backlog labels Aug 13, 2014
@Alxandr
Alxandr commented Aug 13, 2014

I have exams. So by all mean, please do write it on your own.

@blairconrad blairconrad self-assigned this Aug 13, 2014
@blairconrad blairconrad added this to the 1.24.0 milestone Aug 13, 2014
@blairconrad blairconrad added 2 - Working and removed 1 - Ready labels Aug 14, 2014
@blairconrad blairconrad added a commit to blairconrad/FakeItEasy that referenced this issue Aug 15, 2014
@blairconrad blairconrad #192 - added CallsBaseMethods options builder extension 2bbce00
@patrik-hagne
Member

I love it!

@blairconrad
Member

@patrik-hagne, I'm glad! If you love it enough, don't be shy about merging it. 😉

@blairconrad blairconrad added 3 - Done and removed 2 - Working labels Aug 19, 2014
@blairconrad blairconrad referenced this issue Aug 20, 2014
Closed

1.24.0 release #347

16 of 16 tasks complete
@blairconrad
Member

@alxandr, thanks very much for your work on this issue. Look for your name in the release notes. 🏆

This issue has been fixed in release 1.24.0.

https://www.nuget.org/packages/FakeItEasy/1.24.0
https://github.com/FakeItEasy/FakeItEasy/releases/tag/1.24.0

@Alxandr
Alxandr commented Aug 25, 2014

No problem :)
Thanks for such an awesome piece of software in the first case. Glad to help :).

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