common: do not accept invalid input in VersionHelper and add negative…

… unit tests
microsoft · Aug 28, 2020 · fc4ac80 · fc4ac80
1 parent 870f109
commit fc4ac80
Show file tree

Hide file tree

Showing 8 changed files with 144 additions and 36 deletions.
diff --git a/src/common/UnitTests-CommonLib/UnitTestsVersionHelper.cpp b/src/common/UnitTests-CommonLib/UnitTestsVersionHelper.cpp
@@ -4,11 +4,20 @@
 
 using namespace Microsoft::VisualStudio::CppUnitTestFramework;
 
+namespace Microsoft::VisualStudio::CppUnitTestFramework
+{
+    template<>
+    inline std::wstring ToString<VersionHelper>(const VersionHelper& v)
+    {
+        return v.toWstring();
+    }
+}
+
 namespace UnitTestsVersionHelper
 {
-    const int MAJOR_VERSION_0 = 0;
-    const int MINOR_VERSION_12 = 12;
-    const int REVISION_VERSION_0 = 0;
+    const size_t MAJOR_VERSION_0 = 0;
+    const size_t MINOR_VERSION_12 = 12;
+    const size_t REVISION_VERSION_0 = 0;
 
     TEST_CLASS (UnitTestsVersionHelper)
     {
@@ -23,9 +32,9 @@ namespace UnitTestsVersionHelper
         }
         TEST_METHOD (integerConstructorShouldProperlyInitializationWithDifferentVersionNumbers)
         {
-            const int testcaseMajor = 2;
-            const int testcaseMinor = 25;
-            const int testcaseRevision = 1;
+            const size_t testcaseMajor = 2;
+            const size_t testcaseMinor = 25;
+            const size_t testcaseRevision = 1;
             VersionHelper sut(testcaseMajor, testcaseMinor, testcaseRevision);
 
             Assert::AreEqual(testcaseMajor, sut.major);
@@ -36,17 +45,77 @@ namespace UnitTestsVersionHelper
         {
             VersionHelper sut("v0.12.3");
 
-            Assert::AreEqual(0, sut.major);
-            Assert::AreEqual(12, sut.minor);
-            Assert::AreEqual(3, sut.revision);
+            Assert::AreEqual(0ull, sut.major);
+            Assert::AreEqual(12ull, sut.minor);
+            Assert::AreEqual(3ull, sut.revision);
         }
         TEST_METHOD (stringConstructorShouldProperlyInitializationWithDifferentVersionNumbers)
         {
             VersionHelper sut("v2.25.1");
 
-            Assert::AreEqual(2, sut.major);
-            Assert::AreEqual(25, sut.minor);
-            Assert::AreEqual(1, sut.revision);
+            Assert::AreEqual(2ull, sut.major);
+            Assert::AreEqual(25ull, sut.minor);
+            Assert::AreEqual(1ull, sut.revision);
+        }
+        TEST_METHOD (emptyStringNotAccepted)
+        {
+            Assert::ExpectException<std::logic_error>([] {
+                VersionHelper sut("");
+            });
+        }
+        TEST_METHOD (invalidStringNotAccepted1)
+        {
+            Assert::ExpectException<std::logic_error>([] {
+                VersionHelper sut("v2a.");
+            });
+        }
+        TEST_METHOD (invalidStringNotAccepted2)
+        {
+            Assert::ExpectException<std::logic_error>([] {
+                VersionHelper sut("12abc2vv.0");
+            });
+        }
+        TEST_METHOD (invalidStringNotAccepted3)
+        {
+            Assert::ExpectException<std::logic_error>([] {
+                VersionHelper sut("123");
+            });
+        }
+        TEST_METHOD (invalidStringNotAccepted4)
+        {
+            Assert::ExpectException<std::logic_error>([] {
+                VersionHelper sut("v1v2v3");
+            });
+        }
+        TEST_METHOD (invalidStringNotAccepted5)
+        {
+            Assert::ExpectException<std::logic_error>([] {
+                VersionHelper sut("v.1.2.3v");
+            });
+        }
+        TEST_METHOD (partialVersionStringNotAccepted1)
+        {
+            Assert::ExpectException<std::logic_error>([] {
+                VersionHelper sut("v1.");
+            });
+        }
+        TEST_METHOD (partialVersionStringNotAccepted2)
+        {
+            Assert::ExpectException<std::logic_error>([] {
+                VersionHelper sut("v1.2");
+            });
+        }
+        TEST_METHOD (partialVersionStringNotAccepted3)
+        {
+            Assert::ExpectException<std::logic_error>([] {
+                VersionHelper sut("v1.2.");
+            });
+        }
+        TEST_METHOD (parsedWithoutLeadingV)
+        {
+            VersionHelper expected{ 12ull, 13ull, 111ull };
+            VersionHelper actual("12.13.111");
+            Assert::AreEqual(actual, expected);
         }
         TEST_METHOD (whenMajorVersionIsGreaterComparisonOperatorShouldReturnProperValue)
         {

diff --git a/src/common/VersionHelper.cpp b/src/common/VersionHelper.cpp
@@ -1,30 +1,32 @@
 #include "pch.h"
 #include "VersionHelper.h"
 
+#include "string_utils.h"
+
 #include <algorithm>
 #include <sstream>
 
 VersionHelper::VersionHelper(std::string str)
 {
-    std::replace(str.begin(), str.end(), '.', ' ');
-    std::replace(str.begin(), str.end(), 'v', ' ');
-    std::stringstream ss;
-
-    ss << str;
+    // Remove whitespaces chars and a leading 'v'
+    str = left_trim(trim(str), "v");
+    // Replace '.' with spaces
+    replace_chars(str, ".", ' ');
 
-    std::string temp;
-    ss >> temp;
-    std::stringstream(temp) >> major;
-    ss >> temp;
-    std::stringstream(temp) >> minor;
-    ss >> temp;
-    std::stringstream(temp) >> revision;
+    std::istringstream ss{ str };
+    ss >> major;
+    ss >> minor;
+    ss >> revision;
+    if (ss.fail() || !ss.eof())
+    {
+        throw std::logic_error("VersionHelper: couldn't parse the supplied version string");
+    }
 }
 
-VersionHelper::VersionHelper(int major, int minor, int revision) :
-    major(major),
-    minor(minor),
-    revision(revision)
+VersionHelper::VersionHelper(const size_t major, const size_t minor, const size_t revision) :
+    major{ major },
+    minor{ minor },
+    revision{ revision }
 {
 }
 

diff --git a/src/common/VersionHelper.h b/src/common/VersionHelper.h
@@ -6,13 +6,13 @@
 struct VersionHelper
 {
     VersionHelper(std::string str);
-    VersionHelper(int major, int minor, int revision);
+    VersionHelper(const size_t major, const size_t minor, const size_t revision);
 
     auto operator<=>(const VersionHelper&) const = default;
 
-    int major;
-    int minor;
-    int revision;
+    size_t major;
+    size_t minor;
+    size_t revision;
 
     std::wstring toWstring() const;
 };
diff --git a/src/common/common.vcxproj b/src/common/common.vcxproj
@@ -138,6 +138,7 @@
     <ClInclude Include="os-detect.h" />
     <ClInclude Include="RestartManagement.h" />
     <ClInclude Include="shared_constants.h" />
+    <ClInclude Include="string_utils.h" />
     <ClInclude Include="timeutil.h" />
     <ClInclude Include="two_way_pipe_message_ipc.h" />
     <ClInclude Include="VersionHelper.h" />

diff --git a/src/common/common.vcxproj.filters b/src/common/common.vcxproj.filters
@@ -131,13 +131,16 @@
     </ClInclude>
     <ClInclude Include="comUtils.h">
       <Filter>Header Files</Filter>
-    </ClInclude>    
+    </ClInclude>
     <ClInclude Include="LowlevelKeyboardEvent.h">
       <Filter>Header Files</Filter>
     </ClInclude>
     <ClInclude Include="WinHookEvent.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="string_utils.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="d2d_svg.cpp">
@@ -223,4 +226,4 @@
   <ItemGroup>
     <None Include="packages.config" />
   </ItemGroup>
-</Project>
+</Project>
diff --git a/src/common/d2d_svg.cpp b/src/common/d2d_svg.cpp
@@ -34,10 +34,10 @@ D2DSVG& D2DSVG::resize(int x, int y, int width, int height, float fill, float ma
     transform = transform * D2D1::Matrix3x2F::Translation((width - svg_width) / 2.0f, (height - svg_height) / 2.0f);
     float h_scale = fill * height / svg_height;
     float v_scale = fill * width / svg_width;
-    used_scale = min(h_scale, v_scale);
+    used_scale = std::min(h_scale, v_scale);
     if (max_scale > 0)
     {
-        used_scale = min(used_scale, max_scale);
+        used_scale = std::min(used_scale, max_scale);
     }
     transform = transform * D2D1::Matrix3x2F::Scale(used_scale, used_scale, D2D1::Point2F(width / 2.0f, height / 2.0f));
     transform = transform * D2D1::Matrix3x2F::Translation((float)x, (float)y);

diff --git a/src/common/pch.h b/src/common/pch.h
@@ -1,3 +1,4 @@
+#define NOMINMAX
 #include <winrt/base.h>
 #include <winrt/Windows.Foundation.Collections.h>
 #include <Windows.h>

diff --git a/src/common/string_utils.h b/src/common/string_utils.h
@@ -0,0 +1,32 @@
+#pragma once
+
+#include <string_view>
+#include <string>
+#include <algorithm>
+
+constexpr inline std::string_view default_trim_arg = " \t\r\n";
+
+inline std::string_view left_trim(std::string_view s, const std::string_view chars_to_trim = default_trim_arg)
+{
+    s.remove_prefix(std::min(s.find_first_not_of(chars_to_trim), size(s)));
+    return s;
+}
+
+inline std::string_view right_trim(std::string_view s, const std::string_view chars_to_trim = default_trim_arg)
+{
+    s.remove_suffix(std::min(size(s) - s.find_last_not_of(chars_to_trim) - 1, size(s)));
+    return s;
+}
+
+inline std::string_view trim(std::string_view s, const std::string_view chars_to_trim = default_trim_arg)
+{
+    return left_trim(right_trim(s, chars_to_trim), chars_to_trim);
+}
+
+inline void replace_chars(std::string& s, const std::string_view chars_to_replace, const char replacement_char)
+{
+    for (const char c : chars_to_replace)
+    {
+        std::replace(begin(s), end(s), c, replacement_char);
+    }
+}