4

I am focusing on writing easy to test code these days and I realize that most of my classes follow the skeleton below:

Some class ObjectManager needs a DatabaseRepo, while there is only one type DatabaseRepo in production (OracleRepo), I am still implementing it against an interface because I want to be able to mock out the DatabaseRepo, thus I need to make use of polymorphism. I inject the DatabaseRepo as a unique_ptr to the ObjectManager's constructor, in order to indicate that ObjectManager owns the DatabaseRepo.

One thing, for example, which is flawed here is that in the unit test, I access the state of the MockRepo after I moved it into the ObjectManager. While its is safe (ObjectManager) is still alive it looks like a hack.


Question: Using this as a general pattern for the situation where class A has an object of type B, are there any design aspects I could improve?


class ObjectManager {
  public:
     ObjectManager(std::unique_ptr<DatabaseRepo> initRepo) : repo(initRepo) {

     }

     void process() {
       repo->updateRecords();
     }


  private:
    std::unique_ptr<DatabaseRepo> repo;
};


class DatabaseRepo {
  public:
    virtual void updateRecords = 0;
} 

class OracleRepo : public DatabaseRepo {
  public:
    void updateRecords() overrides {

    }
}

class MockRepo : public DatabaseRepo {
  public:
    void updateRecords() overrides {

    }
}

testObjectManager() {
   std::unique_ptr<DatabaeRepo> mockrepo = 
       std::unique_ptr<DatabseRepo>(new MockRepo);

   ObjectManager om(std::move(mockrepo)); 
   om.process();

   EXPECT_CALLED_ONCE(mockrepo.get()->updateRecords());
}
3

You know that the pointer is going to remain valid until ObjectManager is destroyed, but you want to avoid deferencing a moved-from unique_ptr. In this case it should be OK to keep the original pointer and use that:

testObjectManager() {
   DatabaseRepo* mockrepo = new MockRepo;

   ObjectManager om(std::unique_ptr<DatabaseRepo>(mockrepo)); 
   om.process();

   EXPECT_CALLED_ONCE(mockrepo->updateRecords());
}
1

Why do you pass it as an std::unique_ptr<> ?

I would use a non-const reference instead.

One thing, for example, which is flawed here is that in the unit test, I access the state of the MockRepo after I moved it into the ObjectManager.

You will be ensured that the mock lives as long as you need it to, as you have full control over the life-time yourself.

Your code will even be simpler, as there is no need to use new:

testObjectManager() {
   DatabaseRepo mockrepo;

   ObjectManager om(mockrepo);
   om.process();

   EXPECT_CALLED_ONCE(mockrepo.updateRecords());
}
  • I pass a std::unique_ptr because the ObjectManager manages the DatabaseRepo's lifetime. If I pass a DatabaseRepo reference, then the lifetime of DatabaseRepo can be shorter than that of ObjectManager. – user695652 Mar 18 '17 at 17:18
  • 1
    @user695652 But is there a strict reason why it has to manage the DatabaseRepo's lifetime? Those objects are created once, somewhere in main() or another high level; It's not hard to ensure proper lifetime in that case, as using local variables - on the stack - will work correctly. IMHO, you're creating more problems with this lifetime management of yours than you're solving. – Sjoerd Mar 19 '17 at 0:12
  • "But is there a strict reason why it has to manage the DatabaseRepo's lifetime?" Yes because only the ObjectManager knows how long it exists. Note that the ObjectManager can be constructed anywhere, for example in a factory function... There is a notion of ownership semantics and using a unique_ptr is a clear way of indicating that the creator of DatabaseRepo does not need to care about it is lifetime. – user695652 Mar 19 '17 at 0:40
  • @user695652 The one constructing an ObjectManager knows how long that ObjectManager exists, and can create a DatabaseRepo with at least the same lifetime. Problem solved. (If you really want, you could even create a wrapper struct with two members - the order of constructing and destructing members will take care of the proper lifetime management. But in practice, this is barely ever needed). – Sjoerd Mar 19 '17 at 1:19
0

One possible solution would be to create mocks as unique_ptrs, then set up the expectations, then create the class under test (thus invalidating the unique_ptrs). So the mocks can be members of the fixture, but the class under test cannot be.

This assumes that the expectations are set up ahead of time rather than checked after the fact. I use gmock, so the assumption holds. Your mocking system may be different.

0

You can take action in the destructor of MockRepo to inform other parts of the test code that the mock has been destroyed.

class MockRepo : public DatabaseRepo {
    MockRepo *& signal;
  public:
    MockRepo(MockRepo *& signal) : signal(signal) { signal = this; }
    ~MockRepo() { signal = nullptr; }
    void updateRecords() override {

    }
}

testObjectManager() {
   MockRepo * mock; 

   ObjectManager om(std::make_unique<MockRepo>(mock)); 
   om.process();

   EXPECT_NOT_NULL(mock);
   EXPECT_CALLED_ONCE(mock->updateRecords());
}

Note that your program has undefined behaviour, as DatabaseRepo lacks a virtual destructor.

class DatabaseRepo {
  public:
    DatabaseRepo(const DatabaseRepo &) = delete;
    DatabaseRepo& operator=(const DatabaseRepo &) = delete;
    DatabaseRepo(DatabaseRepo &&) = delete;
    DatabaseRepo& operator=(DatabaseRepo &&) = delete;
    virtual ~DatabaseRepo() = default;
    virtual void updateRecords() = 0;
} 

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.