Improve error messages in "robot-simulator"

[siebenschlaefer]

The tests in the exercise "robot-simulator" compare the result of the member functions get_position() and get_bearing() with the operator== like this:

TEST_CASE("A_robots_is_created_with_a_position_and_a_direction")
{
    const Robot r;

    const std::pair<int, int> expected_robot_position{0, 0};
    REQUIRE(expected_robot_position == r.get_position());
    REQUIRE(Bearing::NORTH == r.get_bearing());
}

But Catch2 does not know how to print a std::pair, and it prints an enum or enum class like an integer:

-------------------------------------------------------------------------------
A_robots_is_created_with_a_position_and_a_direction
-------------------------------------------------------------------------------
/home/user/exercism/cpp/robot-simulator/robot_simulator_test.cpp:13
...............................................................................

/home/user/exercism/cpp/robot-simulator/robot_simulator_test.cpp:18: FAILED:
  REQUIRE( expected_robot_position == r.get_position() )
with expansion:
  {?} == {?}


-------------------------------------------------------------------------------
A_robots_is_created_with_a_position_and_a_direction
-------------------------------------------------------------------------------
/home/user/exercism/cpp/robot-simulator/robot_simulator_test.cpp:13
...............................................................................

/home/user/exercism/cpp/robot-simulator/robot_simulator_test.cpp:19: FAILED:
  REQUIRE( Bearing::NORTH == r.get_bearing() )
with expansion:
  0 == 2

That's not really helpful.

---

Catch2 has the a StringMaker for printing custom classes (see the documentation), and it has the macro CATCH_REGISTER_ENUM() for better error messages when working with enums (see the documentation).

By adding a few lines somewhere at the beginning of robot_simulator_test.cpp

// for better error messages
namespace Catch
{
    template <typename T1, typename T2>
    struct StringMaker<std::pair<T1, T2>>
    {
        static std::string convert(const std::pair<T1, T2>& value)
        {
            std::string result = "std::pair{";
            result += StringMaker<T1>::convert(value.first);
            result += ", ";
            result += StringMaker<T2>::convert(value.second);
            result += '}';
            return result;
        }
    };
}
CATCH_REGISTER_ENUM(robot_simulator::Bearing,
        robot_simulator::Bearing::NORTH,
        robot_simulator::Bearing::WEST,
        robot_simulator::Bearing::SOUTH,
        robot_simulator::Bearing::EAST)

we would get better error messages:

-------------------------------------------------------------------------------
A_robots_is_created_with_a_position_and_a_direction
-------------------------------------------------------------------------------
/home/user/exercism/cpp/robot-simulator/robot_simulator_test.cpp:36
...............................................................................

/home/user/exercism/cpp/robot-simulator/robot_simulator_test.cpp:41: FAILED:
  REQUIRE( expected_robot_position == r.get_position() )
with expansion:
  std::pair{0, 0} == std::pair{0, 1}


-------------------------------------------------------------------------------
A_robots_is_created_with_a_position_and_a_direction
-------------------------------------------------------------------------------
/home/user/exercism/cpp/robot-simulator/robot_simulator_test.cpp:36
...............................................................................

/home/user/exercism/cpp/robot-simulator/robot_simulator_test.cpp:42: FAILED:
  REQUIRE( Bearing::NORTH == r.get_bearing() )
with expansion:
  NORTH == SOUTH

---

AFAIK there are only two possible problems:

1. robot_simulator_test.cpp becomes more complex. But IMHO those 22 lines can be ignored.

2. That would effectively enforce the use of enum or enum class for Bearing. IMHO that's not a problem for us because we want idiomatic solutions, and that's enum or better enum class.

IMHO the benefits outweigh these problems. What do you folks think?

[siebenschlaefer]

If we want to enforce the use of enum / enum class, then we would have to add even more code to the tester, since StringMaker only changes how the logs look as far as I understand,

I'd rather leave that to the mentoring. My main concern (for now) is improving the output of the tests.

Even though enum (class) is the way to go for this exercise, I think making it the only way - especially without providing info about that fact - wouldn't be smart

That's a valid point.

even then the example you provided only works when the user names their enums those exact ways

Sorry, I don't understand. The tests require those exact names.

I personally think we should not enforce the enums, but also encourage the learner to use them - maybe give the exercise some tags, and make one of the be enum?

Are tags displayed somewhere where the student can see them as a hint? And I don't understand the last part about "one of the be enum". We could add an enum class Bearing to the initial robot_simulator.h. But that would quite different than what we do for the other exercises where we let the students discover these things on their own and leave it to the mentors to talk about idiomatic C++.

If we don't enforce the Bearing to be an enum or enum class then I don't know how we could improve the error message (0 == 2) without making the tests more complicated.