0
\$\begingroup\$

Should I write unit tests which verify that constructor is throwing in simple cases when a nullptr was provided instead of a shared_ptr?

Let's say I have the following class:

#pragma once

#include "SharedItemsListInteractorInput.hpp"
#include "SharedItemsListInteractorOutput.hpp"
#include "Store.hpp"

namespace ri {

class ItemsListInteractor : public SharedItemsListInteractorInput {
private:
    std::shared_ptr<Store> _store;
    std::shared_ptr<SharedItemsListInteractorOutput> _output;
public:
    ItemsListInteractor(
        const std::shared_ptr<Store> &store,
        const std::shared_ptr<SharedItemsListInteractorOutput> &output
    );

    void fetchItems() override;
};

}

Implementation:

#include "ItemsListInteractor.hpp"
#include <iostream>

namespace ri {

ItemsListInteractor::ItemsListInteractor(
    const std::shared_ptr<Store> &store,
    const std::shared_ptr<SharedItemsListInteractorOutput> &output
) {
    if (store == nullptr) {
        throw std::invalid_argument("Store should not be an std::nullptr");
    }
    if (output == nullptr) {
        throw std::invalid_argument("Output should not be an std::nullptr");
    }
    _store = store;
    _output = output;
}

void ItemsListInteractor::fetchItems() {
    _store->getItems(0, [=] (std::vector<SharedItem> items, bool nextPage) {
        if (_output != nullptr) {
            _output->didFetchItems(items, nextPage);
        }
    });
}

}

Construction tests:

#include "ItemsListInteractor.hpp"
#include "catch.hpp"
#include "fakeit.hpp"

using namespace Catch;
using namespace fakeit;
using namespace ri;

TEST_CASE( "items list interactor", "[ItemsListInteractor]" ) {

    SECTION( "no store" ) {
        Mock<SharedItemsListInteractorOutput> outputMock;
        Fake(Dtor(outputMock));
        auto output = std::shared_ptr<SharedItemsListInteractorOutput>(&outputMock.get());

        REQUIRE_THROWS_AS(ItemsListInteractor(nullptr, output), std::invalid_argument);
    }
    SECTION( "no output" ) {
        Mock<Store> storeMock;
        Fake(Dtor(storeMock));
        auto store = std::shared_ptr<Store>(&storeMock.get());

        REQUIRE_THROWS_AS(ItemsListInteractor(store, nullptr), std::invalid_argument);
    }
}

Feels like writing constructor tests, in this case, brings too much boilerplate code.

\$\endgroup\$
0

1 Answer 1

Reset to default
4
\$\begingroup\$

Missing includes

You are missing at least:

  • in the header:

      #include <memory>

  • in the implementation:

      #include <exception>

Extraneous includes

The implementation includes <iostream> but never uses it. Prefer <iosfwd> over <iostream> where possible, and prefer nothing over <iosfwd> where possible.

Prefer initialization to assignment

Instead of

_store = store;
_output = output;

it's better to use initializers - especially if you set your compiler to warn when you forget to initialize (recommended):

ItemsListInteractor::ItemsListInteractor(
    std::shared_ptr<Store> store,
    std::shared_ptr<SharedItemsListInteractorOutput> output)
  : store{std::move(store)},
    output{std::move(output)}
 {
    if (!this->store)
        throw std::invalid_argument("Store should not be null");
    if (!this->output)
        throw std::invalid_argument("Output should not be null");
}

I made other improvements in the above:

  • pass by value, then std::move() in preference to passing by const ref
  • name the members the same as the arguments to prevent accidental use after move from (and to be less ugly - a leading underscore shouts "DANGER", especially to those who also write C).
  • use the (more idiomatic) operator bool to test the pointers' validity - this is easier to understand than comparing against a shared pointer implicitly constructed from std::nullptr_t.
  • I changed the message - an empty shared pointer is a different type to the null pointer.

Don't repeat the test in the method

There's no way that output can be assigned to, other than via the compiler-provide copy constructor and assignment operator, both of which require a valid ItemsListInteractor. A valid ItemsListInteractor cannot have a null output if its constructor succeeded.

void ItemsListInteractor::fetchItems() {
    store->getItems(0, [=](std::vector<SharedItem> items, bool nextPage) {
        output->didFetchItems(items, nextPage);
    });
}

Tests

The tests look adequate, as far as they go. The fetchItems() method should also be tested.

\$\endgroup\$
10
  • \$\begingroup\$ @TobySpeight thank you. I am actually compiling my project without those inclusions. Your constructor updates are valuable for me. Question about "Don't repeat the test": what if shared_ptr becomes null over time? I am using a complex cross-platform environment and it happens a lot. It actually means that shared_ptr behaves more like a weak_ptr, but I can't control that and can't use weak_ptr.fetchItems() is tested in another file. \$\endgroup\$ Commented Nov 21, 2017 at 17:23
  • 1
    \$\begingroup\$ Then you've broken the implicit contract set in your constructor, @asdf. Check your (implicit) specification whether the shared_ptr might become null over time? \$\endgroup\$ Commented Nov 21, 2017 at 17:25
  • 1
    \$\begingroup\$ In my version the ref-count is increased slightly earlier (when the constructor is called, rather than when the member is assigned within the constructor). That does mean that if the constructor throws, there will be slightly more work (the refcount will be decremented again as the construction is unwound) - but that's pretty insignificant against the cost of throwing. None of this changes the semantics at all - the object still owns copies of the shared pointers. \$\endgroup\$ Commented Nov 22, 2017 at 11:24
  • 1
    \$\begingroup\$ There's not much context in the linked answer, but it seems to only address the case where the shared pointer is merely used and not copied, by the function. Here, we copy into a member, and it's usually more appropriate to pass by value in this case (as we know that a copy will be taken) and std::move() to the member. This allows the caller to std::move() from an lvalue, or to pass an rvalue, more efficiently. \$\endgroup\$ Commented Nov 22, 2017 at 12:11
  • 1
    \$\begingroup\$ I usually use GCC. Don't rely on compilers to diagnose missing headers - they are allowed to transitively include more than the standard requires, so absence of a warning does not indicate absence of a problem. \$\endgroup\$ Commented Oct 10, 2018 at 17:25

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.