Unit testing, mocking - simple case: Service - Repository

Question

Consider a following chunk of service:

public class ProductService : IProductService {

   private IProductRepository _productRepository;

   // Some initlization stuff

   public Product GetProduct(int id) {
      try {
         return _productRepository.GetProduct(id);
      } catch (Exception e) {
         // log, wrap then throw
      }
   }
}

Let's consider a simple unit test:

[Test]
public void GetProduct_return_the_same_product_as_getProduct_on_productRepository() {
   var product = EntityGenerator.Product();

   _productRepositoryMock.Setup(pr => pr.GetProduct(product.Id)).Returns(product);

   Product returnedProduct = _productService.GetProduct(product.Id);

   Assert.AreEqual(product, returnedProduct);

   _productRepositoryMock.VerifyAll();
}

At first it seems that this test is ok. But let's change our service method a little bit:

public Product GetProduct(int id) {
   try {
      var product = _productRepository.GetProduct(id);

      product.Owner = "totallyDifferentOwner";

      return product;
   } catch (Exception e) {
      // log, wrap then throw
   }
}

How to rewrite a given test that it'd pass with the first service method and fail with a second one?

How do you handle this kind of simple scenarios?

HINT 1: A given test is bad coz product and returnedProduct is actually the same reference.

HINT 2: Implementing equality members (object.equals) is not the solution.

HINT 3: As for now, I create a clone of the Product instance (expectedProduct) with AutoMapper - but I don't like this solution.

HINT 4: I'm not testing that the SUT does NOT do sth. I'm trying to test that SUT DOES return the same object as it is returned from repository.

Answer 1

Uhhhhhhhhhhh...................

Q1: Don't make changes to code then write a test. Write a test first for the expected behavior. Then you can do whatever you want to the SUT.

Q2: You don't make the changes in your Product Gateway to change the owner of the product. You make the change in your model.

But if you insist, then listen to your tests. They are telling you that you have the possibility for products to be pulled from the gateway that have the incorrect owners. Oops, Looks like a business rule. Should be tested for in the model.

Also your using a mock. Why are you testing an implementation detail? The gateway only cares that the _productRepository.GetProduct(id) returns a product. Not what the product is.

If you test in this manner you will be creating fragile tests. What if product changes further. Now you have failing tests all over the place.

Your consumers of product (MODEL) are the only ones that care about the implementation of Product.

So your gateway test should look like this:

[Test]
public void GetProduct_return_the_same_product_as_getProduct_on_productRepository() {
   var product = EntityGenerator.Product();

   _productRepositoryMock.Setup(pr => pr.GetProduct(product.Id)).Returns(product);

   _productService.GetProduct(product.Id);

   _productRepositoryMock.VerifyAll();
}

Don't put business logic where it doesn't belong! And it's corollary is don't test for business logic where there should be none.

Answer 2

Personally, I wouldn't care about this. The test should make sure that the code is doing what you intend. It's very hard to test what code is not doing, I wouldn't bother in this case.

The test actually should just look like this:

[Test]
public void GetProduct_GetsProductFromRepository() 
{
   var product = EntityGenerator.Product();

   _productRepositoryMock
     .Setup(pr => pr.GetProduct(product.Id))
     .Returns(product);

   Product returnedProduct = _productService.GetProduct(product.Id);

   Assert.AreSame(product, returnedProduct);
}

I mean, it's one line of code you are testing.

Answer 3

Well, one way is to pass around a mock of product rather than the actual product. Verify nothing to affect the product by making it strict. (I assume you are using Moq, it looks like you are)

[Test]
public void GetProduct_return_the_same_product_as_getProduct_on_productRepository() {
   var product = new Mock<EntityGenerator.Product>(MockBehavior.Strict);

   _productRepositoryMock.Setup(pr => pr.GetProduct(product.Id)).Returns(product);

   Product returnedProduct = _productService.GetProduct(product.Id);

   Assert.AreEqual(product, returnedProduct);

   _productRepositoryMock.VerifyAll();
   product.VerifyAll();
}

That said, I'm not sure you should be doing this. The test is doing to much, and might indicate there is another requirement somewhere. Find that requirement and create a second test. It might be that you just want to stop yourself from doing something stupid. I don't think that scales, because there are so many stupid things you can do. Trying to test each would take too long.

Answer 4

I'm not sure, if the unit test should care about "what given method does not". There are zillion steps which are possible. In strict the test "GetProduct(id) return the same product as getProduct(id) on productRepository" is correct with or without the line product.Owner = "totallyDifferentOwner".

However you can create a test (if is required) "GetProduct(id) return product with same content as getProduct(id) on productRepository" where you can create a (propably deep) clone of one product instance and then you should compare contents of the two objects (so no object.Equals or object.ReferenceEquals).

The unit tests are not guarantee for 100% bug free and correct behaviour.

Answer 5

One way of thinking of unit tests is as coded specifications. When you use the EntityGenerator to produce instances both for the Test and for the actual service, your test can be seen to express the requirement

• The Service uses the EntityGenerator to produce Product instances.

This is what your test verifies. It's underspecified because it doesn't mention if modifications are allowed or not. If we say

• The Service uses the EntityGenerator to produce Product instances, which cannot be modified.

Then we get a hint as to the test changes needed to capture the error:

var product = EntityGenerator.Product();
// [ Change ] 
var originalOwner = product.Owner;  
// assuming owner is an immutable value object, like String
// [...] - record other properties as well.

Product returnedProduct = _productService.GetProduct(product.Id);

Assert.AreEqual(product, returnedProduct);

// [ Change ] verify the product is equivalent to the original spec
Assert.AreEqual(originalOwner, returnedProduct.Owner);
// [...] - test other properties as well

(The change is that we retrieve the owner from the freshly created Product and check the owner from the Product returned from the service.)

This embodies the fact that the Owner and other product properties must equal the the original value from the generator. This may seem like I'm stating the obvious, since the code is pretty trivial, but it runs quite deep if you think in terms of requirement specifications.

I often "test my tests" by stipulating "if I change this line of code, tweak a critical constant or two, or inject a few code burps (e.g. changing != to ==), which test will capture the error?" Doing it for real finds if there is a test that captures the problem. Sometimes not, in which case it's time to look at the requirements implicit in the tests, and see how we can tighten them up. In projects with no real requirements capture/analysis this can be a useful tool to toughen up tests so they fail when unexpected changes occur.

Of course, you have to be pragmatic. You can't reasonably expect to handle all changes - some will simply be absurd and the program will crash. But logical changes like the Owner change are good candidates for test strengthening.

By dragging talk of requirements into a simple coding fix, some may think I've gone off the deep end, but thorough requirements help produce thorough tests, and if you have no requirements, then you need to work doubly hard to make sure your tests are thorough, since you're implicitly doing requirements capture as you write the tests.

EDIT: I'm answering this from within the contraints set in the question. Given a free choice, I would suggest not using the EntityGenerator to create Product test instances, and instead create them "by hand" and use an equality comparison. Or more direct, compare the fields of the returned Product to specific (hard-coded) values in the test, again, without using the EntityGenerator in the test.

Answer 6

Why don't you mock the product as well as the productRepository?

If you mock the product using a strict mock, you will get a failure when the repository touches your product.

If this is a completely ridiculous idea, can you please explain why? Honestly, I'd like to learn.

Answer 7

You can return an interface to product instead of a concrete Product.

Such as

public IProduct GetProduct(int id) 
{ 
   return _productRepository.GetProduct(id);
}

And then verify the Owner property was not set:

Dep<IProduct>().AssertWasNotCalled(p => p.Owner = Arg.Is.Anything);

If you care about all the properties and or methods, then there is probably a pre-existing way with Rhino. Otherwise you can make an extension method that probably uses reflection such as:

Dep<IProduct>().AssertNoPropertyOrMethodWasCalled()

Our behaviour specifications are like so:

[Specification]
public class When_product_service_has_get_product_called_with_any_id 
       : ProductServiceSpecification
{
   private int _productId;

   private IProduct _actualProduct;

   [It] 
   public void Should_return_the_expected_product()
   {
     this._actualProduct.Should().Be.EqualTo(Dep<IProduct>());
   }

   [It]
   public void Should_not_have_the_product_modified()
   {
     Dep<IProduct>().AssertWasNotCalled(p => p.Owner = Arg<string>.Is.Anything);

     // or write your own extension method:
     // Dep<IProduct>().AssertNoPropertyOrMethodWasCalled();
   }


   public override void GivenThat()
   {
     var randomGenerator = new RandomGenerator();
     this._productId = randomGenerator.Generate<int>();

     Stub<IProductRepository, IProduct>(r => r.GetProduct(this._productId));
   }

   public override void WhenIRun()
   {
       this._actualProduct = Sut.GetProduct(this._productId);
   }
}

Enjoy.

Answer 8

If you really want to guarantee that the service method doesn't change the attributes of your products, you have two options:

• Define the expected product attributes in your test and assert that the resulting product matches these values. (This appears to be what you're doing now by cloning the object.)

• Mock the product and specify expectations to verify that the service method does not change its attributes.

This is how I'd do the latter with NMock:

// If you're not a purist, go ahead and verify all the attributes in a single
// test - Get_Product_Does_Not_Modify_The_Product_Returned_By_The_Repository
[Test]
public Get_Product_Does_Not_Modify_Owner() {

    Product mockProduct = mockery.NewMock<Product>(MockStyle.Transparent);

    Stub.On(_productRepositoryMock)
        .Method("GetProduct")
        .Will(Return.Value(mockProduct);

    Expect.Never
          .On(mockProduct)
          .SetProperty("Owner");

    _productService.GetProduct(0);

    mockery.VerifyAllExpectationsHaveBeenMet();
}

Answer 9

My previous answer stands, though it assumes the members of the Product class that you care about are public and virtual. This is not likely if the class is a POCO / DTO.

What you're looking for might be rephrased as a way to do comparison of the values (not instance) of the object.

One way to compare to see if they match when serialized. I did this recently for some code... Was replacing a long parameter list with a parameterized object. The code is crufty, I don't want to refactor it though as its going away soon anyhow. So I just do this serialization comparison as a quick way to see if they have the same value.

I wrote some utility functions... Assert2.IsSameValue(expected,actual) which functions like NUnit's Assert.AreEqual(), except it serializes via JSON before comparing. Likewise, It2.IsSameSerialized() can be used to describe parameters passed to mocked calls in a manner similar to Moq.It.Is().

public class Assert2
{
    public static void IsSameValue(object expectedValue, object actualValue) {

        JavaScriptSerializer serializer = new JavaScriptSerializer();

        var expectedJSON = serializer.Serialize(expectedValue);
        var actualJSON = serializer.Serialize(actualValue);

        Assert.AreEqual(expectedJSON, actualJSON);
    }
}

public static class It2
{
    public static T IsSameSerialized<T>(T expectedRecord) {

        JavaScriptSerializer serializer = new JavaScriptSerializer();

        string expectedJSON = serializer.Serialize(expectedRecord);

        return Match<T>.Create(delegate(T actual) {

            string actualJSON = serializer.Serialize(actual);

            return expectedJSON == actualJSON;
        });
    }
}

Answer 10

If all consumers of ProductService.GetProduct() expect the same result as if they had asked it from the ProductRepository, why don't they just call ProductRepository.GetProduct() itself ? It seems you have an unwanted Middle Man here.

There's not much value added to ProductService.GetProduct(). Dump it and have the client objects call ProductRepository.GetProduct() directly. Put the error handling and logging into ProductRepository.GetProduct() or the consumer code (possibly via AOP).

No more Middle Man, no more discrepancy problem, no more need to test for that discrepancy.

Answer 11

Let me state the problem as I see it.

1. You have a method and a test method. The test method validates the original method.
2. You change the system under test by altering the data. What you want to see is that the same unit test fails.

So in effect you're creating a test that verifies that the data in the data source matches the data in your fetched object AFTER the service layer returns it. That probably falls under the class of "integration test."

You don't have many good options in this case. Ultimately, you want to know that every property is the same as some passed-in property value. So you're forced to test each property independently. You could do this with reflection, but that won't work well for nested collections.

I think the real question is: why test your service model for the correctness of your data layer, and why write code in your service model just to break the test? Are you concerned that you or other users might set objects to invalid states in your service layer? In that case you should change your contract so that the Product.Owner is readonly.

You'd be better off writing a test against your data layer to ensure that it fetches data correctly, then use unit tests to check the business logic in your service layer. If you're interested in more details about this approach reply in the comments.

Answer 12

Having look on all 4 hints provided it seems that you want to make an object immutable at runtime. C# language does no support that. It is possible only with refactoring the Product class itself. For refactoring you can take IReadonlyProduct approach and protect all setters from being called. This however still allows modification of elements of containers like List<> being returned by getters. ReadOnly collection won't help either. Only WPF lets you change immutability at runtime with Freezable class.

So I see the only proper way to make sure objects have same contents is by comparing them. Probably the easiest way would be to add [Serializable] attribute to all involved entities and do the serialization-with-comparison as suggested by Frank Schwieterman.