Testing legacy code by adding singletons

This is not a good idea: Michael Feathers says "STOP IT NOW"

Testing legacy code

Many people have read Mike Feather's excellent book, "Working effectively with legacy code." including people on my team. Some people like Mocks. Watch this space - Overload 127 will contain an article asking if mocks are always the right thing to use.

My team has lots of legacy code, that is code without tests. We want to get it under test and I want these tests to run on our Jenkins box. I want any quick running tests to run on each checkin and email us if the build got broken and whoever broke it fix it. A girl can dream.

Stop it - you're doing it wrong

We seem to be developing a "pattern" whereby we introduce singletons in order to make our code testable. Yes, I just said introduce singletons in order to make the code testable.
I think this is happening because "we" (well, they) want to use gmock because it's brilliant. I could be wrong. Perhaps it doesn't matter why it is happening we just need to stop this and do something different.

Why does gmock make you write singletons?

Let's look at an example, with the names changed to protect the guilty.
Suppose you have some code like this (C++).

class Asset
{
    //miles and miles of public functions and comments
    double Value(std::string logMessage, double someIrrelevantNumberToLog);
  

};

double Asset::Value(std::string logMessage, double someIrrelevantNumberToLog)
{
    ENTERPRISE_INHOUSE_LOG_FRAMEWORK_THAT_PULLS_IN_THE_WORLD(info, logMessage, someIrrelevantNumberToLog);
    double value = 0.0;
    if (isSpot)
        value = spotValue(m_notional, m_exchangeRate);
    else
        value = futureValue(m_notional, m_exchangeRate);
    return value;
}

spotValue and futureValue are C functions that may or may not call COBOL or FORTRAN or similar.

We have ended up with some tests. Yay! Which use singletons. Boo!!
(Hope you like the comment being in red - as a warning rather than the odd convention of making them green in many IDEs).

No, but, HOW?

In order to test this, and armed with gmock we have something like mockSpotValue.h (namespaces and include guards left as an exercise for the reader for brevity)


#include <gmock>

class MockSpotValue 
{
    public:
    MOCK_CONST_METHOD(spotValue, double(double, double));

  void rest() 
  {
      Mock::VerifyAndClear(this);
  }
};

/**
 * Singleton
 * /
MockSpotValue & mockSpotValue();

Let's not point out this isn't a singleton. I'll leave the "mockSpotValue" instance create factory builder method as an exercise for the reader too. Making comments in red reminds me of being a teacher. It's the future. Or spot on. Depending on a boolean.

Now we use a linker seam to make our very own spotValue we can call in a test on a dev box.

double spotValue(double x, double y)
{
    mockSpotValue().(x,y);
}

And where's the test(s)?

Ah. Tests. Yes, having done this we should write some. Or maybe just one for brevity.

TEST_F(ValueTest, testGetSpotValueWithZeroNotional)
{
  MockSpotValue & valueApi = mockSpotValue();
  valueApi,reset();
  Asset asset;
  asset.makeSpot();//or something mad like that
  EXPECT_CALL(valueApi, spotValue(_, _)).WillOnce(Return(42));

  EXPECT_THAT(asset.Value(), DoubleEq(42.0));

}
I have simplified this. In order to get something like this Asset into a test we did some things with a sprout. This may require another blog post.

BUT that's a B(ad) U(nit) T(est)

How do we know this is a bad test? Because we have seen the singleton? Even without that the name smells. "testGetSpotValueWithZeroNotional" 
I have seen worse. I saw one call "testDefaultIsValid" which asserted that a thing constructed with defaults IS NOT valid. I digress.
So, testGetSpotValueWithZeroNotional. What are we testing? Can we make this test name clearly express what is tests?

The best I can come up with is testThatSpotAssetValueReturnsTheValueIToldTheMockToReturn or more simply
testThatGMockDoesWhatItIsSupposedToCosYouCannotTrustThesePeople

Help

I like that we are trying to get tests round legacy code. I just have a few qualms about how we are doing this. Please comment with suggestions on how to test this better.
I feel like banning mocks until we have written a few characterisation tests. At least there will be fewer singletons that way. Who ever heard of adding singletons in order to test code?