Update feature flag logic to disable only semantics

src/Microsoft.TestPlatform.CoreUtilities/FeatureFlag/FeatureFlag.cs

@@ -8,36 +8,50 @@
 
 namespace Microsoft.VisualStudio.TestPlatform.Utilities;
 
+/// <summary>
+/// !!!NEVER USE A FLAG TO ENABLE FUNCTIONALITY!!!
+/// 
+/// The reasoning is:
+/// 
+/// * New version will automatically ship with the feature enabled. There is no action needed to be done just before release.
+/// * Anyone interested in the new feature will get it automatically by grabbing our preview.
+/// * Anyone who needs more time before using the new feature can disable it in the released package.
+/// * If we remove the flag from our code, we will do it after the feature is the new default, or after the functionality was completely removed from our codebase.
+/// * If there is a very outdated version of an assembly that for some reason loaded with the newest version of TP and it cannot find a feature flag, because we removed that feature flag in the meantime, it will just run with all it's newest features enabled.
+///
+/// Use constants so the feature name will be compiled directly into the assembly that references this, to avoid backwards compatibility issues, when the flag is removed in newer version.
+/// </summary>
+
 // !!! FEATURES MUST BE KEPT IN SYNC WITH https://github.com/dotnet/sdk/blob/main/src/Cli/dotnet/commands/dotnet-test/VSTestFeatureFlag.cs !!!
 internal partial class FeatureFlag : IFeatureFlag
 {
     private static readonly Dictionary<string, bool> FeatureFlags = new();
 
-    private const string VSTEST_FEATURE = nameof(VSTEST_FEATURE);
+    private const string VSTEST_ = nameof(VSTEST_);
 
     public static IFeatureFlag Instance { get; } = new FeatureFlag();
 
     static FeatureFlag()
     {
-        FeatureFlags.Add(ARTIFACTS_POSTPROCESSING, true);
-        FeatureFlags.Add(ARTIFACTS_POSTPROCESSING_SDK_KEEP_OLD_UX, false);
+        FeatureFlags.Add(DISABLE_ARTIFACTS_POSTPROCESSING, false);
+        FeatureFlags.Add(DISABLE_ARTIFACTS_POSTPROCESSING_NEW_SDK_UX, false);
     }
 
-    // Added for artifact porst-processing, it enable/disable the post processing.
+    // Added for artifact post-processing, it enable/disable the post processing.
     // Added in 17.2-preview 7.0-preview
-    public static string ARTIFACTS_POSTPROCESSING = VSTEST_FEATURE + "_" + "ARTIFACTS_POSTPROCESSING";
+    public const string DISABLE_ARTIFACTS_POSTPROCESSING = VSTEST_ + "_" + nameof(DISABLE_ARTIFACTS_POSTPROCESSING);
 
-    // Added for artifact porst-processing, it will show old output for dotnet sdk scenario.
+    // Added for artifact post-processing, it will show old output for dotnet sdk scenario.
     // It can be useful if we need to restore old UX in case users are parsing the console output.
     // Added in 17.2-preview 7.0-preview
-    public static string ARTIFACTS_POSTPROCESSING_SDK_KEEP_OLD_UX = VSTEST_FEATURE + "_" + "ARTIFACTS_POSTPROCESSING_SDK_KEEP_OLD_UX";
+    public const string DISABLE_ARTIFACTS_POSTPROCESSING_NEW_SDK_UX = VSTEST_ + "_" + nameof(DISABLE_ARTIFACTS_POSTPROCESSING_NEW_SDK_UX);
 
     // For now we're checking env var.
     // We could add it also to some section inside the runsettings.
-    public bool IsEnabled(string featureName) =>
-        int.TryParse(Environment.GetEnvironmentVariable(featureName), out int enabled) ?
-        enabled == 1 :
-        FeatureFlags.TryGetValue(featureName, out bool isEnabled) && isEnabled;
+    public bool IsDisabled(string featureName) =>
+        int.TryParse(Environment.GetEnvironmentVariable(featureName), out int disabled) ?
+        disabled == 1 :
+        FeatureFlags.TryGetValue(featureName, out bool isDisabled) && isDisabled;
 }
 
 #endif

src/Microsoft.TestPlatform.CoreUtilities/FeatureFlag/IFeatureFlag.cs

@@ -1,11 +1,9 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
-#nullable disable
-
 namespace Microsoft.VisualStudio.TestPlatform.Utilities;
 
 internal interface IFeatureFlag
 {
-    bool IsEnabled(string featureName);
+    bool IsDisabled(string featureName);
 }

src/Microsoft.TestPlatform.CrossPlatEngine/PostProcessing/ArtifactProcessingManager.cs

@@ -71,7 +71,7 @@ public ArtifactProcessingManager(string? testSessionCorrelationId,
 
     public void CollectArtifacts(TestRunCompleteEventArgs testRunCompleteEventArgs!!, string runSettingsXml!!)
     {
-        if (!_featureFlag.IsEnabled(FeatureFlag.ARTIFACTS_POSTPROCESSING))
+        if (_featureFlag.IsDisabled(FeatureFlag.DISABLE_ARTIFACTS_POSTPROCESSING))
         {
             EqtTrace.Verbose("ArtifactProcessingManager.CollectArtifacts: Feature disabled");
             return;
@@ -107,7 +107,7 @@ public void CollectArtifacts(TestRunCompleteEventArgs testRunCompleteEventArgs!!
 
     public async Task PostProcessArtifactsAsync()
     {
-        if (!_featureFlag.IsEnabled(FeatureFlag.ARTIFACTS_POSTPROCESSING))
+        if (_featureFlag.IsDisabled(FeatureFlag.DISABLE_ARTIFACTS_POSTPROCESSING))
         {
             EqtTrace.Verbose("ArtifactProcessingManager.PostProcessArtifacts: Feature disabled");
             return;

src/vstest.console/Internal/ConsoleLogger.cs

@@ -665,9 +665,9 @@ private void TestRunCompleteHandler(object sender, TestRunCompleteEventArgs e)
         if (runLevelAttachementCount > 0)
         {
             // If ARTIFACTS_POSTPROCESSING is disabled
-            if (!_featureFlag.IsEnabled(FeatureFlag.ARTIFACTS_POSTPROCESSING) ||
-                // ARTIFACTS_POSTPROCESSING_SDK_KEEP_OLD_UX(old UX) is enabled
-                _featureFlag.IsEnabled(FeatureFlag.ARTIFACTS_POSTPROCESSING_SDK_KEEP_OLD_UX) ||
+            if (_featureFlag.IsDisabled(FeatureFlag.DISABLE_ARTIFACTS_POSTPROCESSING) ||
+                // DISABLE_ARTIFACTS_POSTPROCESSING_NEW_SDK_UX(new UX) is disabled
+                _featureFlag.IsDisabled(FeatureFlag.DISABLE_ARTIFACTS_POSTPROCESSING_NEW_SDK_UX) ||
                 // TestSessionCorrelationId is null(we're not running through the dotnet SDK).
                 CommandLineOptions.Instance.TestSessionCorrelationId is null)
             {

src/vstest.console/Processors/ArtifactProcessingPostProcessModeProcessor.cs

@@ -45,7 +45,7 @@ public Lazy<IArgumentExecutor> Executor
     }
 
     public static bool ContainsPostProcessCommand(string[]? args, IFeatureFlag? featureFlag = null)
-        => (featureFlag ?? FeatureFlag.Instance).IsEnabled(FeatureFlag.ARTIFACTS_POSTPROCESSING) &&
+        => !(featureFlag ?? FeatureFlag.Instance).IsDisabled(FeatureFlag.DISABLE_ARTIFACTS_POSTPROCESSING) &&
             (args?.Contains("--artifactsProcessingMode-postprocess", StringComparer.OrdinalIgnoreCase) == true ||
             args?.Contains(CommandName, StringComparer.OrdinalIgnoreCase) == true);
 }

src/vstest.console/Processors/Utilities/ArgumentProcessorFactory.cs

@@ -58,11 +58,11 @@ protected ArgumentProcessorFactory(IEnumerable<IArgumentProcessor> argumentProce
     /// The feature flag support.
     /// </param>
     /// <returns>ArgumentProcessorFactory.</returns>
-    internal static ArgumentProcessorFactory Create(IFeatureFlag featureFlag = null)
+    internal static ArgumentProcessorFactory Create(IFeatureFlag disableFeatureFlag = null)
     {
         var defaultArgumentProcessor = DefaultArgumentProcessors;
 
-        if ((featureFlag ?? FeatureFlag.Instance).IsEnabled(FeatureFlag.ARTIFACTS_POSTPROCESSING))
+        if (!(disableFeatureFlag ?? FeatureFlag.Instance).IsDisabled(FeatureFlag.DISABLE_ARTIFACTS_POSTPROCESSING))
         {
             defaultArgumentProcessor.Add(new ArtifactProcessingCollectModeProcessor());
             defaultArgumentProcessor.Add(new ArtifactProcessingPostProcessModeProcessor());

test/Microsoft.TestPlatform.AcceptanceTests/PostProcessingTests.cs

@@ -55,14 +55,12 @@ public void DotnetSDKSimulation_PostProcessing()
 
             string testContainer = Directory.GetFiles(Path.Combine(projectFolder, "bin"), $"{i}.dll", SearchOption.AllDirectories).Single();
 
-            ExecuteVsTestConsole($"{testContainer} --Collect:\"SampleDataCollector\" --TestAdapterPath:\"{extensionsPath}\" --ResultsDirectory:\"{Path.GetDirectoryName(testContainer)}\" --Settings:\"{runSettings}\" --ArtifactsProcessingMode-Collect --TestSessionCorrelationId:\"{correlationSessionId}\" --Diag:\"{TempDirectory.Path + '/'}\"", out stdOut, out stdError, out exitCode,
-                new Dictionary<string, string>() { { "VSTEST_FEATURE_ARTIFACTS_POSTPROCESSING", "1" } });
+            ExecuteVsTestConsole($"{testContainer} --Collect:\"SampleDataCollector\" --TestAdapterPath:\"{extensionsPath}\" --ResultsDirectory:\"{Path.GetDirectoryName(testContainer)}\" --Settings:\"{runSettings}\" --ArtifactsProcessingMode-Collect --TestSessionCorrelationId:\"{correlationSessionId}\" --Diag:\"{TempDirectory.Path + '/'}\"", out stdOut, out stdError, out exitCode);
             Assert.AreEqual(exitCode, 0);
         });
 
         // Post process artifacts
-        ExecuteVsTestConsole($"--ArtifactsProcessingMode-PostProcess --TestSessionCorrelationId:\"{correlationSessionId}\" --Diag:\"{TempDirectory.Path + '/'}\"", out string stdOut, out string stdError, out int exitCode,
-                new Dictionary<string, string>() { { "VSTEST_FEATURE_ARTIFACTS_POSTPROCESSING", "1" } });
+        ExecuteVsTestConsole($"--ArtifactsProcessingMode-PostProcess --TestSessionCorrelationId:\"{correlationSessionId}\" --Diag:\"{TempDirectory.Path + '/'}\"", out string stdOut, out string stdError, out int exitCode);
         Assert.AreEqual(exitCode, 0);
 
         using StringReader reader = new(stdOut);

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/PostProcessing/ArtifactProcessingTests.cs

@@ -38,7 +38,7 @@ public class ArtifactProcessingTests
 
     public ArtifactProcessingTests()
     {
-        _featureFlagMock.Setup(x => x.IsEnabled(It.IsAny<string>())).Returns(true);
+        _featureFlagMock.Setup(x => x.IsDisabled(It.IsAny<string>())).Returns(false);
         _fileHelperMock.Setup(x => x.GetTempPath()).Returns("/tmp");
 
         _artifactProcessingManager =

test/vstest.console.UnitTests/Internal/ConsoleLoggerTests.cs

@@ -1238,7 +1238,7 @@ private void Setup()
         _mockRequestData = new Mock<IRequestData>();
         _mockMetricsCollection = new Mock<IMetricsCollection>();
         _mockFeatureFlag = new Mock<IFeatureFlag>();
-        _mockFeatureFlag.Setup(x => x.IsEnabled(It.IsAny<string>())).Returns(true);
+        _mockFeatureFlag.Setup(x => x.IsDisabled(It.IsAny<string>())).Returns(false);
         _mockRequestData.Setup(rd => rd.MetricsCollection).Returns(_mockMetricsCollection.Object);
 
         _mockOutput = new Mock<IOutput>();

test/vstest.console.UnitTests/Processors/ArtifactProcessingPostProcessModeProcessorTest.cs

@@ -63,7 +63,7 @@ public void ProcessorExecutorInitialize_ExceptionShouldNotBubbleUp()
     [TestMethod]
     public void ArtifactProcessingPostProcessMode_ContainsPostProcessCommand()
     {
-        _featureFlagMock.Setup(x => x.IsEnabled(It.IsAny<string>())).Returns(true);
+        _featureFlagMock.Setup(x => x.IsDisabled(It.IsAny<string>())).Returns(false);
         Assert.IsTrue(ArtifactProcessingPostProcessModeProcessor.ContainsPostProcessCommand(new string[] { "--artifactsProcessingMode-postprocess" }, _featureFlagMock.Object));
         Assert.IsTrue(ArtifactProcessingPostProcessModeProcessor.ContainsPostProcessCommand(new string[] { "--ARTIfactsProcessingMode-postprocess" }, _featureFlagMock.Object));
         Assert.IsFalse(ArtifactProcessingPostProcessModeProcessor.ContainsPostProcessCommand(new string[] { "-ARTIfactsProcessingMode-postprocess" }, _featureFlagMock.Object));

test/vstest.console.UnitTests/Processors/Utilities/ArgumentProcessorFactoryTests.cs

@@ -153,7 +153,7 @@ public void BuildCommadMapsForMultipleProcessorAddsProcessorToAppropriateMaps()
         }
 
         Mock<IFeatureFlag> featureFlag = new();
-        featureFlag.Setup(x => x.IsEnabled(It.IsAny<string>())).Returns(true);
+        featureFlag.Setup(x => x.IsDisabled(It.IsAny<string>())).Returns(false);
         ArgumentProcessorFactory factory = ArgumentProcessorFactory.Create(featureFlag.Object);
 
         // Expect command processors to contain both long and short commands.