Add ability to ignore specific test cases #4457

Youssef1313 · 2024-12-27T09:21:57Z

TODO: Add tests. The tests should cover:

Both TestDataSourceUnfoldingStrategy.Unfold and TestDataSourceUnfoldingStrategy.Fold
Fixtures (test init/cleanup, class init/cleanup)

Youssef1313 · 2024-12-27T09:25:42Z

src/Adapter/MSTest.TestAdapter/ObjectModel/UnitTestElement.cs

-    /// <summary>
-    /// Gets or sets a value indicating whether the unit test should be ignored at run-time.
-    /// </summary>
-    public bool Ignored { get; set; }


This was never read (except in a unit test) and is adding noise to the code. The class is internal so I removed it altogether

Copilot reviewed 5 out of 19 changed files in this pull request and generated no comments.

Files not reviewed (14)

src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt: Language not supported
src/TestFramework/TestFramework/Attributes/TestMethod/TestClassAttribute.cs: Evaluated as low risk
src/TestFramework/TestFramework/Attributes/DataSource/DynamicDataAttribute.cs: Evaluated as low risk
src/Adapter/MSTest.TestAdapter/ObjectModel/UnitTestElement.cs: Evaluated as low risk
src/TestFramework/TestFramework/Attributes/DataSource/DataRowAttribute.cs: Evaluated as low risk
src/Adapter/MSTest.TestAdapter/ObjectModel/TestMethod.cs: Evaluated as low risk
src/Adapter/MSTest.TestAdapter/Execution/UnitTestRunner.cs: Evaluated as low risk
src/Adapter/MSTest.TestAdapter/Extensions/TestCaseExtensions.cs: Evaluated as low risk
src/Adapter/MSTest.TestAdapter/Extensions/TestResultExtensions.cs: Evaluated as low risk
src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs: Evaluated as low risk
test/UnitTests/MSTestAdapter.UnitTests/Execution/TestExecutionManagerTests.cs: Evaluated as low risk
src/Adapter/MSTest.TestAdapter/Constants.cs: Evaluated as low risk
test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs: Evaluated as low risk
src/TestFramework/TestFramework/Attributes/TestMethod/TestResult.cs: Evaluated as low risk

Comments suppressed due to low confidence (4)

src/Adapter/MSTest.TestAdapter/Execution/TestMethodRunner.cs:171

Ensure that tests are added to cover the new functionality of ignoring specific test cases based on the TestDataSourceIgnoreReason.

if (_test.TestDataSourceIgnoreReason is not null)

src/TestFramework/TestFramework/Attributes/TestMethod/TestMethodAttribute.cs:56

Ensure that the new Ignore property is covered by tests. The TODO in the pull request description indicates that tests are yet to be added.

public string? Ignore { get; set; }

src/Adapter/MSTest.TestAdapter/Execution/TestClassInfo.cs:571

The new behavior of ignoring the class cleanup should be covered by tests. Ensure that tests cover both TestDataSourceUnfoldingStrategy.Unfold and TestDataSourceUnfoldingStrategy.Fold.

if (ClassAttribute.Ignore is null &&

src/Adapter/MSTest.TestAdapter/Execution/TestClassInfo.cs:583

The new behavior of ignoring the class cleanup should be covered by tests. Ensure that tests cover both TestDataSourceUnfoldingStrategy.Unfold and TestDataSourceUnfoldingStrategy.Fold.

if (ClassAttribute.Ignore is null &&

Youssef1313 · 2024-12-27T09:32:09Z

src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt

 Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType.AutoDetect = 2 -> Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType
 *REMOVED*Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataAttribute.DynamicDataAttribute(string! dynamicDataSourceName, Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType dynamicDataSourceType = Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType.Property) -> void
 *REMOVED*Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataAttribute.DynamicDataAttribute(string! dynamicDataSourceName, System.Type! dynamicDataDeclaringType, Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType dynamicDataSourceType = Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType.Property) -> void
+Microsoft.VisualStudio.TestTools.UnitTesting.ITestDataSourceIgnoreCapability


TODO: Discuss the naming. Possible options:

Ignore

IgnoreReason

IgnoreMessage

Note that the property on the existing IgnoreAttribute is named IgnoreMessage. So we may want to keep the consistency.

I like the IgnoreMessage in case we start adding more "options" linked to ignoring a test (e.g. I like NUnit concept of Until on the [Ignore] attribute). I guess we could also think about creating a IgnoreOptions type (I never recall the limitations of types in attributes).

Evangelink · 2024-12-28T14:17:27Z

src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt

 Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType.AutoDetect = 2 -> Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType
 *REMOVED*Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataAttribute.DynamicDataAttribute(string! dynamicDataSourceName, Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType dynamicDataSourceType = Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType.Property) -> void
 *REMOVED*Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataAttribute.DynamicDataAttribute(string! dynamicDataSourceName, System.Type! dynamicDataDeclaringType, Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType dynamicDataSourceType = Microsoft.VisualStudio.TestTools.UnitTesting.DynamicDataSourceType.Property) -> void
+Microsoft.VisualStudio.TestTools.UnitTesting.ITestDataSourceIgnoreCapability


I like the IgnoreMessage in case we start adding more "options" linked to ignoring a test (e.g. I like NUnit concept of Until on the [Ignore] attribute). I guess we could also think about creating a IgnoreOptions type (I never recall the limitations of types in attributes).

Evangelink · 2024-12-28T14:18:30Z

src/TestFramework/TestFramework/Interfaces/ITestDataSourceIgnoreCapability.cs

+/// <summary>
+/// Specifies the capability of a test data source to be ignored and define the ignore reason.
+/// </summary>
+public interface ITestDataSourceIgnoreCapability


Could it be worth making this one more generic (not linked to data sources) so we can apply it to test methods and test classes too?

@Evangelink The impl is quite different, at least currently. For example, for specific test cases, we need to have a VSTest property for that to restore the associated ignore message. But we don't need that for when it's the whole test method or test class being ignored.

Evangelink · 2024-12-28T14:18:54Z

src/TestFramework/TestFramework/Attributes/DataSource/DynamicDataAttribute.cs

+    /// <summary>
+    /// Gets or sets a reason to ignore this dynamic data source. Setting the property to non-null value will ignore the dynamic data source.
+    /// </summary>


Suggested change

/// <summary>

/// Gets or sets a reason to ignore this dynamic data source. Setting the property to non-null value will ignore the dynamic data source.

/// </summary>

/// <inheritdoc />

I was intentionally not inheriting as I'm adjusting the wording a little bit for the specific concrete implementation. It's not super important though as the wording from the interface should still be clear enough for the concrete implementations.

Evangelink · 2024-12-28T14:18:58Z

src/TestFramework/TestFramework/Attributes/DataSource/DataRowAttribute.cs

+    /// <summary>
+    /// Gets or sets a reason to ignore the specific test case. Setting the property to non-null value will ignore the test case.
+    /// </summary>


Suggested change

/// <summary>

/// Gets or sets a reason to ignore the specific test case. Setting the property to non-null value will ignore the test case.

/// </summary>

/// <inheritdoc />

Evangelink · 2024-12-28T14:19:56Z

src/Adapter/MSTest.TestAdapter/Execution/TestMethodRunner.cs

+                            // This is closest to ignore. This enum doesn't have a value specific to Ignore.
+                            // It may be a better idea to add a value there, but the enum is public and we need to think more carefully before adding the API.
+                            // For now, TestResultExtensions.ToUnitTestResults method will convert this to Ignored value of ObjectModel enum when IgnoreReason is non-null.
+                            Outcome = UTF.UnitTestOutcome.NotRunnable,


It's probably good to have more states but I will have a closer look later.

Sure. Let's discuss later

Youssef1313 · 2024-12-29T09:15:13Z

src/Adapter/MSTest.TestAdapter/Execution/UnitTestRunner.cs

@@ -347,9 +347,17 @@ private bool IsTestMethodRunnable(
            }
        }

+        // TODO: Executor should never be null. Is it incorrectly annotated?
+        string? ignoreMessage = testMethodInfo.TestMethodOptions.Executor?.Ignore ?? testMethodInfo.Parent.ClassAttribute.Ignore;


TODO: I think we should check the class first, then the method.

Youssef1313 added 2 commits December 27, 2024 09:50

Initial work for ignoring specific test cases

158cba5

Progress

1bae8e7

Youssef1313 commented Dec 27, 2024

View reviewed changes

Youssef1313 requested a review from Copilot December 27, 2024 09:27

Copilot AI reviewed Dec 27, 2024

View reviewed changes

Youssef1313 commented Dec 27, 2024

View reviewed changes

Merge branch 'main' into ignore-specific-cases

4617364

Evangelink reviewed Dec 28, 2024

View reviewed changes

Evangelink added 2 commits December 28, 2024 15:41

Merge branch 'main' into ignore-specific-cases

e115201

Merge branch 'main' into ignore-specific-cases

b85a392

Youssef1313 commented Dec 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to ignore specific test cases #4457

Add ability to ignore specific test cases #4457

Youssef1313 commented Dec 27, 2024 •

edited

Loading

Youssef1313 Dec 27, 2024

Youssef1313 Dec 27, 2024

Evangelink Dec 28, 2024

Evangelink Dec 28, 2024

Evangelink Dec 28, 2024

Youssef1313 Dec 29, 2024

Evangelink Dec 28, 2024

Youssef1313 Dec 29, 2024

Evangelink Dec 28, 2024

Evangelink Dec 28, 2024

Youssef1313 Dec 29, 2024 •

edited

Loading

Youssef1313 Dec 29, 2024

Add ability to ignore specific test cases #4457

Are you sure you want to change the base?

Add ability to ignore specific test cases #4457

Conversation

Youssef1313 commented Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 commented Dec 27, 2024 •

edited

Loading

Youssef1313 Dec 29, 2024 •

edited

Loading