-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Begin transition to Microsoft.CodeAnalysis.Testing #41717
Conversation
sharwell
commented
Feb 15, 2020
- Add new verification helpers
- Convert AddAccessibilityModifiersTests to the new test library
} | ||
|
||
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] | ||
public async Task TestRefStructs() | ||
{ | ||
await TestInRegularAndScriptAsync(@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing in script code is not currently supported, because diagnostics in script code fail to include the document path in the diagnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine with me.
src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs
Show resolved
Hide resolved
src/EditorFeatures/TestUtilities/CodeActions/CSharpCodeFixVerifier`2+Test.cs
Outdated
Show resolved
Hide resolved
81ea310
to
ec82406
Compare
src/EditorFeatures/TestUtilities/CodeActions/CSharpCodeFixVerifier`2+Test.cs
Show resolved
Hide resolved
src/EditorFeatures/TestUtilities/CodeActions/VisualBasicCodeFixVerifier`2+Test.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/TestUtilities/CodeActions/VisualBasicCodeFixVerifier`2+Test.cs
Show resolved
Hide resolved
src/EditorFeatures/TestUtilities/CodeActions/VisualBasicCodeFixVerifier`2.cs
Show resolved
Hide resolved
namespace Outer | ||
{ | ||
namespace Inner1.Inner2 | ||
{ | ||
internal class {|FixAllInDocument:C|} | ||
internal partial class [|C|] : I |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why partial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➡️ Compile errors were fixed in ec82406 to allow for a constrained review. This change addresses the CS0751 error.
options = options.WithChangedOption(key, value); | ||
} | ||
|
||
solution = solution.WithOptions(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this approach of options testing will not work for analyzers ported to CodeStyle layer. Solution/workspace options are currently not passed down to the AnalyzerConfigOptionSet
which means they will be unavailable in CI. I understand we would like to eventually pass these down, but until that is implemented, these tests will not work in code style layer. This led to me eventually use the approach here: https://github.com/dotnet/roslyn/pull/41363/files#r378505952.
I can confirm this once this PR goes in, and I will try to rebase #41363 on top of this verifier. If required, I will port similar logic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this approach of options testing will not work for analyzers ported to CodeStyle layer
I traced the original implementation through the base test classes and mirrored the approach (i.e. this is a direct translation of the original tests). Changes may be required for the CodeStyle layer, but even if necessary would be constrained to the two helpers.
|
||
protected override AnalyzerOptions GetAnalyzerOptions(Project project) | ||
{ | ||
return new WorkspaceAnalyzerOptions(base.GetAnalyzerOptions(project), project.Solution); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: WorkspaceAnalyzerOptions
type is not available in CodeStyle layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WorkspaceAnalyzerOptions is required for the current analyzer implementations. Translation to CodeStyle layer equivalents would be left for the porting process.
private IDictionary<OptionKey, object> OmitDefaultModifiers => | ||
OptionsSet(SingleOption(CodeStyleOptions.RequireAccessibilityModifiers, AccessibilityModifiersRequired.OmitIfDefault, NotificationOption.Suggestion)); | ||
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] | ||
public void TestStandardProperties() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use the approach used in current test framework where there is a common base type for all diagnostics tests which have these common tests, so that every new analyzer gets these tests without having to explicitly remember to author them:
Lines 56 to 119 in e704ca6
[Fact] | |
public void TestSupportedDiagnosticsMessageTitle() | |
{ | |
using (var workspace = new AdhocWorkspace()) | |
{ | |
var diagnosticAnalyzer = CreateDiagnosticProviderAndFixer(workspace).Item1; | |
if (diagnosticAnalyzer == null) | |
{ | |
return; | |
} | |
foreach (var descriptor in diagnosticAnalyzer.SupportedDiagnostics) | |
{ | |
if (descriptor.CustomTags.Contains(WellKnownDiagnosticTags.NotConfigurable)) | |
{ | |
// The title only displayed for rule configuration | |
continue; | |
} | |
Assert.NotEqual("", descriptor.Title?.ToString() ?? ""); | |
} | |
} | |
} | |
[Fact] | |
public void TestSupportedDiagnosticsMessageDescription() | |
{ | |
using (var workspace = new AdhocWorkspace()) | |
{ | |
var diagnosticAnalyzer = CreateDiagnosticProviderAndFixer(workspace).Item1; | |
if (diagnosticAnalyzer == null) | |
{ | |
return; | |
} | |
foreach (var descriptor in diagnosticAnalyzer.SupportedDiagnostics) | |
{ | |
if (ShouldSkipMessageDescriptionVerification(descriptor)) | |
{ | |
continue; | |
} | |
Assert.NotEqual("", descriptor.MessageFormat?.ToString() ?? ""); | |
} | |
} | |
} | |
[Fact(Skip = "https://github.com/dotnet/roslyn/issues/26717")] | |
public void TestSupportedDiagnosticsMessageHelpLinkUri() | |
{ | |
using (var workspace = new AdhocWorkspace()) | |
{ | |
var diagnosticAnalyzer = CreateDiagnosticProviderAndFixer(workspace).Item1; | |
if (diagnosticAnalyzer == null) | |
{ | |
return; | |
} | |
foreach (var descriptor in diagnosticAnalyzer.SupportedDiagnostics) | |
{ | |
Assert.NotEqual("", descriptor.HelpLinkUri ?? ""); | |
} | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are covered by one new test in each analyzer test class:
[Fact]
public void TestStandardProperties()
=> VerifyCS.VerifyStandardProperties();
If we have a problem remembering to add this test, we can add an analyzer for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 If we want to break out this test into each of the individual properties to match the old code, we could use this instead:
[Fact, CombinatorialData]
public void TestStandardProperty(StandardAnalyzerProperty property)
=> VerifyCS.VerifyStandardProperty(property);
public enum StandardAnalyzerProperty {
Title,
Message,
HelpLinkUri,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks very promising step forward, though we might need more tweaks as we support more scenarios.