6
\$\begingroup\$

I would like a review on just a single class today. More important to me than a review on the class itself, however, is a review of the unit-tests I wrote for it. You see this is my first attempt at writing unit tests with a full test framework.

The class in question is called UpgradeButton and is exactly that. It is a button for representing upgrades in my resource management game. The buttons look like this:

picture of button, with green and red lights and text 'Oil Production'

The boxes along the bottom are updated programatically through the input() and update() functions that indicate what level you are at and what you can purchase. The number of tiers is assigned in the constructor, as is the label.

UpgradeButton.h

#pragma once

#include "Expressions.h"

#include <SFML\Graphics.hpp>

#include <string>

namespace fleet {
    class UpgradeButton : public sf::Drawable {
    public:
        explicit UpgradeButton(const std::string& newLabel, const sf::Font& font, unsigned short numUpgrades);

        void setPosition(float x, float y);
        void setPosition(const sf::Vector2f& position);

        void setLabelString(const std::string& string);
        void setCharacterSize(unsigned newSize);
        void setTextFillColor(const sf::Color& color);

        const sf::Vector2f& getPosition() const { return button.getPosition(); }
        const sf::String& getLabelString() const { return label.getString(); }
        unsigned getCharacterSize() const { return label.getCharacterSize(); }
        const sf::Color& getTextFillColor() const { return label.getFillColor(); }

        bool input(const sf::Vector2f& mousePos, bool canAfford);
        void update(const sf::Vector2f& mousePos, unsigned currentLevel, bool canAfford);
    private:
        sf::RectangleShape button{ sf::Vector2f(default_upgrade_width, default_upgrade_height) };
        sf::Text label;
        std::vector<sf::RectangleShape> indicators;

        void draw(sf::RenderTarget& target, sf::RenderStates states) const override;
    };
}

Expressions.h is just a header of constexpr for replacing magic numbers, and as you can see I am using SFML. Version 2.4.2 if it matters. I'd also like to point out that I am aware of the difference between #pragma once and include gaurds. I chose the former because they are less verbose and support by the major compilers. Lastly, it is a small personal project, completely in my control.

UpgradeButton.cpp

#include "UpgradeButton.h"

namespace fleet {
    UpgradeButton::UpgradeButton(const std::string& newLabel, const sf::Font& font, unsigned short numUpgrades) :
        label{ newLabel, font }
    {
        float indicatorWidth = default_upgrade_width / numUpgrades;
        for (unsigned i = 0; i < numUpgrades; ++i) {
            indicators.emplace_back(sf::RectangleShape(sf::Vector2f(indicatorWidth, default_indicator_height)));
        }

        button.setFillColor(sf::Color::Cyan);
        button.setOutlineThickness(upgrade_button_outline);
        for (auto& indicator : indicators) {
            indicator.setFillColor(sf::Color::Cyan);
            indicator.setOutlineThickness(upgrade_button_outline);
        }
    }

    void UpgradeButton::setPosition(float x, float y)
    {
        button.setPosition(x, y);
        float indicatorX = x;
        float indicatorY = y + (default_upgrade_height - default_indicator_height);
        for (auto& indicator : indicators) {
            indicator.setPosition(indicatorX, indicatorY);
            indicatorX += indicator.getSize().x;
        }
        float labelY = y + default_upgrade_height + upgrade_label_y_offset;
        label.setPosition(x, labelY);
    }
    void UpgradeButton::setPosition(const sf::Vector2f& position)
    {
        setPosition(position.x, position.y);
    }
    void UpgradeButton::setLabelString(const std::string& string)
    {
        label.setString(string);
    }
    void UpgradeButton::setCharacterSize(unsigned newSize)
    {
        label.setCharacterSize(newSize);
    }
    void UpgradeButton::setTextFillColor(const sf::Color& color)
    {
        label.setFillColor(color);
    }

    bool UpgradeButton::input(const sf::Vector2f& mousePos, bool canAfford)
    {
        return canAfford && button.getGlobalBounds().contains(mousePos);
    }
    void UpgradeButton::update(const sf::Vector2f& mousePos, unsigned currentLevel, bool canAfford)
    {
        for (unsigned i = 0; i < currentLevel && i < indicators.size(); ++i) {
            indicators[i].setFillColor(sf::Color::Green);
        }
        for (unsigned i = currentLevel; i < indicators.size(); ++i) {
            indicators[i].setFillColor(sf::Color::Cyan);
        }

        if (currentLevel == indicators.size()) { return; }

        if (button.getGlobalBounds().contains(mousePos)) {
            if (canAfford) {
                indicators[currentLevel].setFillColor(sf::Color::Green);
            }
            else {
                indicators[currentLevel].setFillColor(sf::Color::Red);
            }
        }
        else {
            indicators[currentLevel].setFillColor(sf::Color::Cyan);
        }
    }

    void UpgradeButton::draw(sf::RenderTarget& target, sf::RenderStates states) const
    {
        target.draw(button, states);
        for (auto& indicator : indicators) {
            target.draw(indicator, states);
        }
        target.draw(label, states);
    }
}

Lastly are the tests. I tried to test each of the public functions. The exception was the update() function. It's entire purpose is to update internal unexposed implementation details, so I was unsure how to test it. Or rather uncertain if I even should. I'm using Google Test for my framework and I'm using a test fixture class to construct a default object to test on. My tests all pass.

UpgradeButtonTests.cpp

#include "gtest/gtest.h"

#include "UpgradeButton.h"

#include <SFML\Graphics.hpp>

namespace fleet {
    class UpgradeButtonTest : public testing::Test {
    protected:
        UpgradeButton button{ "Test Button", sf::Font(), 5 };
    };

    TEST_F(UpgradeButtonTest, setPosition) {
        sf::Vector2f position{ 0.F, 0.F };
        button.setPosition(position);
        ASSERT_FLOAT_EQ(position.x, button.getPosition().x);
        ASSERT_FLOAT_EQ(position.y, button.getPosition().y);
        button.setPosition(0.f, 0.f);
        ASSERT_FLOAT_EQ(position.x, button.getPosition().x);
        ASSERT_FLOAT_EQ(position.y, button.getPosition().y);
        button.setPosition(sf::Vector2f());
        ASSERT_FLOAT_EQ(position.x, button.getPosition().x);
        ASSERT_FLOAT_EQ(position.y, button.getPosition().y);
        button.setPosition(200.F, 200.F);
        ASSERT_NE(position.x, button.getPosition().x);
        ASSERT_NE(position.y, button.getPosition().y);
    }

    TEST_F(UpgradeButtonTest, setLabelString) {
        std::string test_string{ "Test Pass" };
        ASSERT_STREQ("Test Button", button.getLabelString().toAnsiString().c_str()) << "Not named Test Button";
        button.setLabelString(test_string);
        ASSERT_STREQ("Test Pass", button.getLabelString().toAnsiString().c_str()) << test_string << " not assigned properly";
        ASSERT_STREQ(test_string.c_str(), button.getLabelString().toAnsiString().c_str()) << test_string << " not assigned properly pt2";
        button.setLabelString("");
        ASSERT_STREQ("",button.getLabelString().toAnsiString().c_str()) << "Breaking on empty string";
        ASSERT_STRNE(test_string.c_str(), button.getLabelString().toAnsiString().c_str()) << "Breaking on inequality test";
    }

    TEST_F(UpgradeButtonTest, setCharacterSize) {
        unsigned test_size{ 52 };
        ASSERT_EQ(30, button.getCharacterSize());
        ASSERT_NE(test_size, button.getCharacterSize());
        button.setCharacterSize(test_size);
        ASSERT_EQ(52, button.getCharacterSize());
    }

    TEST_F(UpgradeButtonTest, setTextFillColor) {
        sf::Color color{ sf::Color::Green };
        ASSERT_NE(color, button.getTextFillColor());
        ASSERT_EQ(sf::Color::White, button.getTextFillColor());
        button.setTextFillColor(color);
        ASSERT_EQ(sf::Color::Green, button.getTextFillColor());
        button.setTextFillColor(sf::Color());
        ASSERT_EQ(sf::Color::Black, button.getTextFillColor());
    }

    TEST_F(UpgradeButtonTest, input) {
        ASSERT_TRUE(button.input(sf::Vector2f(), true));
        ASSERT_FALSE(button.input(sf::Vector2f(), false));
        ASSERT_FALSE(button.input(sf::Vector2f(500.F, 900.F), true));
    }
}
\$\endgroup\$
3
\$\begingroup\$

I would like to talk about two basic topics regarding your tests: test scope and testing what is interesting.

Let me start with the point commonly encountered in tests by people just starting out with testing. Each test seems to test one public method instead of testing one expected behaviour per test.

Should one of your tests fail, you know that something is wrong with a specific method. However, you generally do not know which behaviour or contract is broken. This is especially problematic since a failure of a later Assert might actually be the root cause for the failure of an earlier one. Moreover, your test do not really tell what they test.

Usually, it is a better idea to have one test per expected behaviour and to name it in the Form <situation>_<expected outcome>. E.g. you could have initialfillColor_White or setColor_hasColor.

This way, you know from the test output which behaviour is broken.

Now, regarding what to test. Although testing everything is a good goal, I think that testing trivial logic like a setter that simply forwards to a backing field together with a getter that reads that field is not worth the effort to write the tests. What should be tested is all interesting behaviour, i.e. the non-trivial behaviour that can be observed from the outside. Unfortunately, that is the behaviour you do not test. It is contained in the setPosition method after the first line and in the update method. Although these methods only change internal state, this state is very visible via the draw method.

I think exposing state via the screen, is generally a problem for ubit testing UI components. The approach I have seen the most so far is to not wrote unit test for displayed components at all and instead to make the view trivial.

One more thing I realized regarding your tests is that they focus on the happy path. It might be good to document in the tests what you expect for the cases of 0, 0.2 and NaN for numUpgrades.

\$\endgroup\$
3
\$\begingroup\$

I'd suggest that the most important things to test are the constructor, and the update() function. These functions do more complicated things, and have some (currently hidden) requirements that we need to be careful with.

The other functions are mainly setter / getter functions (which we could arguably skip testing entirely), or only easily testable by visual inspection (setPosition(), draw()).


When testing the constructor, we should think about the following:

  • The label string: is an empty string ok? Is there a max string size? Do we need to do something special with the text (cut it off, scale it down, or just render the whole thing?).

    Assuming the constructor already does what we want, we don't actually need to touch the constructor code. However, adding a test case with an empty string and a test case with a long string is a good idea. This documents the behavior, and proves that it works.

  • numUpgrades is used in calculations in the constructor. If it's zero, we'll divide by zero! Also, should there be an upper limit (e.g. a global max_upgrades)?

    We can at least guard against a zero argument in the constructor code by throwing an exception, asserting, terminating the program, or breaking to the debugger. And we should then add a test case to document the behavior. (GTest has ASSERT_DEATH / EXPECT_DEATH and ASSERT_THROW / EXPECT_THROW for such things).


The input test looks fine, but to be comprehensive we should also test with both parameters resulting in a negative result, i.e.:

ASSERT_FALSE(button.input(sf::Vector2f(500.F, 900.F), false));

For setPosition() there's not a whole lot else we can test. Checking the position of the label and indicators would just be typing that same code again, which isn't very helpful.


We can definitely test the update() function. Looking at each parameter in turn:

  • For mousePos, any valid float is ok (checking for NaN / infinity is probably overkill). So we just need to test the logic that uses it (i.e. cases where the mouse is inside and outside of the button).

  • For currentLevel, we need to establish valid bounds. What if it's zero? What if it's equal to indicatorSize? What if it's greater than indicator size (doing indicators[currentLevel] seems like a bad idea...)? So it looks like we need to add some asserts to the code, and test these cases.

  • For canAfford, we just need to test the logic.

To check the logic, we need access to the results, so we might add a std::vector<sf::RectangleShape> const& getIndicators() const; function.

It would also be useful to define constants for the colors, e.g. current_affordable_upgrade_color current_unaffordable_upgrade_color, etc.


\$\endgroup\$
  • \$\begingroup\$ What would be the best way to handle numUpgrades being zero in the constructor? An exception seems ideal but I'm uncertain. \$\endgroup\$ – bruglesco Apr 21 at 19:02
  • 1
    \$\begingroup\$ Exceptions are fine if we're prepared to handle the exception and recover from it. Otherwise there's no benefit over simply terminating the program. Personally I'd go with an assert (or an equivalent that also triggers in release builds). If numUpgrades is loaded from data, the loading code should ensure that we have a valid value. That way we catch the problem as soon as possible. \$\endgroup\$ – user673679 Apr 21 at 20:29
  • \$\begingroup\$ I hate not giving you a check as well. I accepted the other answer because it changed my entire approach to unit-testing. I still learned a lot from your answer and appreciate it. Thank you. \$\endgroup\$ – bruglesco Apr 26 at 3:09

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.