Update feature flag logic to disable only semantics

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

@@ -0,0 +1,58 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+#if !NETSTANDARD1_0
+
+using System;
+using System.Collections.Generic;
+
+namespace Microsoft.VisualStudio.TestPlatform.Utilities;
+
+/// <summary>
+/// !!!NEVER USE A FLAG TO ENABLE A 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 hosted directly inside the assembly that reference this one avoiding back compatibility issue.
+/// </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 DisableFeatureFlag : IDisableFeatureFlag
+{
+    private static readonly Dictionary<string, bool> FeatureFlags = new();
+
+    // NEVER USE A FLAG TO ENABLE A FUNCTIONALITY
+    private const string VSTEST_FEATURE_DISABLE = nameof(VSTEST_FEATURE_DISABLE);
+
+    public static IDisableFeatureFlag Instance { get; } = new DisableFeatureFlag();
+
+    static DisableFeatureFlag()
+    {
+        FeatureFlags.Add(DISABLE_ARTIFACTS_POSTPROCESSING, false);
+        FeatureFlags.Add(DISABLE_ARTIFACTS_POSTPROCESSING_NEWSDKUX, false);
+    }
+
+    // Added for artifact post-processing, it enable/disable the post processing.
+    // Added in 17.2-preview 7.0-preview
+    public const string DISABLE_ARTIFACTS_POSTPROCESSING = VSTEST_FEATURE_DISABLE + "_" + "ARTIFACTS_POSTPROCESSING";
+
+    // 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 const string DISABLE_ARTIFACTS_POSTPROCESSING_NEWSDKUX = VSTEST_FEATURE_DISABLE + "_" + "ARTIFACTS_POSTPROCESSING_NEWSDKUX";
+
+    // For now we're checking env var.
+    // We could add it also to some section inside the runsettings.
+    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 → src/Microsoft.TestPlatform.CoreUtilities/FeatureFlag/IDisableFeatureFlag.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
+internal interface IDisableFeatureFlag
 {
-    bool IsEnabled(string featureName);
+    bool IsDisabled(string featureName);
 }

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

@@ -36,29 +36,29 @@ internal class ArtifactProcessingManager : IArtifactProcessingManager
     private readonly string? _processArtifactFolder;
     private readonly IDataSerializer _dataSerialized;
     private readonly ITestRunAttachmentsProcessingEventsHandler _testRunAttachmentsProcessingEventsHandler;
-    private readonly IFeatureFlag _featureFlag;
+    private readonly IDisableFeatureFlag _disableFeatureFlag;
 
     public ArtifactProcessingManager(string testSessionCorrelationId) :
         this(testSessionCorrelationId,
             new FileHelper(),
             new TestRunAttachmentsProcessingManager(TestPlatformEventSource.Instance, new DataCollectorAttachmentsProcessorsFactory()),
             JsonDataSerializer.Instance,
             new PostProcessingTestRunAttachmentsProcessingEventsHandler(ConsoleOutput.Instance),
-            FeatureFlag.Instance)
+            DisableFeatureFlag.Instance)
     { }
 
     public ArtifactProcessingManager(string? testSessionCorrelationId,
         IFileHelper fileHelper!!,
         ITestRunAttachmentsProcessingManager testRunAttachmentsProcessingManager!!,
         IDataSerializer dataSerialized!!,
         ITestRunAttachmentsProcessingEventsHandler testRunAttachmentsProcessingEventsHandler!!,
-        IFeatureFlag featureFlag!!)
+        IDisableFeatureFlag disableFeatureFlag!!)
     {
         _fileHelper = fileHelper;
         _testRunAttachmentsProcessingManager = testRunAttachmentsProcessingManager;
         _dataSerialized = dataSerialized;
         _testRunAttachmentsProcessingEventsHandler = testRunAttachmentsProcessingEventsHandler;
-        _featureFlag = featureFlag;
+        _disableFeatureFlag = disableFeatureFlag;
 
         // We don't validate for null, it's expected, we'll have testSessionCorrelationId only in case of .NET SDK run.
         if (testSessionCorrelationId is not null)
@@ -71,7 +71,7 @@ public ArtifactProcessingManager(string? testSessionCorrelationId,
 
     public void CollectArtifacts(TestRunCompleteEventArgs testRunCompleteEventArgs!!, string runSettingsXml!!)
     {
-        if (!_featureFlag.IsEnabled(FeatureFlag.ARTIFACTS_POSTPROCESSING))
+        if (_disableFeatureFlag.IsDisabled(DisableFeatureFlag.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 (_disableFeatureFlag.IsDisabled(DisableFeatureFlag.DISABLE_ARTIFACTS_POSTPROCESSING))
         {
             EqtTrace.Verbose("ArtifactProcessingManager.PostProcessArtifacts: Feature disabled");
             return;

src/vstest.console/Internal/ConsoleLogger.cs

@@ -119,11 +119,11 @@ public ConsoleLogger()
     /// <summary>
     /// Constructor added for testing purpose
     /// </summary>
-    internal ConsoleLogger(IOutput output, IProgressIndicator progressIndicator, IFeatureFlag featureFlag)
+    internal ConsoleLogger(IOutput output, IProgressIndicator progressIndicator, IDisableFeatureFlag disableFeatureFlag)
     {
         Output = output;
         _progressIndicator = progressIndicator;
-        _featureFlag = featureFlag;
+        _disableFeatureFlag = disableFeatureFlag;
     }
 
     /// <summary>
@@ -138,7 +138,7 @@ protected static IOutput Output
 
     private IProgressIndicator _progressIndicator;
 
-    private readonly IFeatureFlag _featureFlag = FeatureFlag.Instance;
+    private readonly IDisableFeatureFlag _disableFeatureFlag = DisableFeatureFlag.Instance;
 
     /// <summary>
     /// Get the verbosity level for the console logger
@@ -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 (_disableFeatureFlag.IsDisabled(DisableFeatureFlag.DISABLE_ARTIFACTS_POSTPROCESSING) ||
+                // DISABLE_ARTIFACTS_POSTPROCESSING_NEWSDKUX(old UX) is disabled
+                _disableFeatureFlag.IsDisabled(DisableFeatureFlag.DISABLE_ARTIFACTS_POSTPROCESSING_NEWSDKUX) ||
                 // TestSessionCorrelationId is null(we're not running through the dotnet SDK).
                 CommandLineOptions.Instance.TestSessionCorrelationId is null)
             {

src/vstest.console/Processors/ArtifactProcessingPostProcessModeProcessor.cs

@@ -44,8 +44,8 @@ public Lazy<IArgumentExecutor> Executor
         set => _executor = value;
     }
 
-    public static bool ContainsPostProcessCommand(string[]? args, IFeatureFlag? featureFlag = null)
-        => (featureFlag ?? FeatureFlag.Instance).IsEnabled(FeatureFlag.ARTIFACTS_POSTPROCESSING) &&
+    public static bool ContainsPostProcessCommand(string[]? args, IDisableFeatureFlag? featureFlag = null)
+        => !(featureFlag ?? DisableFeatureFlag.Instance).IsDisabled(DisableFeatureFlag.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(IDisableFeatureFlag disableFeatureFlag = null)
     {
         var defaultArgumentProcessor = DefaultArgumentProcessors;
 
-        if ((featureFlag ?? FeatureFlag.Instance).IsEnabled(FeatureFlag.ARTIFACTS_POSTPROCESSING))
+        if (!(disableFeatureFlag ?? DisableFeatureFlag.Instance).IsDisabled(DisableFeatureFlag.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.CoreUtilities.UnitTests/FeatureFlag/FeatureFlagTests.cs → test/Microsoft.TestPlatform.CoreUtilities.UnitTests/FeatureFlag/DisableFeatureFlagTests.cs

@@ -7,11 +7,11 @@
 namespace Microsoft.TestPlatform.CoreUtilities.UnitTests;
 
 [TestClass]
-public class FeatureFlagTests
+public class DisableFeatureFlagTests
 {
     [TestMethod]
     public void SingletonAlwaysReturnsTheSameInstance()
     {
-        Assert.IsTrue(ReferenceEquals(FeatureFlag.Instance, FeatureFlag.Instance));
+        Assert.IsTrue(ReferenceEquals(DisableFeatureFlag.Instance, DisableFeatureFlag.Instance));
     }
 }

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

@@ -31,14 +31,14 @@ public class ArtifactProcessingTests
     private readonly Mock<IFileHelper> _fileHelperMock = new();
     private readonly Mock<ITestRunAttachmentsProcessingManager> _testRunAttachmentsProcessingManagerMock = new();
     private readonly Mock<ITestRunAttachmentsProcessingEventsHandler> _testRunAttachmentsProcessingEventsHandlerMock = new();
-    private readonly Mock<IFeatureFlag> _featureFlagMock = new();
+    private readonly Mock<IDisableFeatureFlag> _featureFlagMock = new();
     private readonly Mock<IDataSerializer> _dataSerializer = new();
     private readonly Mock<ITestRunStatistics> _testRunStatistics = new();
     private ArtifactProcessingManager _artifactProcessingManager;
 
     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

@@ -38,7 +38,7 @@ public class ConsoleLoggerTests
     private Mock<IOutput> _mockOutput;
     private ConsoleLogger _consoleLogger;
     private Mock<IProgressIndicator> _mockProgressIndicator;
-    private Mock<IFeatureFlag> _mockFeatureFlag;
+    private Mock<IDisableFeatureFlag> _mockFeatureFlag;
 
     private const string PassedTestIndicator = "  Passed ";
     private const string FailedTestIndicator = "  Failed ";
@@ -1237,8 +1237,8 @@ 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 = new Mock<IDisableFeatureFlag>();
+        _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

@@ -16,7 +16,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CommandLine.UnitTests.Processors;
 public class ArtifactProcessingPostProcessModeProcessorTest
 {
     private readonly Mock<IArtifactProcessingManager> _artifactProcessingManagerMock = new();
-    private readonly Mock<IFeatureFlag> _featureFlagMock = new();
+    private readonly Mock<IDisableFeatureFlag> _featureFlagMock = new();
 
     [TestMethod]
     public void ProcessorExecutorInitialize_ShouldFailIfNullCtor()
@@ -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

@@ -152,8 +152,8 @@ public void BuildCommadMapsForMultipleProcessorAddsProcessorToAppropriateMaps()
             xplatShortCommandName.Add(name.Replace('/', '-'));
         }
 
-        Mock<IFeatureFlag> featureFlag = new();
-        featureFlag.Setup(x => x.IsEnabled(It.IsAny<string>())).Returns(true);
+        Mock<IDisableFeatureFlag> featureFlag = new();
+        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.