Sustainable C++ Testing

Riddle #7: Strategic

We have a TradingStrategy, which main goal is to price assets based on an externally provided factor. Upon start, the strategy registers itself with FactorStrategy object, from which it will receive factor updates.

#include <cmath>
#include <gtest/gtest.h>

class FactorStrategy {
public:
  class FactorListener {
  public:
    virtual ~FactorListener() = default;
    virtual void onFactorUpdate(double newFactor) = 0;
  };
  void registerFactorListener(FactorListener* /*listener*/) {}
};

class TradingStrategy : public FactorStrategy::FactorListener {
public:
  void start(FactorStrategy& factorStrategy) {
    factorStrategy.registerFactorListener(this);
    started = true;
  }
  
  long calculateStrategyPrice(long assetPrice) {
    assert(started);
    return static_cast<long>(assetPrice * factor);
  }
  
  // received through the FactorStrategy
  void onFactorUpdate(double newFactor) override {
    factor = newFactor;
  }
  
  bool started { false };
  double factor { 1.0 };
};

TEST(TradingStrategyTest, calculatePrice_beforeFirstFactorUpdate) {
  TradingStrategy strategy;
  strategy.started = true;
  
  EXPECT_EQ(1234, strategy.calculateStrategyPrice(1234));
}

#include <cmath>
#include <gtest/gtest.h>

class FactorStrategy {
public:
  class FactorListener {
  public:
    virtual ~FactorListener() = default;
    virtual void onFactorUpdate(double newFactor) = 0;
  };
  void registerFactorListener(FactorListener* /*listener*/) {}
};

class TradingStrategy : public FactorStrategy::FactorListener {
public:
  void start(FactorStrategy& factorStrategy) {
    factor = NAN;
    factorStrategy.registerFactorListener(this);
    started = true;
  }
  
  long calculateStrategyPrice(long assetPrice) {
    assert(started);
    return static_cast<long>(assetPrice * factor);
  }
  
  // received through the FactorStrategy
  void onFactorUpdate(double newFactor) override {
    factor = newFactor;
  }
  
  bool started { false };
  double factor { 1.0 };
};

TEST(TradingStrategyTest, calculatePrice_beforeFirstFactorUpdate) {
  TradingStrategy strategy;
  strategy.started = true;
  
  // FAIL! In reality, calculateStrategyPrice would return NAN in this scenario
  EXPECT_EQ(1234, strategy.calculateStrategyPrice(1234));
}

#include <cmath>
#include <gtest/gtest.h>
#include <gmock/gmock.h>

class FactorStrategy {
public:
  class FactorListener {
  public:
    virtual ~FactorListener() = default;
    virtual void onFactorUpdate(double newFactor) = 0;
  };
  void registerFactorListener(FactorListener* /*listener*/) {}
};

class TradingStrategy : public FactorStrategy::FactorListener {
public:
  void start(FactorStrategy& factorStrategy) {
    factorStrategy.registerFactorListener(this);
    started = true;
  }
  
  long calculateStrategyPrice(long assetPrice) {
    assert(started);
    return static_cast<long>(assetPrice * factor);
  }
  
  // received through the FactorStrategy
  void onFactorUpdate(double newFactor) override {
    factor = newFactor;
  }

private:
  bool started { false };
  double factor { 1.0 };
};

class MockFactorStrategy : public FactorStrategy {
public:
  MOCK_METHOD1(registerFactorListener, void(FactorListener*));
};

TEST(TradingStrategyTest, calculatePrice_beforeFirstFactorUpdate) {
  testing::NiceMock<MockFactorStrategy> mockFactorStrategy;

  TradingStrategy strategy;
  strategy.start(mockFactorStrategy);
  
  EXPECT_EQ(1234, strategy.calculateStrategyPrice(1234));
}

Can you spot a weakness in this setup?

Give me a hint

All members of the strategy are public.

Give me a hint

The test takes a shortcut and directly sets started = true, instead of calling start(). Presumably, because the latter would require making a test instance of FactorStrategy available, which maybe test author wanted to avoid. What kind of trouble can the test get into because of such setup? What kind of changes it is sensitive to?

Give me a hint

What if start() changed? Imagine the strategy was elaborated and now supports restarts and as part of that feature, start() was changed to reset the factor. Show me

Reveal the answer

Diagnosis

By circumventing the official start() method, the test takes on responsibility of performing all relevant steps that start() would otherwise do. Worse yet, when start() changes, all tests need to be reconsidered and adapted. If that is not done, the tests may suddenly be testing unrealistic scenarios and not fulfilling on their promises.
Violations of class encapsulation have similar risks in tests as they do in production code.

Reveal the remedy

Remedy

Encapsulation needs to be respected, not only within production code (strategy should have the data members private), but also between production and test code (e.g. tests should not befriend the unit they are testing).
Show me
Now, if the start() was changed to reset the factor to NAN, the test would correctly start failing:

[ RUN ] TradingStrategyTest.calculatePrice_beforeFirstFactorUpdate test.cpp:49: Failure Expected equality of these values: 1234 strategy.calculateStrategyPrice(1234) Which is: -9223372036854775808 [ FAILED ] TradingStrategyTest.calculatePrice_beforeFirstFactorUpdate (1 ms)

Feedback on this riddle? 3c6120687265663d226d61696c746f3a666565646261636b407375737461696e61626c6563707074657374696e672e636f6d3f7375626a6563743d5375737461696e61626c6520432b2b2054657374696e672c20726964646c65233720666565646261636b223e446f2067657420696e20746f756368213c2f613e