From 193d105a3fde2ab0d0063d8c32e1bc5eb5ba38a7 Mon Sep 17 00:00:00 2001
From: MrSimbax <matt@przybyl.io>
Date: Wed, 8 Dec 2021 22:33:23 +0100
Subject: [PATCH] Refactor CBotUT.TestSaveStateIoFunctions

Split the test into several parametrised tests for each data type.

Add more tests with different values.

Fix the ReadBinary() function in order to fix the ReadDouble() function
for negative values (the sign bit was being lost).
---
 src/CBot/CBotFileUtils.cpp            |   2 +-
 test/unit/CBot/CBotFileUtils_test.cpp | 285 ++++++++++++++++++++++++++
 test/unit/CBot/CBot_test.cpp          | 194 ------------------
 test/unit/CMakeLists.txt              |   3 +-
 4 files changed, 288 insertions(+), 196 deletions(-)
 create mode 100644 test/unit/CBot/CBotFileUtils_test.cpp

diff --git a/src/CBot/CBotFileUtils.cpp b/src/CBot/CBotFileUtils.cpp
index cdbc62af..cebe9acb 100644
--- a/src/CBot/CBotFileUtils.cpp
+++ b/src/CBot/CBotFileUtils.cpp
@@ -59,7 +59,7 @@ static bool ReadBinary(std::istream &istr, T &value)
     while (true)        // unsigned LEB128
     {
         if (!istr.read(reinterpret_cast<char*>(&chr), 1)) return false;
-        if (shift < sizeof(T) * 8 - 1)
+        if (shift < sizeof(T) * 8)
             value |= static_cast<T>(chr & 0x7F) << shift;
         shift += 7;
         if ((chr & 0x80) == 0) break;
diff --git a/test/unit/CBot/CBotFileUtils_test.cpp b/test/unit/CBot/CBotFileUtils_test.cpp
new file mode 100644
index 00000000..8f35118c
--- /dev/null
+++ b/test/unit/CBot/CBotFileUtils_test.cpp
@@ -0,0 +1,285 @@
+#include "CBot/CBotFileUtils.h"
+
+#include <gtest/gtest.h>
+
+namespace
+{
+std::string createTestStringWithAllPossibleCharacters()
+{
+    std::string testString;
+    for (char c = std::numeric_limits<char>::min(); ; ++c)
+    {
+        testString.push_back(c);
+        if (c == std::numeric_limits<char>::max()) break;
+    }
+    return testString;
+}
+}
+
+namespace CBot
+{
+
+struct CBotFileUtilsTest : public testing::Test
+{
+    std::stringstream stream;
+};
+
+struct CBotFileUtilsReadWriteByteTest : public CBotFileUtilsTest, public testing::WithParamInterface<char>
+{
+};
+
+TEST_P(CBotFileUtilsReadWriteByteTest, ReadByteValueShouldMatchWrittenValue)
+{
+    char expectedValue{GetParam()};
+    ASSERT_TRUE(WriteByte(stream, expectedValue));
+    char value{1};
+    ASSERT_TRUE(ReadByte(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    CBotIoReadWriteTest,
+    CBotFileUtilsReadWriteByteTest,
+    testing::Values(
+        '\0',
+        static_cast<char>(42),
+        std::numeric_limits<char>::min(),
+        std::numeric_limits<char>::max()));
+
+struct CBotFileUtilsReadWriteWordTest : public CBotFileUtilsTest, public testing::WithParamInterface<unsigned short>
+{
+};
+
+TEST_P(CBotFileUtilsReadWriteWordTest, ReadWordValueShouldMatchWrittenValue)
+{
+    unsigned short expectedValue{GetParam()};
+    ASSERT_TRUE(WriteWord(stream, expectedValue));
+    unsigned short value{1};
+    ASSERT_TRUE(ReadWord(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    CBotIoReadWriteTest,
+    CBotFileUtilsReadWriteWordTest,
+    testing::Values(
+        static_cast<unsigned short>(0),
+        static_cast<unsigned short>(42),
+        std::numeric_limits<unsigned short>::min(),
+        std::numeric_limits<unsigned short>::max() / 2,
+        std::numeric_limits<unsigned short>::max()));
+
+struct CBotFileUtilsReadWriteShortTest : public CBotFileUtilsTest, public testing::WithParamInterface<short>
+{
+};
+
+TEST_P(CBotFileUtilsReadWriteShortTest, ReadShortValueShouldMatchWrittenValue)
+{
+    short expectedValue{GetParam()};
+    ASSERT_TRUE(WriteShort(stream, expectedValue));
+    short value{1};
+    ASSERT_TRUE(ReadShort(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    CBotIoReadWriteTest,
+    CBotFileUtilsReadWriteShortTest,
+    testing::Values(
+        static_cast<short>(-7),
+        static_cast<short>(-1),
+        static_cast<short>(0),
+        static_cast<short>(42),
+        std::numeric_limits<short>::min(),
+        std::numeric_limits<short>::min() / 2,
+        std::numeric_limits<short>::max() / 2,
+        std::numeric_limits<short>::max()));
+
+struct CBotFileUtilsReadWriteUInt32Test : public CBotFileUtilsTest, public testing::WithParamInterface<uint32_t>
+{
+};
+
+TEST_P(CBotFileUtilsReadWriteUInt32Test, ReadUInt32ValueShouldMatchWrittenValue)
+{
+    uint32_t expectedValue{GetParam()};
+    ASSERT_TRUE(WriteUInt32(stream, expectedValue));
+    uint32_t value{1};
+    ASSERT_TRUE(ReadUInt32(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    CBotIoReadWriteTest,
+    CBotFileUtilsReadWriteUInt32Test,
+    testing::Values(
+        static_cast<uint32_t>(0),
+        static_cast<uint32_t>(42),
+        std::numeric_limits<uint32_t>::max() / 2,
+        std::numeric_limits<uint32_t>::max()));
+
+struct CBotFileUtilsReadWriteIntTest : public CBotFileUtilsTest, public testing::WithParamInterface<int>
+{
+};
+
+TEST_P(CBotFileUtilsReadWriteIntTest, ReadIntValueShouldMatchWrittenValue)
+{
+    int expectedValue{GetParam()};
+    ASSERT_TRUE(WriteInt(stream, expectedValue));
+    int value{1};
+    ASSERT_TRUE(ReadInt(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    CBotIoReadWriteTest,
+    CBotFileUtilsReadWriteIntTest,
+    testing::Values(
+        static_cast<int>(7),
+        static_cast<int>(-1),
+        static_cast<int>(0),
+        static_cast<int>(42),
+        std::numeric_limits<int>::min(),
+        std::numeric_limits<int>::min() / 2,
+        std::numeric_limits<int>::max() / 2,
+        std::numeric_limits<int>::max()));
+
+struct CBotFileUtilsReadWriteLongTest : public CBotFileUtilsTest, public testing::WithParamInterface<long>
+{
+};
+
+TEST_P(CBotFileUtilsReadWriteLongTest, ReadLongValueShouldMatchWrittenValue)
+{
+    long value{1};
+    long expectedValue{GetParam()};
+    ASSERT_TRUE(WriteLong(stream, expectedValue));
+    ASSERT_TRUE(ReadLong(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+TEST_P(CBotFileUtilsReadWriteLongTest, ReadLongValueShouldMatchWrittenValueWithPadding)
+{
+    constexpr int padding = 10;
+    long expectedValue{GetParam()};
+    ASSERT_TRUE(WriteLong(stream, expectedValue, padding));
+    long value{1};
+    ASSERT_TRUE(ReadLong(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+TEST_P(CBotFileUtilsReadWriteLongTest, ReadLongValueShouldMatchWrittenValueWithPaddingAndMultipleValues)
+{
+    constexpr int padding = 10;
+    long value{1};
+    long expectedValue{GetParam()};
+    int anotherValue{1};
+    int anotherExpectedValue{2};
+    ASSERT_TRUE(WriteLong(stream, expectedValue, padding));
+    ASSERT_TRUE(WriteInt(stream, anotherExpectedValue));
+    ASSERT_TRUE(ReadLong(stream, value));
+    ASSERT_TRUE(ReadInt(stream, anotherValue));
+    ASSERT_EQ(expectedValue, value);
+    ASSERT_EQ(anotherExpectedValue, anotherValue);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    CBotIoReadWriteTest,
+    CBotFileUtilsReadWriteLongTest,
+    testing::Values(
+        static_cast<long>(7),
+        static_cast<long>(-1),
+        static_cast<long>(0),
+        static_cast<long>(42),
+        std::numeric_limits<long>::min(),
+        std::numeric_limits<long>::min() / 2,
+        std::numeric_limits<long>::max() / 2,
+        std::numeric_limits<long>::max()));
+
+struct CBotFileUtilsReadWriteFloatTest : public CBotFileUtilsTest, public testing::WithParamInterface<float>
+{
+};
+
+TEST_P(CBotFileUtilsReadWriteFloatTest, ReadFloatValueShouldMatchWrittenValue)
+{
+    float expectedValue{GetParam()};
+    ASSERT_TRUE(WriteFloat(stream, expectedValue));
+    float value{1.0f};
+    ASSERT_TRUE(ReadFloat(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    CBotIoReadWriteTest,
+    CBotFileUtilsReadWriteFloatTest,
+    testing::Values(
+        7.0f,
+        -1.0f,
+        0.0f,
+        42.0f,
+        3.14f,
+        -2.73f,
+        std::numeric_limits<float>::min(),
+        std::numeric_limits<float>::min() / 2.0f,
+        std::numeric_limits<float>::max() / 2.0f,
+        std::numeric_limits<float>::max()));
+
+struct CBotFileUtilsReadWriteDoubleTest : public CBotFileUtilsTest, public testing::WithParamInterface<double>
+{
+};
+
+TEST_P(CBotFileUtilsReadWriteDoubleTest, ReadDoubleValueShouldMatchWrittenValue)
+{
+    double expectedValue{GetParam()};
+    ASSERT_TRUE(WriteDouble(stream, expectedValue));
+    double value{1.0};
+    ASSERT_TRUE(ReadDouble(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    CBotIoReadWriteTest,
+    CBotFileUtilsReadWriteDoubleTest,
+    testing::Values(
+        7.0,
+        -1.0,
+        0.0,
+        42.0,
+        3.14,
+        -2.73,
+        std::numeric_limits<double>::min(),
+        std::numeric_limits<double>::min() / 2.0,
+        std::numeric_limits<double>::max() / 2.0,
+        std::numeric_limits<double>::max()));
+
+struct CBotFileUtilsReadWriteStringTest : public CBotFileUtilsTest, public testing::WithParamInterface<std::string>
+{
+};
+
+TEST_P(CBotFileUtilsReadWriteStringTest, ReadStringValueShouldMatchWrittenValue)
+{
+    std::string expectedValue{GetParam()};
+    ASSERT_TRUE(WriteString(stream, expectedValue));
+    std::string value{"test"};
+    ASSERT_TRUE(ReadString(stream, value));
+    ASSERT_EQ(expectedValue, value);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    CBotIoReadWriteTest,
+    CBotFileUtilsReadWriteStringTest,
+    testing::Values(
+        "",
+        "123",
+        "abc",
+        createTestStringWithAllPossibleCharacters()));
+
+TEST_F(CBotFileUtilsTest, ReadStreamShouldMatchWrittenStream)
+{
+    std::string expectedValue{"Lorem ipsum dolor sit amet"};
+    std::stringstream initialStream{expectedValue};
+    ASSERT_TRUE(WriteStream(stream, initialStream));
+    std::stringstream newStream{};
+    ASSERT_TRUE(ReadStream(stream, newStream));
+    ASSERT_EQ(expectedValue, newStream.str());
+}
+
+}
diff --git a/test/unit/CBot/CBot_test.cpp b/test/unit/CBot/CBot_test.cpp
index 3d1ee516..7da15f69 100644
--- a/test/unit/CBot/CBot_test.cpp
+++ b/test/unit/CBot/CBot_test.cpp
@@ -320,200 +320,6 @@ protected:
     }
 };
 
-TEST_F(CBotUT, TestSaveStateIOFunctions)
-{
-    std::stringstream sstr("");
-    std::string teststring;
-
-    for (char c = std::numeric_limits<char>::min() ;; ++c)
-    {
-        teststring.push_back(c);
-        if ( c == std::numeric_limits<char>::max() ) break;
-    }
-
-    auto CallWriteFunctions = [&sstr, &teststring]() -> bool
-    {
-        if (!WriteWord(sstr, static_cast<unsigned short>(0))) return false;
-        if (!WriteWord(sstr, std::numeric_limits<unsigned short>::max() / 2)) return false;
-        if (!WriteWord(sstr, std::numeric_limits<unsigned short>::max())) return false;
-
-        if (!WriteByte(sstr, std::numeric_limits<char>::min())) return false;
-        if (!WriteByte(sstr, std::numeric_limits<char>::max())) return false;
-
-        if (!WriteShort(sstr, std::numeric_limits<short>::min())) return false;
-        if (!WriteShort(sstr, std::numeric_limits<short>::min() / 2)) return false;
-        if (!WriteShort(sstr, -1)) return false;
-        if (!WriteShort(sstr, 0)) return false;
-        if (!WriteShort(sstr, std::numeric_limits<short>::max() / 2)) return false;
-        if (!WriteShort(sstr, std::numeric_limits<short>::max())) return false;
-
-        if (!WriteUInt32(sstr, static_cast<uint32_t>(0))) return false;
-        if (!WriteUInt32(sstr, std::numeric_limits<uint32_t>::max() / 2)) return false;
-        if (!WriteUInt32(sstr, std::numeric_limits<uint32_t>::max())) return false;
-
-        if (!WriteInt(sstr, std::numeric_limits<int>::min())) return false;
-        if (!WriteInt(sstr, std::numeric_limits<int>::min() / 2)) return false;
-        if (!WriteInt(sstr, -1)) return false;
-        if (!WriteInt(sstr, 0)) return false;
-        if (!WriteInt(sstr, std::numeric_limits<int>::max() / 2)) return false;
-        if (!WriteInt(sstr, std::numeric_limits<int>::max())) return false;
-
-        if (!WriteLong(sstr, std::numeric_limits<long>::min())) return false;
-        if (!WriteLong(sstr, std::numeric_limits<long>::min() / 2L)) return false;
-        if (!WriteLong(sstr, -1L)) return false;
-        if (!WriteLong(sstr, 0L)) return false;
-        if (!WriteLong(sstr, std::numeric_limits<long>::max() / 2L)) return false;
-        if (!WriteLong(sstr, std::numeric_limits<long>::max())) return false;
-
-        // test with padding bytes (not currently used anywhere)
-        if (!WriteLong(sstr, 1234567890L, 10)) return false;
-
-        if (!WriteFloat(sstr, std::numeric_limits<float>::min())) return false;
-        if (!WriteFloat(sstr, 0.0f)) return false;
-        if (!WriteFloat(sstr, std::numeric_limits<float>::max())) return false;
-
-        if (!WriteDouble(sstr, std::numeric_limits<double>::min())) return false;
-        if (!WriteDouble(sstr, 0.0)) return false;
-        if (!WriteDouble(sstr, std::numeric_limits<double>::max())) return false;
-
-        if (!WriteString(sstr, "")) return false;
-        if (!WriteString(sstr, teststring)) return false;
-        return true;
-    };
-
-    if ( !CallWriteFunctions() )
-    {
-        ADD_FAILURE() << "failed in CallWriteFunctions()" << std::endl;
-        return;
-    }
-
-    std::stringstream savestream("");
-
-    if ( !WriteStream(savestream, sstr) )
-    {
-        ADD_FAILURE() << "CBot::WriteStream() failed" << std::endl;
-        return;
-    }
-
-    std::stringstream newstream("");
-
-    if ( !ReadStream(savestream, newstream) )
-    {
-        ADD_FAILURE() << "CBot:ReadStream() failed" << std::endl;
-        return;
-    }
-
-    auto CallReadFunctions = [&teststring, &newstream]() -> bool
-    {
-        unsigned short w = 1;
-        if (!ReadWord(newstream, w)) return false;
-        if (w != static_cast<unsigned short>(0)) return false;
-        if (!ReadWord(newstream, w)) return false;
-        if (w != std::numeric_limits<unsigned short>::max() / 2) return false;
-
-        if (!ReadWord(newstream, w)) return false;
-        if (w != std::numeric_limits<unsigned short>::max()) return false;
-
-        char c = 1;
-        if (!ReadByte(newstream, c)) return false;
-        if (c != std::numeric_limits<char>::min()) return false;
-        if (!ReadByte(newstream, c)) return false;
-        if (c != std::numeric_limits<char>::max()) return false;
-
-        short s = 1;
-        if (!ReadShort(newstream, s)) return false;
-        if (s != std::numeric_limits<short>::min()) return false;
-        if (!ReadShort(newstream, s)) return false;
-        if (s != std::numeric_limits<short>::min() / 2) return false;
-
-        if (!ReadShort(newstream, s)) return false;
-        if (s != -1) return false;
-        if (!ReadShort(newstream, s)) return false;
-        if (s != 0) return false;
-
-        if (!ReadShort(newstream, s)) return false;
-        if (s != std::numeric_limits<short>::max() / 2) return false;
-        if (!ReadShort(newstream, s)) return false;
-        if (s != std::numeric_limits<short>::max()) return false;
-
-        uint32_t u = 1;
-        if (!ReadUInt32(newstream, u)) return false;
-        if (u != static_cast<uint32_t>(0)) return false;
-        if (!ReadUInt32(newstream, u)) return false;
-        if (u != std::numeric_limits<uint32_t>::max() / 2) return false;
-
-        if (!ReadUInt32(newstream, u)) return false;
-        if (u != std::numeric_limits<uint32_t>::max()) return false;
-
-        int i = 1;
-        if (!ReadInt(newstream, i)) return false;
-        if (i != std::numeric_limits<int>::min()) return false;
-        if (!ReadInt(newstream, i)) return false;
-        if (i != std::numeric_limits<int>::min() / 2) return false;
-
-        if (!ReadInt(newstream, i)) return false;
-        if (i != -1) return false;
-        if (!ReadInt(newstream, i)) return false;
-        if (i != 0) return false;
-
-        if (!ReadInt(newstream, i)) return false;
-        if (i != std::numeric_limits<int>::max() / 2) return false;
-        if (!ReadInt(newstream, i)) return false;
-        if (i != std::numeric_limits<int>::max()) return false;
-
-        long l = 1L;
-        if (!ReadLong(newstream, l)) return false;
-        if (l != std::numeric_limits<long>::min()) return false;
-        if (!ReadLong(newstream, l)) return false;
-        if (l != std::numeric_limits<long>::min() / 2L) return false;
-
-        if (!ReadLong(newstream, l)) return false;
-        if (l != -1L) return false;
-        if (!ReadLong(newstream, l)) return false;
-        if (l != 0L) return false;
-
-        if (!ReadLong(newstream, l)) return false;
-        if (l != std::numeric_limits<long>::max() / 2L) return false;
-        if (!ReadLong(newstream, l)) return false;
-        if (l != std::numeric_limits<long>::max()) return false;
-
-        if (!ReadLong(newstream, l)) return false;
-        if (l != 1234567890L) return false;
-
-        float f = 1.0f;
-        if (!ReadFloat(newstream, f)) return false;
-        if (f != std::numeric_limits<float>::min()) return false;
-        if (!ReadFloat(newstream, f)) return false;
-        if (f != 0.0f) return false;
-
-        if (!ReadFloat(newstream, f)) return false;
-        if (f != std::numeric_limits<float>::max()) return false;
-
-        double d = 1.0;
-        if (!ReadDouble(newstream, d)) return false;
-        if (d != std::numeric_limits<double>::min()) return false;
-        if (!ReadDouble(newstream, d)) return false;
-        if (d != 0.0) return false;
-
-        if (!ReadDouble(newstream, d)) return false;
-        if (d != std::numeric_limits<double>::max()) return false;
-
-        std::string newstring = "should be empty string after next read";
-        if (!ReadString(newstream, newstring)) return false;
-        if (newstring != "") return false;
-
-        if (!ReadString(newstream, newstring)) return false;
-        if (newstring != teststring) return false;
-        return true;
-    };
-
-    if ( !CallReadFunctions() )
-    {
-        ADD_FAILURE() << "failed in CallReadFunctions()" << std::endl;
-        return;
-    }
-}
-
 TEST_F(CBotUT, EmptyTest)
 {
     ExecuteTest(
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index b549229e..8eb7d7a5 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -11,8 +11,9 @@ add_definitions(-DGTEST_HAS_TR1_TUPLE=0)
 add_executable(colobot_ut
     main.cpp
     app/app_test.cpp
-    CBot/CBotToken_test.cpp
     CBot/CBot_test.cpp
+    CBot/CBotFileUtils_test.cpp
+    CBot/CBotToken_test.cpp
     common/config_file_test.cpp
     common/timeutils_test.cpp
     graphics/engine/lightman_test.cpp