22

I've been struggling with an increasingly annoying problem regarding our unit tests that we are implementing in my team. We are attempting to add unit tests into legacy code that wasn't well designed and while we haven't had any difficulty with the actual addition of the tests we are starting to struggle with how the tests are turning out.

As an example of the problem let's say you have a method that calls 5 other methods as part of its execution. A test for this method might be to confirm that a behavior occurs as a result of one of these 5 other methods being called. So, because a unit test should fail for one reason and one reason only, you want to eliminate potential issues caused by calling these other 4 methods and mock them out. Great! The unit test executes, the mocked methods are ignored (and their behavior can be confirmed as part of other unit tests), and the verification works.

But there's a new problem - the unit test has intimate knowledge of how you confirmed that behavior and any signature changes to any of those other 4 methods in the future, or any new methods that need to be added to the 'parent method', will result in having to change the unit test to avoid possible failures.

Naturally the problem could be mitigated somewhat by simply having more methods accomplish less behaviors but I was hoping there was perhaps a more elegant solution available.

Here's an example unit test that captures the problem.

As a quick note 'MergeTests' is a unit testing class that inherits from the class we are testing and overrides behavior as needed. This is a 'pattern' we employ in our tests to allow us to override calls to external classes / dependencies. 

[TestMethod]
public void VerifyMergeStopsSpinner()
{
    var mockViewModel = new Mock<MergeTests> { CallBase = true };
    var mockMergeInfo = new MergeInfo(Mock.Of<IClaim>(), Mock.Of<IClaim>(), It.IsAny<bool>());

    mockViewModel.Setup(m => m.ClaimView).Returns(Mock.Of<IClaimView>);
    mockViewModel.Setup(
        m =>
        m.TryMergeClaims(It.IsAny<Func<bool>>(), It.IsAny<IClaim>(), It.IsAny<IClaim>(), It.IsAny<bool>(),
                         It.IsAny<bool>()));
    mockViewModel.Setup(m => m.GetSourceClaimAndTargetClaimByMergeState(It.IsAny<MergeState>())).Returns(mockMergeInfo);
    mockViewModel.Setup(m => m.SwitchToOverviewTab());
    mockViewModel.Setup(m => m.IncrementSaveRequiredNotification());
    mockViewModel.Setup(m => m.OnValidateAndSaveAll(It.IsAny<object>()));
    mockViewModel.Setup(m => m.ProcessPendingActions(It.IsAny<string>()));

    mockViewModel.Object.OnMerge(It.IsAny<MergeState>());    

    mockViewModel.Verify(mvm => mvm.StopSpinner(), Times.Once());
}

How have the rest of you dealt with this or is there no great 'simple' way of handling it?

Update - I appreciate everyone's feedback. Unfortunately, and it's no surprise really, there doesn't seem to be a great solution, pattern, or practice one can follow in unit testing if the code being tested is poor. I marked the answer that best captured this simple truth.

Stephane Rolland
  • 596
  • 1
  • 6
  • 17
PremiumTier
  • 479
  • 1
  • 5
  • 15
  • Wow, I only see mock setup, no SUT instantiation or anything, are you testing any actual implementation in here? Who is supposed to call StopSpinner? OnMerge? You should mock any dependencies it may call out to but not the thing itself.. – Joppe Mar 08 '13 at 23:07
  • It's a bit hard to see, but the Mock is the SUT. We set the CallBase flag to ensure the 'OnMerge' method executes on the actual object, but mock out methods called by 'OnMerge' that would could cause the test to fail due to dependency issues etc. The goal of the test is the last line - to verify we stopped the spinner in this case. – PremiumTier Mar 09 '13 at 00:15
  • MergeTests sounds like another instrumented class, not something that lives in production hence the confusion. – Joppe Mar 09 '13 at 00:19
  • Understood. I have highlighted some text in the question that explains what we did with MergeTests as well as added some clarification. – PremiumTier Mar 09 '13 at 00:24
  • 3
    [The problem with mocking is that sometimes the code wins.](http://dilbert.com/fast/2013-02-24/) – Mason Wheeler Mar 09 '13 at 01:07
  • If I had a choice? I would stop. – Erik Reppen Mar 09 '13 at 02:27
  • 1
    Completely aside from your other issues, it seems wrong to me that your SUT is a Mock. Why would you test a Mock? Why aren't you testing the MergeTests class itself? – Eric King Mar 09 '13 at 05:02
  • @EricKing By testing the mocked version of the class we allow ourselves to mock out methods of the class that can't execute as part of an individual test. Even aside from the adherence to the "fail for one reason and one reason only rule" we do this because those methods would need to be re-factored to remove dependencies before they can execute as part of the test. – PremiumTier Mar 09 '13 at 17:45

3 Answers3

23

  1. Fix the code to be better designed. If your tests have these issues, then your code will have worse issues when you try to change things.

  2. If you can't, then perhaps you need to be less ideal. Test against the pre and post-conditions of the method. Who cares if you're using the other 5 methods? They presumably have their own unit tests making it clear(er) what caused the failure when the tests fail.

"unit tests should have only one reason to fail" is a good guideline, but in my experience, impractical. Hard to write tests don't get written. Fragile tests don't get believed.

Telastyn
  • 108,850
  • 29
  • 239
  • 365
  • I agree completely with fixing the design of the code but in the less ideal world of developing for a large company with tight timelines it can be hard to make the case for 'paying off' the technical debt incurred by past teams or poor decisions all at once. To your second point much of the mocking isn't just because we want the test to only fail for one reason - it's because the code being executed can't be permitted to execute without also first handling a large number of dependencies created inside that code. Sorry for moving the goal posts on that one. – PremiumTier Mar 08 '13 at 19:46
  • If a better design is not realistic I agree with 'Who cares if you're using the other 5 methods?' Verify the method performs the required function, not how it's doing that. – Kwebble Mar 08 '13 at 23:01
  • @Kwebble - Understood, however the goal of the question was to determine if there was a simple way to verify behavior for a method when you also have to mock out other behaviors called within the method in order to run the test at all. I want to remove the 'how', but I don't know how :) – PremiumTier Mar 09 '13 at 00:28
  • There's no magic silver bullet. There's no "simple way" to test poor code. Either the code-under-test needs to be refactored, or the test code, itself, will also be poor. Either the test will be poor because it will be too specific to the internal details, as you have run into, or as *btilly* suggested, you can run the tests against a working environment, but then the tests will be much slower and more complex. Either way, the tests will be harder to write, harder to maintain, and prone to false-negatives. – Steven Doggart Mar 11 '13 at 13:32
10

Breaking up large methods into more focused small methods is definitely a best practice. You see it as pain in verifying unit test behavior, but you're experiencing the pain in other ways as well.

That said, it is a heresy but I am personally a fan of creating realistic temporary test environments. That is, rather than mocking out everything that is hidden inside of those other methods, make sure there is an easy to set up temporary environment (complete with private databases and schemas - SQLite may help here) which lets you run all of that stuff. The responsibility for knowing how to build/tear down that test environment lives with the code that requires that, so that when it changes, you don't have to change all of the unit test code that depended on its existence.

But I do note that this is a heresy on my part. People who are heavily into unit testing advocate "pure" unit tests and call what I described "integration tests". I don't personally worry about that distinction.

btilly
  • 18,250
  • 1
  • 49
  • 75
4

I would consider easing on the mocks and just formulate tests that might include the methods it calls out to.

Don't test the how, test the what. It's the result that matters, include the sub-methods if need be.

From another angle you could formulate a test, make it pass with one big method, refactor and end up with a tree of methods after refactoring. You don't need to test each and every one of them in isolation. It's the end result that counts.

If the sub methods make it hard to test some aspects consider breaking them out to separate classes so you can mock them there more cleanly without your class under test being heavily instrumented/seamed. It's kind of hard to tell if you are actually testing any concrete implementation in your example test.

Joppe
  • 4,548
  • 17
  • 25
  • The problem is we have to mock the 'how' to test the 'what'. It is a limitation imposed by the code's design. I certainly have no wish to 'mock' the how as that is what makes the test brittle. – PremiumTier Mar 09 '13 at 00:17
  • Looking at the method names I think your tested class is simply taking on too many responsibilities. Read up on single responsibility principle. Borrowing from MVC may help a bit, your class seems to handle both UI, infrastructure and business concerns. – Joppe Mar 09 '13 at 11:14
  • 1
    Yeah :( That would be that poorly-designed legacy code I was referring to. We are working on the re-design and refactor but we felt it would be best to put the source under test first. – PremiumTier Mar 09 '13 at 17:41