8

Given a abstract factory implementation:

public class FooFactory : IFooFactory {
   public IFoo Create(object param1, object param2) {
      return new Foo(param1, param2);
   }
}

What unit tests would be written for this class? How can I verify that param1 and param2 were forwarded to the creation of Foo? Do I have to make these public properties of Foo? Wouldn't that be breaking encapsulation? Or should I leave this to integration testing?

JontyMC
  • 6,038
  • 6
  • 38
  • 39
  • Unit tests should depend on the functional requirement and expectations over the component being tested. I don't see any value of unit testing that class without the rest of the context. – Ivo Feb 02 '12 at 23:22
  • @ivowiblo I think you are confusing unit tests with BDD-style tests. If you don't write a test for this unit, then how do you know if it works? – Mark Seemann Feb 03 '12 at 06:57
  • make the unit test, but what will you be testing? which is the expectation for that unit? I said I don't see the value of doing unit test of that class without knowing anything else. Doing unit tests just because of doing unit tests doesn't make sense at all. Testing (units, behaviors, complete functionalities, whatever you want) should always be driven by expectations and requirements. If not, it's just snobism (if that word exists). – Ivo Feb 04 '12 at 04:39
  • @ivowiblo the test ensures that the correct parameters are passed to the creation of Foo. – JontyMC Feb 09 '12 at 12:04
  • And what is the value of that, besides improving coverage? – Ivo Feb 09 '12 at 14:23
  • I find value in improving coverage: by keeping it high, it is easier to locate the areas that need testing. – beluchin Sep 05 '12 at 20:23

5 Answers5

11

Here's how I would write one of a couple of unit tests for such a factory (with xUnit.net):

[Fact]
public void CreateReturnsInstanceWithCorrectParam1()
{
    var sut = new FooFactory();
    var expected = new object();
    var actual = sut.Create(expected, new object());
    var concrete = Assert.IsAssignableFrom<Foo>(actual);
    Assert.Equal(expected, concrete.Object1);
}

Does it break encapsulation? Yes and no... a little bit. Encapsulation is not only about data hiding - more importantly, it's about protecting the invariants of objects.

Let's assume that Foo exposes this public API:

public class Foo : IFoo
{
    public Foo(object param1, object param2);

    public void MethodDefinedByInterface();

    public object Object1 { get; }
}

While the Object1 property slightly violate the Law of Demeter, it doesn't mess with the invariants of the class because it's read-only.

Furthermore, the Object1 property is part of the concrete Foo class - not the IFoo interface:

public interface IFoo
{
    void MethodDefinedByInterface();
}

Once you realize that in a loosely coupled API, concrete members are implementation details, such concrete-only read-only properties have a very low impact on encapsulation. Think about it this way:

The public Foo constructor is also a part of the API of the concrete Foo class, so just by inspecting the public API, we learn that param1 and param2 are part of the class. In a sense, this already 'breaks encapsulation', so making each parameter available as read-only properties on the concrete class doesn't change much.

Such properties provide the benefit that we can now unit test the structural shape of the Foo class returned by the factory.

This is much easier than having to repeat a set of behavioral unit tests that, we must assume, already cover the concrete Foo class. It's almost like a logical proof:

  1. From tests of the concrete Foo class we know that it correctly uses/interacts with its constructor parameters.
  2. From these tests we also know that the constructor parameter is exposed as a read-only property.
  3. From a test of the FooFactory we know that it returns an instance of the concrete Foo class.
  4. Furthermore, we know from the tests of the Create method that its parameters are correctly passed to the Foo constructor.
  5. Q.E.D.
Mark Seemann
  • 225,310
  • 48
  • 427
  • 736
  • Yes, makes sense. I like the idea of differentiating between the public API of the interface and the concrete class. – JontyMC Feb 03 '12 at 09:27
2

Well, presumably those parameters make the returned IFoo have something true about it. Test for that being true about the returned instance.

jason
  • 236,483
  • 35
  • 423
  • 525
  • Wouldn't that mean you'd need to know about the internals of the IFoo in question? – JontyMC Feb 03 '12 at 00:05
  • No. You're passing in those parameters for a reason, to get some expected behavior. What is that behavior? Test for it. The spec for `FooFactory.Create` should say something like "creates an object that implements `IFoo` such that `P(param1, param2)` where `P` is a predicate dependent on `param1` and `param2`." Test that `P(param1, param2)` is true for the returned object! – jason Feb 03 '12 at 00:14
  • So... assuming that Foo is non-trivial and already covered by N tests, you'd essentially need to write N new, almost identical tests to test that the behavior of the returned Foo instance is consistent with the input to the Create method. That sounds like you'd be indirectly coupling the tests. Maintainability issues will follow. – Mark Seemann Feb 03 '12 at 07:05
0

You could make FooFactory a generic class that expects Foo as a parameter, and specify that to be a mock. Calling factory.Create(...) then creates an instance of the mock, and you could then confirm that the mock received the arguments you expected.

Arafangion
  • 11,517
  • 1
  • 40
  • 72
0

It depends on what Foo does with the 2 input objects. Check for something that Foo does with them that can easily be queried against. For instance, if methods (including getters or setters) are called on the objects, provide mocks or stubs that you can verify had the expected things called. If there is something on the IFoo that can be tested against, you can pass known values in through your mock objects to test against after creation. The objects themselves should not need to be publicly available to verify that you got them passed through.

I might also verify that the expected type (Foo) is returned, depending on if the other behaviors tested depend on the difference between IFoo implementations.

If there are any error conditions that can result, I'd try to force those as well, what happens if you pass in null for either or both objects, etc. Foo would be tested separately, of course, so you can assume during a FooFactory test that Foo does what it should.

That Chuck Guy
  • 443
  • 4
  • 12
  • You'd test the return type? What on earth is that buying you? You've set up all these abstractions and now you're going to write a test that breaks as soon as you change an implementation detail. Yikes. – jason Feb 02 '12 at 23:46
  • @Jason If I'm going to write test for multiple factories that each create an IFoo implementation, I would check that each returns the concrete type I expect. I question the need to test a factory that does so little as call new, but within the realm of what was asked... – That Chuck Guy Feb 02 '12 at 23:51
  • No. It's not behavior comsumers of the factory can depend on (the return type is `IFoo`) so it should not be tested. – jason Feb 02 '12 at 23:53
  • @Jason The "real" consumers absolutely should not depend on the return type, you're right. But, tests can depending on what you need to do to verify correct behavior. I'll edit to say "might", because it's true that isn't always something to look for. – That Chuck Guy Feb 02 '12 at 23:58
  • The tests shouldn't depend on it either! They're as real as any other consumer. – jason Feb 03 '12 at 00:03
  • @ThatChuckGuy "within the realm of what was asked..." I did offer the option of not unit testing at all and leaving it to integration tests. I suspect this is what most people are doing, but I was wondering if I was missing something. – JontyMC Feb 03 '12 at 00:10
  • No. This is definitely for unit tests. Different implementations of the factory interface might have different expected behavior. This is clearly for the realm of unit tests. – jason Feb 03 '12 at 00:16
  • @JontyMC I answered your question assuming that this was a hypothetical example. Therefor, I tried to answer as broadly as possible to hopefully give some ideas for your (or anyone else reading this later) exact situation. If all your factory really does is call new, then no, I would not unit test that. If it's more involved, it may warrant unit testing. – That Chuck Guy Feb 03 '12 at 00:17
  • @Jason For a factory, part of their expected behavior is returning the type you expect. If a factory can return a variety of IFoo implementations depending on inputs, etc, then that is part of the behavior and I'd want to test that it's returning the "correct" one. – That Chuck Guy Feb 03 '12 at 00:23
  • Again. The return type is `IFoo`, so no one can depend on any one concrete type being returned. To do so is to rely on an implementation detail that can chance without any what would be considered breaking changes (those that don't depend on implementation details). Heck, the factory could return one implementation of `IFoo` on one call and another implementation on the next. If you need the concrete type, just call `new Foo`; why bother with the factory? Or, at least, don't type the return type as `IFoo`, type it as `Foo`. – jason Feb 03 '12 at 00:38
  • @Jason The more I think about this, in most situations I agree that even the tests shouldn't care. But there may be times when it is a testable behavior, even though the real consumers would never care. Consider a factory that is supposed to create an IConnection (or an IConnectionFactory), given a configuration string. I would want to make sure that it gave me the right connection type back. Passing a pipe connection init to an https object isn't going to do much good. If the factory gives you the wrong type, all the tests would pass but you'd still have a problem. – That Chuck Guy Feb 03 '12 at 01:05
  • Well, now you've made me rethink my position slightly. If the spec does not say giving a connection string to a SQL Server database returns an instance of `SqlConnection`, then you can't depend on that behavior and shoud not test for it (instead test that it returns an `IConnection` that lets you connect to the database specified in the connection string). However, if the spec says you do get back a `SqlConnection` then that can be depended on and should be tested for. Similarly, if the spec says `FooFactory.Create` returns a `Foo`, test for it. I think we met in the middle. :-) – jason Feb 03 '12 at 02:33
  • @Jason The funny thing about this is that due to refactoring, I just removed this type of testing from 1 of the very few places I've actually had the need to do it. – That Chuck Guy Feb 07 '12 at 16:22
0

Factories are usually not unit-tested, at least according to my experience - there is not much value in unit testing them. Because it is the implementation of that interface-implementation mapping hence it refers the concrete implementation. As such, mocking Foo is not possible to check whether it receives those parameters passed in.

On the other hand, usually DI containers work as factories so if you are using one, you would not need a factory.

Aliostad
  • 80,612
  • 21
  • 160
  • 208
  • Abstract factories are required when a dependency relies on parameters only known at run-time or the lifetime is shorter than that of the consumer: http://stackoverflow.com/questions/1993397/abstract-factory-pattern-on-top-of-ioc – JontyMC Feb 03 '12 at 00:15
  • Most DI frameworks would do the passing of parameters at runtime. They can also do lifetime management. – Aliostad Feb 03 '12 at 11:39