Add ability to ignore specific test cases #4457
Conversation
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 &&
| /// <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?
There was a problem hiding this comment.
@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.
| /// <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 /> |
There was a problem hiding this comment.
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.
| /// <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.
There was a problem hiding this comment.
Sure. Let's discuss later
|
@Evangelink This doesn't yet implement a new type that can be specialized under |
Fixes #1411