From 32dedc28f3fa2e5ba44ed038e1c8a1195eb657c9 Mon Sep 17 00:00:00 2001 From: Mohammad Rahhal Date: Thu, 13 Jul 2023 10:26:54 +0200 Subject: [PATCH 1/2] Catch CompareTo argument exceptions and fallback to string comparison --- .../Parameters/ParameterComparer.cs | 11 ++- .../ParameterComparerTests.cs | 68 +++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/BenchmarkDotNet/Parameters/ParameterComparer.cs b/src/BenchmarkDotNet/Parameters/ParameterComparer.cs index a1363de073..5a1ef3c4a2 100644 --- a/src/BenchmarkDotNet/Parameters/ParameterComparer.cs +++ b/src/BenchmarkDotNet/Parameters/ParameterComparer.cs @@ -28,7 +28,16 @@ private int CompareValues(object x, object y) if (x != null && y != null && x.GetType() == y.GetType() && x is IComparable xComparable) { - return xComparable.CompareTo(y); + try + { + return xComparable.CompareTo(y); + } + // Some types, such as Tuple and ValueTuple, have a fallible CompareTo implementation which can throw if the inner items don't implement IComparable. + // See: https://github.com/dotnet/BenchmarkDotNet/issues/2346 + // For now, catch and ignore the exception, and fallback to string comparison below. + catch (ArgumentException) + { + } } // Anything else. diff --git a/tests/BenchmarkDotNet.Tests/ParameterComparerTests.cs b/tests/BenchmarkDotNet.Tests/ParameterComparerTests.cs index 0f01045b5c..2ef1bc8891 100644 --- a/tests/BenchmarkDotNet.Tests/ParameterComparerTests.cs +++ b/tests/BenchmarkDotNet.Tests/ParameterComparerTests.cs @@ -157,6 +157,70 @@ public void IComparableComparisionTest() Assert.Equal(4, ((ComplexParameter)sortedData[3].Items[0].Value).Value); } + [Fact] + public void ValueTupleWithNonIComparableInnerTypesComparisionTest() + { + var comparer = ParameterComparer.Instance; + var originalData = new[] + { + new ParameterInstances(new[] + { + new ParameterInstance(sharedDefinition, (new ComplexNonIComparableParameter(), 1), null) + }), + new ParameterInstances(new[] + { + new ParameterInstance(sharedDefinition, (new ComplexNonIComparableParameter(), 3), null) + }), + new ParameterInstances(new[] + { + new ParameterInstance(sharedDefinition, (new ComplexNonIComparableParameter(), 2), null) + }), + new ParameterInstances(new[] + { + new ParameterInstance(sharedDefinition, (new ComplexNonIComparableParameter(), 4), null) + }) + }; + + var sortedData = originalData.OrderBy(d => d, comparer).ToArray(); + + Assert.Equal(1, (((ComplexNonIComparableParameter, int))sortedData[0].Items[0].Value).Item2); + Assert.Equal(2, (((ComplexNonIComparableParameter, int))sortedData[1].Items[0].Value).Item2); + Assert.Equal(3, (((ComplexNonIComparableParameter, int))sortedData[2].Items[0].Value).Item2); + Assert.Equal(4, (((ComplexNonIComparableParameter, int))sortedData[3].Items[0].Value).Item2); + } + + [Fact] + public void TupleWithNonIComparableInnerTypesComparisionTest() + { + var comparer = ParameterComparer.Instance; + var originalData = new[] + { + new ParameterInstances(new[] + { + new ParameterInstance(sharedDefinition, Tuple.Create(new ComplexNonIComparableParameter(), 1), null) + }), + new ParameterInstances(new[] + { + new ParameterInstance(sharedDefinition, Tuple.Create(new ComplexNonIComparableParameter(), 3), null) + }), + new ParameterInstances(new[] + { + new ParameterInstance(sharedDefinition, Tuple.Create(new ComplexNonIComparableParameter(), 2), null) + }), + new ParameterInstances(new[] + { + new ParameterInstance(sharedDefinition, Tuple.Create(new ComplexNonIComparableParameter(), 4), null) + }) + }; + + var sortedData = originalData.OrderBy(d => d, comparer).ToArray(); + + Assert.Equal(1, ((Tuple)sortedData[0].Items[0].Value).Item2); + Assert.Equal(2, ((Tuple)sortedData[1].Items[0].Value).Item2); + Assert.Equal(3, ((Tuple)sortedData[2].Items[0].Value).Item2); + Assert.Equal(4, ((Tuple)sortedData[3].Items[0].Value).Item2); + } + private class ComplexParameter : IComparable, IComparable { public ComplexParameter(int value, string name) @@ -199,5 +263,9 @@ public int CompareTo(object obj) return CompareTo(other); } } + + private class ComplexNonIComparableParameter + { + } } } \ No newline at end of file From 215abd56f08c27a97a62e1751a36814ef4468b9d Mon Sep 17 00:00:00 2001 From: Mohammad Rahhal Date: Thu, 13 Jul 2023 10:55:47 +0200 Subject: [PATCH 2/2] Catch when message contains expected IComparable error --- src/BenchmarkDotNet/Parameters/ParameterComparer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BenchmarkDotNet/Parameters/ParameterComparer.cs b/src/BenchmarkDotNet/Parameters/ParameterComparer.cs index 5a1ef3c4a2..6cadf6a946 100644 --- a/src/BenchmarkDotNet/Parameters/ParameterComparer.cs +++ b/src/BenchmarkDotNet/Parameters/ParameterComparer.cs @@ -35,7 +35,7 @@ private int CompareValues(object x, object y) // Some types, such as Tuple and ValueTuple, have a fallible CompareTo implementation which can throw if the inner items don't implement IComparable. // See: https://github.com/dotnet/BenchmarkDotNet/issues/2346 // For now, catch and ignore the exception, and fallback to string comparison below. - catch (ArgumentException) + catch (ArgumentException ex) when (ex.Message.Contains("At least one object must implement IComparable.")) { } }