From 66a04cb45dbbfed17ec4cde76a29829b562452ad Mon Sep 17 00:00:00 2001 From: karashiiro <49822414+karashiiro@users.noreply.github.com> Date: Wed, 13 Mar 2024 18:23:27 -0700 Subject: [PATCH] Fix edge case in GameVersion and refactor - Test coverage has been added for the entire class, and verbose/redundant code has been refactored - Fixes JSON serialization: JsonConstructor requires that the ctor parameters match fields/properties of the target class. Previously, this meant that the JSON constructor would always throw an ArgumentNullException, as `Input` was not a class property. --- Dalamud.Common/Game/GameVersion.cs | 134 +++++----------- Dalamud.Test/Dalamud.Test.csproj | 2 +- Dalamud.Test/Game/GameVersionTests.cs | 212 +++++++++++++++++++++++++- 3 files changed, 250 insertions(+), 98 deletions(-) diff --git a/Dalamud.Common/Game/GameVersion.cs b/Dalamud.Common/Game/GameVersion.cs index 26ff0e48f..2112a43ea 100644 --- a/Dalamud.Common/Game/GameVersion.cs +++ b/Dalamud.Common/Game/GameVersion.cs @@ -23,7 +23,6 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable class. /// /// Version string to parse. - [JsonConstructor] public GameVersion(string version) { var ver = Parse(version); @@ -42,20 +41,9 @@ public sealed class GameVersion : ICloneable, IComparable, IComparableThe day. /// The major version. /// The minor version. - public GameVersion(int year, int month, int day, int major, int minor) + [JsonConstructor] + public GameVersion(int year, int month, int day, int major, int minor) : this(year, month, day, major) { - if ((this.Year = year) < 0) - throw new ArgumentOutOfRangeException(nameof(year)); - - if ((this.Month = month) < 0) - throw new ArgumentOutOfRangeException(nameof(month)); - - if ((this.Day = day) < 0) - throw new ArgumentOutOfRangeException(nameof(day)); - - if ((this.Major = major) < 0) - throw new ArgumentOutOfRangeException(nameof(major)); - if ((this.Minor = minor) < 0) throw new ArgumentOutOfRangeException(nameof(minor)); } @@ -67,17 +55,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparableThe month. /// The day. /// The major version. - public GameVersion(int year, int month, int day, int major) + public GameVersion(int year, int month, int day, int major) : this(year, month, day) { - if ((this.Year = year) < 0) - throw new ArgumentOutOfRangeException(nameof(year)); - - if ((this.Month = month) < 0) - throw new ArgumentOutOfRangeException(nameof(month)); - - if ((this.Day = day) < 0) - throw new ArgumentOutOfRangeException(nameof(day)); - if ((this.Major = major) < 0) throw new ArgumentOutOfRangeException(nameof(major)); } @@ -88,14 +67,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparableThe year. /// The month. /// The day. - public GameVersion(int year, int month, int day) + public GameVersion(int year, int month, int day) : this(year, month) { - if ((this.Year = year) < 0) - throw new ArgumentOutOfRangeException(nameof(year)); - - if ((this.Month = month) < 0) - throw new ArgumentOutOfRangeException(nameof(month)); - if ((this.Day = day) < 0) throw new ArgumentOutOfRangeException(nameof(day)); } @@ -105,11 +78,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable /// The year. /// The month. - public GameVersion(int year, int month) + public GameVersion(int year, int month) : this(year) { - if ((this.Year = year) < 0) - throw new ArgumentOutOfRangeException(nameof(year)); - if ((this.Month = month) < 0) throw new ArgumentOutOfRangeException(nameof(month)); } @@ -183,17 +153,13 @@ public sealed class GameVersion : ICloneable, IComparable, IComparableGameVersion object. public static GameVersion Parse(string input) { - if (input == null) - throw new ArgumentNullException(nameof(input)); + ArgumentNullException.ThrowIfNull(input); if (input.ToLower(CultureInfo.InvariantCulture) == "any") - return new GameVersion(); + return Any; var parts = input.Split('.'); - var tplParts = parts.Select(p => - { - var result = int.TryParse(p, out var value); - return (result, value); - }).ToArray(); + var tplParts = parts.Select( + p => + { + var result = int.TryParse(p, out var value); + return (result, value); + }).ToArray(); if (tplParts.Any(t => !t.result)) throw new FormatException("Bad formatting"); @@ -259,18 +223,15 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable t.value).ToArray(); var len = intParts.Length; - if (len == 1) - return new GameVersion(intParts[0]); - else if (len == 2) - return new GameVersion(intParts[0], intParts[1]); - else if (len == 3) - return new GameVersion(intParts[0], intParts[1], intParts[2]); - else if (len == 4) - return new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3]); - else if (len == 5) - return new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3], intParts[4]); - else - throw new ArgumentException("Too many parts"); + return len switch + { + 1 => new GameVersion(intParts[0]), + 2 => new GameVersion(intParts[0], intParts[1]), + 3 => new GameVersion(intParts[0], intParts[1], intParts[2]), + 4 => new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3]), + 5 => new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3], intParts[4]), + _ => throw new ArgumentException("Too many parts"), + }; } /// @@ -299,17 +260,12 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable public int CompareTo(object? obj) { - if (obj == null) - return 1; - - if (obj is GameVersion value) + return obj switch { - return this.CompareTo(value); - } - else - { - throw new ArgumentException("Argument must be a GameVersion"); - } + null => 1, + GameVersion value => this.CompareTo(value), + _ => throw new ArgumentException("Argument must be a GameVersion", nameof(obj)), + }; } /// @@ -342,16 +298,14 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable value.Minor ? 1 : -1; + // This should never happen return 0; } /// public override bool Equals(object? obj) { - if (obj is not GameVersion value) - return false; - - return this.Equals(value); + return obj is GameVersion value && this.Equals(value); } /// @@ -373,16 +327,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable public override int GetHashCode() { - var accumulator = 0; - - // This might be horribly wrong, but it isn't used heavily. - accumulator |= this.Year.GetHashCode(); - accumulator |= this.Month.GetHashCode(); - accumulator |= this.Day.GetHashCode(); - accumulator |= this.Major.GetHashCode(); - accumulator |= this.Minor.GetHashCode(); - - return accumulator; + // https://learn.microsoft.com/en-us/dotnet/api/system.object.gethashcode?view=net-8.0#notes-to-inheritors + return HashCode.Combine(this.Year, this.Month, this.Day, this.Major, this.Minor); } /// @@ -396,11 +342,11 @@ public sealed class GameVersion : ICloneable, IComparable, IComparablewin-x64 x64 x64;AnyCPU - 9.0 + 11.0 diff --git a/Dalamud.Test/Game/GameVersionTests.cs b/Dalamud.Test/Game/GameVersionTests.cs index dcace4279..b45d55c4b 100644 --- a/Dalamud.Test/Game/GameVersionTests.cs +++ b/Dalamud.Test/Game/GameVersionTests.cs @@ -1,10 +1,71 @@ +using System; + using Dalamud.Common.Game; + +using Newtonsoft.Json; + using Xunit; namespace Dalamud.Test.Game { public class GameVersionTests { + [Fact] + public void VersionComparisons() + { + var v1 = GameVersion.Parse("2021.01.01.0000.0000"); + var v2 = GameVersion.Parse("2021.01.01.0000.0000"); + Assert.True(v1 == v2); + Assert.False(v1 != v2); + Assert.False(v1 < v2); + Assert.True(v1 <= v2); + Assert.False(v1 > v2); + Assert.True(v1 >= v2); + } + + [Fact] + public void VersionAddition() + { + var v1 = GameVersion.Parse("2021.01.01.0000.0000"); + var v2 = GameVersion.Parse("2021.01.05.0000.0000"); + Assert.Equal(v2, v1 + TimeSpan.FromDays(4)); + } + + [Fact] + public void VersionAdditionAny() + { + Assert.Equal(GameVersion.Any, GameVersion.Any + TimeSpan.FromDays(4)); + } + + [Fact] + public void VersionSubtraction() + { + var v1 = GameVersion.Parse("2021.01.05.0000.0000"); + var v2 = GameVersion.Parse("2021.01.01.0000.0000"); + Assert.Equal(v2, v1 - TimeSpan.FromDays(4)); + } + + [Fact] + public void VersionSubtractionAny() + { + Assert.Equal(GameVersion.Any, GameVersion.Any - TimeSpan.FromDays(4)); + } + + [Fact] + public void VersionClone() + { + var v1 = GameVersion.Parse("2021.01.01.0000.0000"); + var v2 = v1.Clone(); + Assert.NotSame(v1, v2); + } + + [Fact] + public void VersionCast() + { + var v = GameVersion.Parse("2021.01.01.0000.0000"); + Assert.Equal("2021.01.01.0000.0000", v); + } + [Theory] [InlineData("any", "any")] [InlineData("2021.01.01.0000.0000", "2021.01.01.0000.0000")] @@ -14,6 +75,18 @@ namespace Dalamud.Test.Game var v2 = GameVersion.Parse(ver2); Assert.Equal(v1, v2); + Assert.Equal(0, v1.CompareTo(v2)); + Assert.Equal(v1.GetHashCode(), v2.GetHashCode()); + } + + [Fact] + public void VersionNullEquality() + { + // Tests `Equals(GameVersion? value)` + Assert.False(GameVersion.Parse("2021.01.01.0000.0000").Equals(null)); + + // Tests `Equals(object? value)` + Assert.False(GameVersion.Parse("2021.01.01.0000.0000").Equals((object)null)); } [Theory] @@ -31,6 +104,67 @@ namespace Dalamud.Test.Game Assert.True(v1.CompareTo(v2) < 0); } + [Theory] + [InlineData("any", "2020.06.15.0000.0000")] + public void VersionComparisonInverse(string ver1, string ver2) + { + var v1 = GameVersion.Parse(ver1); + var v2 = GameVersion.Parse(ver2); + + Assert.True(v1.CompareTo(v2) > 0); + } + + [Fact] + public void VersionComparisonNull() + { + var v = GameVersion.Parse("2020.06.15.0000.0000"); + + // Tests `CompareTo(GameVersion? value)` + Assert.True(v.CompareTo(null) > 0); + + // Tests `CompareTo(object? value)` + Assert.True(v.CompareTo((object)null) > 0); + } + + [Fact] + public void VersionComparisonBoxed() + { + var v1 = GameVersion.Parse("2020.06.15.0000.0000"); + var v2 = GameVersion.Parse("2020.06.15.0000.0000"); + Assert.Equal(0, v1.CompareTo((object)v2)); + } + + [Fact] + public void VersionComparisonBoxedInvalid() + { + var v = GameVersion.Parse("2020.06.15.0000.0000"); + Assert.Throws(() => v.CompareTo(42)); + } + + [Theory] + [InlineData("2020.06.15.0000.0000")] + [InlineData("2021.01.01.0000")] + [InlineData("2021.01.01")] + [InlineData("2021.01")] + [InlineData("2021")] + public void VersionParse(string ver) + { + var v = GameVersion.Parse(ver); + Assert.NotNull(v); + } + + [Theory] + [InlineData("2020.06.15.0000.0000")] + [InlineData("2021.01.01.0000")] + [InlineData("2021.01.01")] + [InlineData("2021.01")] + [InlineData("2021")] + public void VersionTryParse(string ver) + { + Assert.True(GameVersion.TryParse(ver, out var v)); + Assert.NotNull(v); + } + [Theory] [InlineData("2020.06.15.0000.0000")] [InlineData("2021.01.01.0000")] @@ -39,9 +173,8 @@ namespace Dalamud.Test.Game [InlineData("2021")] public void VersionConstructor(string ver) { - var v = GameVersion.Parse(ver); - - Assert.True(v != null); + var v = new GameVersion(ver); + Assert.NotNull(v); } [Theory] @@ -54,5 +187,78 @@ namespace Dalamud.Test.Game Assert.False(result); Assert.Null(v); } + + [Theory] + [InlineData("any", "any")] + [InlineData("2020.06.15.0000.0000", "2020.06.15.0000.0000")] + [InlineData("2021.01.01.0000", "2021.01.01.0000.0000")] + [InlineData("2021.01.01", "2021.01.01.0000.0000")] + [InlineData("2021.01", "2021.01.00.0000.0000")] + [InlineData("2021", "2021.00.00.0000.0000")] + public void VersionToString(string ver1, string ver2) + { + var v1 = GameVersion.Parse(ver1); + Assert.Equal(ver2, v1.ToString()); + } + + [Fact] + public void VersionIsSerializationSafe() + { + var v = GameVersion.Parse("2020.06.15.0000.0000"); + var serialized = JsonConvert.SerializeObject(v); + var deserialized = JsonConvert.DeserializeObject(serialized); + Assert.Equal(v, deserialized); + } + + [Fact] + public void VersionInvalidDeserialization() + { + var serialized = """ + { + "Year": -1, + "Month": -1, + "Day": -1, + "Major": -1, + "Minor": -1, + } + """; + Assert.Throws(() => JsonConvert.DeserializeObject(serialized)); + } + + [Fact] + public void VersionConstructorNegativeYear() + { + Assert.Throws(() => new GameVersion(-2024)); + } + + [Fact] + public void VersionConstructorNegativeMonth() + { + Assert.Throws(() => new GameVersion(2024, -3)); + } + + [Fact] + public void VersionConstructorNegativeDay() + { + Assert.Throws(() => new GameVersion(2024, 3, -13)); + } + + [Fact] + public void VersionConstructorNegativeMajor() + { + Assert.Throws(() => new GameVersion(2024, 3, 13, -1)); + } + + [Fact] + public void VersionConstructorNegativeMinor() + { + Assert.Throws(() => new GameVersion(2024, 3, 13, 0, -1)); + } + + [Fact] + public void VersionParseNull() + { + Assert.Throws(() => GameVersion.Parse(null!)); + } } }