Use of Mocks in Tests_问答_开发者_运维开发者技术经验分享

I just started using mock objects (using Java's mockito) in my tests recently. Needless to say, they simplified the set-up part of the tests, and along with Dependency Injection, I would argue it made the code even more robust.

However, I have found myself tripping in testing against implementation rather than specification. I ended up setting up expectations that I would argue that it's not part of the tests. In开发者_Go百科 more technical terms, I will be testing the interaction between SUT (the class under test) and its collaborators, and such dependency isn't part of contract or the interface of the class!

Consider that you have the following: When dealing with XML node, suppose that you have a method, attributeWithDefault() that returns the attribute value of the node if it's available, otherwise it would return a default value!

I would setup the test like the following:

Element e = mock(Element.class);

when(e.getAttribute("attribute")).thenReturn("what");
when(e.getAttribute("other")).thenReturn(null);

assertEquals(attributeWithDefault(e, "attribute", "default"), "what");
assertEquals(attributeWithDefault(e, "other", "default"), "default");

Well, here not only did I test that attributeWithDefault() adheres to the specification, but I also tested the implementation, as I required it to use Element.getAttribute(), instead of Element.getAttributeNode().getValue() or Element.getAttributes().getNamedItem().getNodeValue(), etc.

I assume that I am going about it in the wrong way, so any tips on how I can improve my usage of mocks and best practices will be appreciated.

EDIT: What's wrong with the test

I made the assumption above that the test is a bad style, here is my rationale.

The specification doesn't specify which method gets called. A client of the library shouldn't care of how attribute is retrieved for example, as long as it is done rightly. The implementor should have free reign to access any of the alternative approaches, in any way he sees fit (with respect to performance, consistency, etc). It's the specification of Element that ensures that all these approaches return identical values.
It doesn't make sense to re-factor Element into a single method interface with getElement() (Go is quite nice about this actually). For ease of use, a client of the method should be able to just to use the standard Element in the standard library. Having interfaces and new classes is just plain silly, IMHO, as it makes the client code ugly, and it's not worth it.
Assuming the spec stays as is and the test stays as is, a new developer may decide to refactor the code to use a different approach of using the state, and cause the test to fail! Well, a test failing when the actual implementation adheres to the specification is valid.
Having a collaborator expose state in multiple format is quite common. A specification and the test shouldn't depend on which particular approach is taken; only the implementation should!

This is a common issue in mock testing, and the general mantra to get away from this is:

Only mock types you own.

Here if you want to mock collaboration with an XML parser (not necessarily needed, honestly, as a small test XML should work just fine in a unit context) then that XML parser should be behind an interface or class that you own that will deal with the messy details of which method on the third party API you need to call. The main point is that it has a method that gets an attribute from an element. Mock that method. This separates implementation from design. The real implementation would have a real unit test that actually tests you get a successful element from a real object.

Mocks can be a nice way of saving boilerplate setup code (acting essentially as Stubs), but that isn't their core purpose in terms of driving design. Mocks are testing behavior (as opposed to state) and are not Stubs.

I should add that when you use Mocks as stubs, they look like your code. Any stub has to make assumptions about how you are going to call it that are tied to your implementation. That is normal. Where it is a problem is if that is driving your design in bad ways.

When designing unit tests you will always effectively test your implementation, and not some abstract specification. Or one can argue that you will test the "technical specification", which is the business specification extended with technical details. There is nothing wrong with this. Instead of testing that:

My method will return a value if defined or a default.

you are testing:

My method will return a value if defined or a default provided that the xml Element supplied will return this attribute when I call getAttribute(name).

The only solution I can see for you here (and I have to admit I'm not familiar with the library you're using) is to create a mock element that has all of the functionality included, that is, also have the ability to set the value of getAttributeNote().getValue() and getAttributes().getNamedItem().getNodeValue().

But, assuming they're all equivalent, it's fine to just test one. It's when it varies that you need to test all cases.

I don't find anything wrong with your use of the mocks. What you are testing is the attributeWithDefault() method and it's implementation, not whether Element is correct or not. So you mocked Element in order to reduce the amount of setup required. The test ensures that the implementation of attributeWithDefault() fits the specification, naturally there needs to be some specific implementation that can be run for the test.

You're effectively testing your mock object here. If you want to test the attributeWithDefault() method, you must assert that e.getAttribute() gets called with the expected argument and forget about the return value. This return value only verifies the setup of your mock object. (I don't know how this is exactly done with Java's mockito, I'm a pure C# guy...)

It depends on whether getting the attribute via calling getAttribute() is part of the specification, or if it is an implementation detail that might change.

If Element is an interface, than stating that you should use 'getAttribute' to get the attribute is probably part of the interface. So your test is fine.

If Element is a concrete class, but attributeWithDefault should not be aware of how you can get the attribute, than maybe there is a interface waiting to appear here.

public interface AttributeProvider {
   // Might return null
   public String getAttribute(String name); 
}

public class Element implements AttributeProvider {
   public String getAttribute(String name) {
      return getAttributeHolder().doSomethingReallyTricky().toString();
   }
}

public class Whatever {
  public String attributeWithDefault(AttributeProvider p, String name, String default) {
     String res = p.getAtribute(name);
     if (res == null) {
       return default;
     }
   }
}

You would then test attributeWithDefault against a Mock AttributeProvider instead of an Element.

Of course in this situation it would probably be an overkill, and your test is probably just fine even with an implementation (You will have to test it somewhere anyway ;) ). However this kind of decoupling might be usefull if the logic ever goes any more complicated, either in getAttribute or in attributeWithDefualt.

Hoping this helps.

It seems to me that there are 3 things you want to verify with this method:

It gets the attribute from the right place (Element.getAttribute())
If the attribute is not null, it is returned
If the attribute is null, the string "default" is returned

You're currently verifying #2 and #3, but not #1. With mockito, you could verify #1 by adding

verify(e.getAttribute("attribute"));
verify(e.getAttribute("other"));

Which ensures that the methods are actually getting called on your mock. Admittedly this is a little clunky in mockito. In easymock, you'd do something like:

expect(e.getAttribute("attribute")).andReturn("what");
expect(e.getAttribute("default")).andReturn(null);

It has the same effect, but I think makes your test a bit easier to read.

If you are using dependency injection then the collaborators should be part of the contract. You need to be able to inject all collaborators in through the constructor or a public property.

Bottom line: if you have a collaborator that you newing up instead of injecting then you probably need to refactor the code. This is a change of mindset necessary for testing/mocking/injecting.

This is a late answer, but it takes a different viewpoint from the other ones.

Basically, the OP is right in thinking the test with mocking is bad, for the reasons he stated in the question. Those saying that mocks are ok have not provided good reasons for it, IMO.

Here is a complete version of the test, in two versions: one with mocking (the BAD one) and another without (the GOOD one). (I took the liberty of using a different mocking library, but that doesn't change the point.)

import javax.xml.parsers.*;
import org.w3c.dom.*;
import org.junit.*;
import static org.junit.Assert.*;
import mockit.*;

public final class XmlTest
{
    // The code under test, embedded here for convenience.
    public static final class XmlReader
    {
        public String attributeWithDefault(
            Element xmlElement, String attributeName, String defaultValue
        ) {
            String attributeValue = xmlElement.getAttribute(attributeName);
            return attributeValue == null || attributeValue.isEmpty() ?
                defaultValue : attributeValue;
        }
    }

    @Tested XmlReader xmlReader;

    // This test is bad because:
    // 1) it depends on HOW the method under test is implemented
    //    (specifically, that it calls Element#getAttribute and not some other method
    //     such as Element#getAttributeNode) - it's therefore refactoring-UNSAFE;
    // 2) it depends on the use of a mocking API, always a complex beast which takes
    //    time to master;
    // 3) use of mocking can easily end up in mock behavior that is not real, as
    //    actually occurred here (specifically, the test records Element#getAttribute
    //    as returning null, which it would never return according to its API
    //    documentation - instead, an empty string would be returned).
    @Test
    public void readAttributeWithDefault_BAD_version(@Mocked final Element e) {
        new Expectations() {{
            e.getAttribute("attribute"); result = "what";

            // This is a bug in the test (and in the CUT), since Element#getAttribute
            // never returns null for real.
            e.getAttribute("other"); result = null;
        }};

        String actualValue  = xmlReader.attributeWithDefault(e, "attribute", "default");
        String defaultValue = xmlReader.attributeWithDefault(e, "other", "default");

        assertEquals(actualValue,  "what");
        assertEquals(defaultValue, "default");
    }

    // This test is better because:
    // 1) it does not depend on how the method under test is implemented, being
    //    refactoring-SAFE;
    // 2) it does not require mastery of a mocking API and its inevitable intricacies;
    // 3) it depends only on reusable test code which is fully under the control of the
    //    developer(s).
    @Test
    public void readAttributeWithDefault_GOOD_version() {
        Element e = getXmlElementWithAttribute("what");

        String actualValue  = xmlReader.attributeWithDefault(e, "attribute", "default");
        String defaultValue = xmlReader.attributeWithDefault(e, "other", "default");

        assertEquals(actualValue,  "what");
        assertEquals(defaultValue, "default");
    }

    // Creates a suitable XML document, or reads one from an XML file/string;
    // either way, in practice this code would be reused in several tests.
    Element getXmlElementWithAttribute(String attributeValue) {
        DocumentBuilder dom;
        try { dom = DocumentBuilderFactory.newInstance().newDocumentBuilder(); }
        catch (ParserConfigurationException e) { throw new RuntimeException(e); }
        Element e = dom.newDocument().createElement("tag");
        e.setAttribute("attribute", attributeValue);
        return e;
    }
}