From 1cda087d0bf576faab693d55055ed640ae58a5d5 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Mon, 26 Aug 2024 09:46:49 +1000 Subject: [PATCH 1/4] Test that StringComparers and StringComparisons are kept in sync These two utility classes should have the same members, with equivalent values. This change adds missing properties to both classes, and adds a test to ensure that they're kept in sync in future. --- .../StringComparers.cs | 1 + .../StringComparisonTests.cs | 103 ++++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed/StringComparers.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed/StringComparers.cs index 4b0dd384203..8d0c578188d 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed/StringComparers.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed/StringComparers.cs @@ -55,6 +55,7 @@ internal static class StringComparisons public static StringComparison PropertyLiteralValues => StringComparison.OrdinalIgnoreCase; public static StringComparison PropertyValues => StringComparison.Ordinal; public static StringComparison RuleNames => StringComparison.OrdinalIgnoreCase; + public static StringComparison CategoryNames => StringComparison.OrdinalIgnoreCase; public static StringComparison ConfigurationDimensionNames => StringComparison.Ordinal; public static StringComparison ConfigurationDimensionValues => StringComparison.Ordinal; public static StringComparison DependencyIds => StringComparison.OrdinalIgnoreCase; diff --git a/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs b/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs new file mode 100644 index 00000000000..1505b03122a --- /dev/null +++ b/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs @@ -0,0 +1,103 @@ +// Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. See the LICENSE.md file in the project root for more information. + +using System.Reflection; + +namespace Microsoft.VisualStudio; + +public sealed class StringComparersTests +{ + [Fact] + public void StringComparersAndStringComparisonsMatch() + { + var flags = BindingFlags.Public | BindingFlags.Static; + + var comparers = typeof(StringComparers).GetProperties(flags).OrderBy(c => c.Name, StringComparer.Ordinal).ToList(); + var comparisons = typeof(StringComparisons).GetProperties(flags).OrderBy(c => c.Name, StringComparer.Ordinal).ToList(); + + var comparerNames = comparers.Select(c => c.Name).ToList(); + var comparisonNames = comparisons.Select(c => c.Name).ToList(); + + // Check that all comparers have a matching comparison. + // Include details about which ones are missing in the test failure message to make fixing easier. + var extraComparers = comparerNames.Except(comparisonNames, StringComparer.Ordinal).ToList(); + var extraComparisons = comparisonNames.Except(comparerNames, StringComparer.Ordinal).ToList(); + + if (extraComparers.Count + extraComparisons.Count != 0) + { + Assert.Fail($""" + Mismatched {nameof(StringComparers)} and {nameof(StringComparisons)}: + - Comparers without matching comparisons: {string.Join(", ", extraComparers)} + - Comparisons without matching comparers: {string.Join(", ", extraComparisons)} + """); + } + + var comparerValues = comparers.Select(c => (c.Name, Value: (StringComparer)c.GetValue(null)!)).ToList(); + var comparisonValues = comparisons.Select(c => (c.Name, Value: (StringComparison)c.GetValue(null)!)).ToList(); + + // Check that all comparer values match the corresponding comparison values. + foreach (var (comparer, comparison) in comparerValues.Zip(comparisonValues, (a, b) => (a, b))) + { + Assert.Equal(comparer.Name, comparison.Name, StringComparer.Ordinal); + + var comparerKind = GetComparerKind(comparer.Value); + var comparisonKind = GetComparisonKind(comparison.Value); + + if (!string.Equals(comparerKind, comparisonKind, StringComparison.Ordinal)) + { + Assert.Fail($""" + Mismatched comparisons: + - {nameof(StringComparers)}.{comparer.Name} = {comparerKind} + - {nameof(StringComparisons)}.{comparer.Name} = {comparisonKind} + """); + } + } + + static string GetComparerKind(StringComparer comparer) + { + foreach (var (c, name) in Comparers()) + { + if (Equals(c, comparer)) + { + return name; + } + } + + Assert.Fail("Unknown comparer: " + comparer); + return null!; // Unreachable + + static IEnumerable<(StringComparer, string)> Comparers() + { + yield return (StringComparer.Ordinal, nameof(StringComparer.Ordinal)); + yield return (StringComparer.OrdinalIgnoreCase, nameof(StringComparer.OrdinalIgnoreCase)); + yield return (StringComparer.CurrentCulture, nameof(StringComparer.CurrentCulture)); + yield return (StringComparer.CurrentCultureIgnoreCase, nameof(StringComparer.CurrentCultureIgnoreCase)); + yield return (StringComparer.InvariantCulture, nameof(StringComparer.InvariantCulture)); + yield return (StringComparer.InvariantCultureIgnoreCase, nameof(StringComparer.InvariantCultureIgnoreCase)); + } + } + + static string GetComparisonKind(StringComparison comparison) + { + foreach (var (c, name) in Comparisons()) + { + if (c == comparison) + { + return name; + } + } + + Assert.Fail("Unknown comparison: " + comparison); + return null!; // Unreachable + + static IEnumerable<(StringComparison, string)> Comparisons() + { + yield return (StringComparison.Ordinal, nameof(StringComparison.Ordinal)); + yield return (StringComparison.OrdinalIgnoreCase, nameof(StringComparison.OrdinalIgnoreCase)); + yield return (StringComparison.CurrentCulture, nameof(StringComparison.CurrentCulture)); + yield return (StringComparison.CurrentCultureIgnoreCase, nameof(StringComparison.CurrentCultureIgnoreCase)); + yield return (StringComparison.InvariantCulture, nameof(StringComparison.InvariantCulture)); + yield return (StringComparison.InvariantCultureIgnoreCase, nameof(StringComparison.InvariantCultureIgnoreCase)); + } + } + } +} From a7f0eb55cfc33f3b3445d62e9c856f5286297c42 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 29 Aug 2024 09:20:31 +1000 Subject: [PATCH 2/4] Make tests pass even when current culture is invariant --- .../StringComparisonTests.cs | 141 +++++++++++------- 1 file changed, 88 insertions(+), 53 deletions(-) diff --git a/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs b/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs index 1505b03122a..eed554c6045 100644 --- a/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs +++ b/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs @@ -1,5 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. See the LICENSE.md file in the project root for more information. +using System.Globalization; using System.Reflection; namespace Microsoft.VisualStudio; @@ -14,89 +15,123 @@ public void StringComparersAndStringComparisonsMatch() var comparers = typeof(StringComparers).GetProperties(flags).OrderBy(c => c.Name, StringComparer.Ordinal).ToList(); var comparisons = typeof(StringComparisons).GetProperties(flags).OrderBy(c => c.Name, StringComparer.Ordinal).ToList(); - var comparerNames = comparers.Select(c => c.Name).ToList(); - var comparisonNames = comparisons.Select(c => c.Name).ToList(); + var currentCulture = CultureInfo.CurrentCulture; + var currentUICulture = CultureInfo.CurrentUICulture; + var defaultThreadCurrentCulture = CultureInfo.DefaultThreadCurrentCulture; + var defaultThreadCurrentUICulture = CultureInfo.DefaultThreadCurrentUICulture; - // Check that all comparers have a matching comparison. - // Include details about which ones are missing in the test failure message to make fixing easier. - var extraComparers = comparerNames.Except(comparisonNames, StringComparer.Ordinal).ToList(); - var extraComparisons = comparisonNames.Except(comparerNames, StringComparer.Ordinal).ToList(); + try + { + // Temporarily set the culture to en-AU to ensure consistent results. + // This prevents test failures when the current culture is the invariant culture. + CultureInfo.CurrentCulture = new CultureInfo("en-AU"); + CultureInfo.CurrentUICulture = new CultureInfo("en-AU"); + CultureInfo.DefaultThreadCurrentCulture = new CultureInfo("en-AU"); + CultureInfo.DefaultThreadCurrentUICulture = new CultureInfo("en-AU"); + + ValidateSets(); + + ValidateValues(); + } + finally + { + CultureInfo.CurrentCulture = currentCulture; + CultureInfo.CurrentUICulture = currentUICulture; + CultureInfo.DefaultThreadCurrentCulture = defaultThreadCurrentCulture; + CultureInfo.DefaultThreadCurrentUICulture = defaultThreadCurrentUICulture; + } - if (extraComparers.Count + extraComparisons.Count != 0) + void ValidateSets() { - Assert.Fail($""" + var comparerNames = comparers.Select(c => c.Name).ToList(); + var comparisonNames = comparisons.Select(c => c.Name).ToList(); + + // Check that all comparers have a matching comparison. + // Include details about which ones are missing in the test failure message to make fixing easier. + var extraComparers = comparerNames.Except(comparisonNames, StringComparer.Ordinal).ToList(); + var extraComparisons = comparisonNames.Except(comparerNames, StringComparer.Ordinal).ToList(); + + if (extraComparers.Count + extraComparisons.Count != 0) + { + Assert.Fail($""" Mismatched {nameof(StringComparers)} and {nameof(StringComparisons)}: - Comparers without matching comparisons: {string.Join(", ", extraComparers)} - Comparisons without matching comparers: {string.Join(", ", extraComparisons)} """); + } } - var comparerValues = comparers.Select(c => (c.Name, Value: (StringComparer)c.GetValue(null)!)).ToList(); - var comparisonValues = comparisons.Select(c => (c.Name, Value: (StringComparison)c.GetValue(null)!)).ToList(); - - // Check that all comparer values match the corresponding comparison values. - foreach (var (comparer, comparison) in comparerValues.Zip(comparisonValues, (a, b) => (a, b))) + void ValidateValues() { - Assert.Equal(comparer.Name, comparison.Name, StringComparer.Ordinal); + var comparerValues = comparers.Select(c => (c.Name, Value: (StringComparer)c.GetValue(null)!)).ToList(); + var comparisonValues = comparisons.Select(c => (c.Name, Value: (StringComparison)c.GetValue(null)!)).ToList(); - var comparerKind = GetComparerKind(comparer.Value); - var comparisonKind = GetComparisonKind(comparison.Value); - - if (!string.Equals(comparerKind, comparisonKind, StringComparison.Ordinal)) + // Check that all comparer values match the corresponding comparison values. + foreach (var (comparer, comparison) in comparerValues.Zip(comparisonValues, (a, b) => (a, b))) { - Assert.Fail($""" + Assert.Equal(comparer.Name, comparison.Name, StringComparer.Ordinal); + + var comparerKind = GetComparerKind(comparer.Value); + var comparisonKind = GetComparisonKind(comparison.Value); + + if (!string.Equals(comparerKind, comparisonKind, StringComparison.Ordinal)) + { + Assert.Fail($""" Mismatched comparisons: - {nameof(StringComparers)}.{comparer.Name} = {comparerKind} - {nameof(StringComparisons)}.{comparer.Name} = {comparisonKind} """); + } } - } - static string GetComparerKind(StringComparer comparer) - { - foreach (var (c, name) in Comparers()) + return; + + static string GetComparerKind(StringComparer comparer) { - if (Equals(c, comparer)) + foreach (var (c, name) in Comparers()) { - return name; + if (Equals(c, comparer)) + { + return name; + } } - } - Assert.Fail("Unknown comparer: " + comparer); - return null!; // Unreachable + Assert.Fail("Unknown comparer: " + comparer); + return null!; // Unreachable - static IEnumerable<(StringComparer, string)> Comparers() - { - yield return (StringComparer.Ordinal, nameof(StringComparer.Ordinal)); - yield return (StringComparer.OrdinalIgnoreCase, nameof(StringComparer.OrdinalIgnoreCase)); - yield return (StringComparer.CurrentCulture, nameof(StringComparer.CurrentCulture)); - yield return (StringComparer.CurrentCultureIgnoreCase, nameof(StringComparer.CurrentCultureIgnoreCase)); - yield return (StringComparer.InvariantCulture, nameof(StringComparer.InvariantCulture)); - yield return (StringComparer.InvariantCultureIgnoreCase, nameof(StringComparer.InvariantCultureIgnoreCase)); + static IEnumerable<(StringComparer, string)> Comparers() + { + yield return (StringComparer.Ordinal, nameof(StringComparer.Ordinal)); + yield return (StringComparer.OrdinalIgnoreCase, nameof(StringComparer.OrdinalIgnoreCase)); + yield return (StringComparer.CurrentCulture, nameof(StringComparer.CurrentCulture)); + yield return (StringComparer.CurrentCultureIgnoreCase, nameof(StringComparer.CurrentCultureIgnoreCase)); + yield return (StringComparer.InvariantCulture, nameof(StringComparer.InvariantCulture)); + yield return (StringComparer.InvariantCultureIgnoreCase, nameof(StringComparer.InvariantCultureIgnoreCase)); + } } - } - static string GetComparisonKind(StringComparison comparison) - { - foreach (var (c, name) in Comparisons()) + static string GetComparisonKind(StringComparison comparison) { - if (c == comparison) + foreach (var (c, name) in Comparisons()) { - return name; + if (c == comparison) + { + return name; + } } - } - Assert.Fail("Unknown comparison: " + comparison); - return null!; // Unreachable + Assert.Fail("Unknown comparison: " + comparison); + return null!; // Unreachable - static IEnumerable<(StringComparison, string)> Comparisons() - { - yield return (StringComparison.Ordinal, nameof(StringComparison.Ordinal)); - yield return (StringComparison.OrdinalIgnoreCase, nameof(StringComparison.OrdinalIgnoreCase)); - yield return (StringComparison.CurrentCulture, nameof(StringComparison.CurrentCulture)); - yield return (StringComparison.CurrentCultureIgnoreCase, nameof(StringComparison.CurrentCultureIgnoreCase)); - yield return (StringComparison.InvariantCulture, nameof(StringComparison.InvariantCulture)); - yield return (StringComparison.InvariantCultureIgnoreCase, nameof(StringComparison.InvariantCultureIgnoreCase)); + static IEnumerable<(StringComparison, string)> Comparisons() + { + yield return (StringComparison.Ordinal, nameof(StringComparison.Ordinal)); + yield return (StringComparison.OrdinalIgnoreCase, nameof(StringComparison.OrdinalIgnoreCase)); + yield return (StringComparison.CurrentCulture, nameof(StringComparison.CurrentCulture)); + yield return (StringComparison.CurrentCultureIgnoreCase, nameof(StringComparison.CurrentCultureIgnoreCase)); + yield return (StringComparison.InvariantCulture, nameof(StringComparison.InvariantCulture)); + yield return (StringComparison.InvariantCultureIgnoreCase, nameof(StringComparison.InvariantCultureIgnoreCase)); + } } } } From d3df7564ec26c2ae2c028c9db7af7665c41f90df Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 29 Aug 2024 09:20:45 +1000 Subject: [PATCH 3/4] Typos in docs --- docs/build-acceleration.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/build-acceleration.md b/docs/build-acceleration.md index d56ba955324..4f54c2d983c 100644 --- a/docs/build-acceleration.md +++ b/docs/build-acceleration.md @@ -169,7 +169,7 @@ Looking through the build output with the following points in mind: ## Reference Assemblies -A reference assembly is a DLLs that models the public API of a project, without any actual implementation. +A reference assembly is a DLL that models the public API of a project, without any actual implementation. During build, reference assembly timestamps are only updated when their project's public API changes. Incremental build systems use file system timestamps for many of their optimisations. If project changes are internal-only (e.g. method bodies, private members added/removed, documentation changed) then the timestamp is not changed. Knowing the time at which a public API was last changed allows skipping some compilation. @@ -184,7 +184,7 @@ In our example, `A` only need to recompile if it has its own changes, or if `B`' Production of a reference assembly is controlled by the `ProduceReferenceAssembly` MSBuild property, and the feature is part of MSBuild directly. This means it works well outside of VS, in case you also do CLI builds. Note that most CI builds are non-incremental (they happen on fresh clones), so this property has no impact there. -When `ProduceReferenceAssembly` was introduced in .NET 5, it was only enabled by default for .NET 5 and later. We investigated changing the default for earlier frameworks too, but this caused issues in a very small number of highly customised builds and we take backwards compatability very seriously. That said, it's generally desirable to configure projects to produce reference assemblies, regardless of whether you use Build Acceleration or not. +When `ProduceReferenceAssembly` was introduced in .NET 5, it was only enabled by default for .NET 5 and later. We investigated changing the default for earlier frameworks too, but this caused issues in a very small number of highly customised builds and we take backwards compatibility very seriously. That said, it's generally desirable to configure projects to produce reference assemblies, regardless of whether you use Build Acceleration or not. For more information, see [Reference Assemblies](https://learn.microsoft.com/dotnet/standard/assembly/reference-assemblies) on Microsoft Learn. From 36921a6bba256ee08434b9dcf0ac2c2f79a49c6a Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 29 Aug 2024 09:21:46 +1000 Subject: [PATCH 4/4] Formatting --- .../StringComparisonTests.cs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs b/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs index eed554c6045..c1140a0f722 100644 --- a/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs +++ b/tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/StringComparisonTests.cs @@ -54,10 +54,10 @@ void ValidateSets() if (extraComparers.Count + extraComparisons.Count != 0) { Assert.Fail($""" - Mismatched {nameof(StringComparers)} and {nameof(StringComparisons)}: - - Comparers without matching comparisons: {string.Join(", ", extraComparers)} - - Comparisons without matching comparers: {string.Join(", ", extraComparisons)} - """); + Mismatched {nameof(StringComparers)} and {nameof(StringComparisons)}: + - Comparers without matching comparisons: {string.Join(", ", extraComparers)} + - Comparisons without matching comparers: {string.Join(", ", extraComparisons)} + """); } } @@ -77,10 +77,10 @@ void ValidateValues() if (!string.Equals(comparerKind, comparisonKind, StringComparison.Ordinal)) { Assert.Fail($""" - Mismatched comparisons: - - {nameof(StringComparers)}.{comparer.Name} = {comparerKind} - - {nameof(StringComparisons)}.{comparer.Name} = {comparisonKind} - """); + Mismatched comparisons: + - {nameof(StringComparers)}.{comparer.Name} = {comparerKind} + - {nameof(StringComparisons)}.{comparer.Name} = {comparisonKind} + """); } } @@ -97,7 +97,7 @@ static string GetComparerKind(StringComparer comparer) } Assert.Fail("Unknown comparer: " + comparer); - return null!; // Unreachable + throw Assumes.NotReachable(); static IEnumerable<(StringComparer, string)> Comparers() { @@ -121,7 +121,7 @@ static string GetComparisonKind(StringComparison comparison) } Assert.Fail("Unknown comparison: " + comparison); - return null!; // Unreachable + throw Assumes.NotReachable(); static IEnumerable<(StringComparison, string)> Comparisons() {