Add ability to ignore specific test cases #4457
Conversation
| /// <summary> | ||
| /// Gets or sets a value indicating whether the unit test should be ignored at run-time. | ||
| /// </summary> | ||
| public bool Ignored { get; set; } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Ignoreproperty 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 &&
| 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 |
There was a problem hiding this comment.
TODO: Discuss the naming. Possible options:
IgnoreIgnoreReasonIgnoreMessage
Note that the property on the existing IgnoreAttribute is named IgnoreMessage. So we may want to keep the consistency.
There was a problem hiding this comment.
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).
| 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 |
There was a problem hiding this comment.
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).
| /// <summary> | ||
| /// Specifies the capability of a test data source to be ignored and define the ignore reason. | ||
| /// </summary> | ||
| public interface ITestDataSourceIgnoreCapability |
There was a problem hiding this comment.
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?
| /// <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> |
There was a problem hiding this comment.
| /// <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 /> |
| /// <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> |
There was a problem hiding this comment.
| /// <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 /> |
| // 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, |
There was a problem hiding this comment.
It's probably good to have more states but I will have a closer look later.
Fixes #1411
TODO: Add tests. The tests should cover: