-
Notifications
You must be signed in to change notification settings - Fork 468
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
Enable FixAll testing by default for all code fix unit tests #1422
Closed
Closed
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,19 +34,19 @@ protected void VerifyCSharpFix(string oldSource, string newSource, int? codeFixI | |
VerifyFix(LanguageNames.CSharp, GetCSharpDiagnosticAnalyzer(), GetCSharpCodeFixProvider(), oldSource, newSource, codeFixIndex, allowNewCompilerDiagnostics, onlyFixFirstFixableDiagnostic, validationMode, false); | ||
} | ||
|
||
protected void VerifyCSharpFixAll(string oldSource, string newSource, bool allowNewCompilerDiagnostics = false, TestValidationMode validationMode = DefaultTestValidationMode) | ||
protected void VerifyCSharpFixAll(string oldSource, string newSource, int? codeFixIndex = null, bool allowNewCompilerDiagnostics = false, TestValidationMode validationMode = DefaultTestValidationMode) | ||
{ | ||
VerifyFixAll(LanguageNames.CSharp, GetCSharpDiagnosticAnalyzer(), GetCSharpCodeFixProvider(), oldSource, newSource, allowNewCompilerDiagnostics, validationMode, false); | ||
VerifyFixAll(LanguageNames.CSharp, GetCSharpDiagnosticAnalyzer(), GetCSharpCodeFixProvider(), oldSource, newSource, codeFixIndex, allowNewCompilerDiagnostics, validationMode, false); | ||
} | ||
|
||
protected void VerifyBasicFix(string oldSource, string newSource, int? codeFixIndex = null, bool allowNewCompilerDiagnostics = false, bool onlyFixFirstFixableDiagnostic = false, TestValidationMode validationMode = DefaultTestValidationMode) | ||
{ | ||
VerifyFix(LanguageNames.VisualBasic, GetBasicDiagnosticAnalyzer(), GetBasicCodeFixProvider(), oldSource, newSource, codeFixIndex, allowNewCompilerDiagnostics, onlyFixFirstFixableDiagnostic, validationMode, false); | ||
} | ||
|
||
protected void VerifyBasicFixAll(string oldSource, string newSource, bool allowNewCompilerDiagnostics = false, TestValidationMode validationMode = DefaultTestValidationMode) | ||
protected void VerifyBasicFixAll(string oldSource, string newSource, int? codeFixIndex = null, bool allowNewCompilerDiagnostics = false, TestValidationMode validationMode = DefaultTestValidationMode) | ||
{ | ||
VerifyFixAll(LanguageNames.VisualBasic, GetBasicDiagnosticAnalyzer(), GetBasicCodeFixProvider(), oldSource, newSource, allowNewCompilerDiagnostics, validationMode, false); | ||
VerifyFixAll(LanguageNames.VisualBasic, GetBasicDiagnosticAnalyzer(), GetBasicCodeFixProvider(), oldSource, newSource, codeFixIndex, allowNewCompilerDiagnostics, validationMode, false); | ||
} | ||
|
||
private void VerifyFix(string language, DiagnosticAnalyzer analyzerOpt, CodeFixProvider codeFixProvider, string oldSource, string newSource, int? codeFixIndex, bool allowNewCompilerDiagnostics, bool onlyFixFirstFixableDiagnostic, TestValidationMode validationMode, bool allowUnsafeCode) | ||
|
@@ -57,12 +57,16 @@ private void VerifyFix(string language, DiagnosticAnalyzer analyzerOpt, CodeFixP | |
VerifyFix(document, analyzerOpt, codeFixProvider, newSource, newSourceFileName, ImmutableArray<TestAdditionalDocument>.Empty, codeFixIndex, allowNewCompilerDiagnostics, onlyFixFirstFixableDiagnostic, validationMode); | ||
} | ||
|
||
private void VerifyFixAll(string language, DiagnosticAnalyzer analyzerOpt, CodeFixProvider codeFixProvider, string oldSource, string newSource, bool allowNewCompilerDiagnostics, TestValidationMode validationMode, bool allowUnsafeCode) | ||
private void VerifyFixAll(string language, DiagnosticAnalyzer analyzerOpt, CodeFixProvider codeFixProvider, string oldSource, string newSource, int? codeFixIndex, bool allowNewCompilerDiagnostics, TestValidationMode validationMode, bool allowUnsafeCode) | ||
{ | ||
Document document = CreateDocument(oldSource, language, allowUnsafeCode: allowUnsafeCode); | ||
var newSourceFileName = document.Name; | ||
var additionalFiles = ImmutableArray<TestAdditionalDocument>.Empty; | ||
var testFixAll = true; | ||
var executeSingleFixOrFixAllPass = true; | ||
|
||
VerifyFixAll(document, analyzerOpt, codeFixProvider, newSource, newSourceFileName, ImmutableArray<TestAdditionalDocument>.Empty, allowNewCompilerDiagnostics, validationMode); | ||
VerifyFixOrFixAllCore(document, analyzerOpt, codeFixProvider, newSource, newSourceFileName, additionalFiles, | ||
codeFixIndex, allowNewCompilerDiagnostics, validationMode, testFixAll, executeSingleFixOrFixAllPass); | ||
} | ||
|
||
protected void VerifyAdditionalFileFix(string language, DiagnosticAnalyzer analyzerOpt, CodeFixProvider codeFixProvider, string source, | ||
|
@@ -97,6 +101,31 @@ private void VerifyFix( | |
bool allowNewCompilerDiagnostics, | ||
bool onlyFixFirstFixableDiagnostic, | ||
TestValidationMode validationMode) | ||
{ | ||
// Verify code fix. | ||
VerifyFixOrFixAllCore(document, analyzerOpt, codeFixProvider, newSource, newSourceFileName, additionalFiles, | ||
codeFixIndex, allowNewCompilerDiagnostics, validationMode, testFixAll: false, executeSingleFixOrFixAllPass: onlyFixFirstFixableDiagnostic); | ||
|
||
// Verify FixAll. | ||
if (!onlyFixFirstFixableDiagnostic && codeFixProvider.GetFixAllProvider() != null) | ||
{ | ||
VerifyFixOrFixAllCore(document, analyzerOpt, codeFixProvider, newSource, newSourceFileName, additionalFiles, | ||
codeFixIndex, allowNewCompilerDiagnostics, validationMode, testFixAll: true, executeSingleFixOrFixAllPass: false); | ||
} | ||
} | ||
|
||
private void VerifyFixOrFixAllCore( | ||
Document document, | ||
DiagnosticAnalyzer analyzerOpt, | ||
CodeFixProvider codeFixProvider, | ||
string newSource, | ||
string newSourceFileName, | ||
IEnumerable<TestAdditionalDocument> additionalFiles, | ||
int? codeFixIndex, | ||
bool allowNewCompilerDiagnostics, | ||
TestValidationMode validationMode, | ||
bool testFixAll, | ||
bool executeSingleFixOrFixAllPass) | ||
{ | ||
var fixableDiagnosticIds = codeFixProvider.FixableDiagnosticIds.ToSet(); | ||
Func<IEnumerable<Diagnostic>, ImmutableArray<Diagnostic>> getFixableDiagnostics = diags => | ||
|
@@ -110,8 +139,9 @@ private void VerifyFix( | |
while (diagnosticIndexToFix < fixableDiagnostics.Length) | ||
{ | ||
var actions = new List<CodeAction>(); | ||
Diagnostic triggerDiagnostic = fixableDiagnostics[diagnosticIndexToFix]; | ||
|
||
var context = new CodeFixContext(document, fixableDiagnostics[diagnosticIndexToFix], (a, d) => actions.Add(a), CancellationToken.None); | ||
var context = new CodeFixContext(document, triggerDiagnostic, (a, d) => actions.Add(a), CancellationToken.None); | ||
codeFixProvider.RegisterCodeFixesAsync(context).Wait(); | ||
if (!actions.Any()) | ||
{ | ||
|
@@ -128,10 +158,22 @@ private void VerifyFix( | |
throw new Exception($"Unable to invoke code fix at index '{codeFixIndex.Value}', only '{actions.Count}' code fixes were registered."); | ||
} | ||
|
||
document = document.Apply(actions.ElementAt(codeFixIndex.Value)); | ||
var codeAction = actions.ElementAt(codeFixIndex.Value); | ||
|
||
if (!testFixAll) | ||
{ | ||
document = document.Apply(codeAction); | ||
} | ||
else | ||
{ | ||
string diagnosticIdToFix = triggerDiagnostic.Id; | ||
var diagnosticsToFix = fixableDiagnostics.Where(d => d.Id == diagnosticIdToFix); | ||
document = ApplyFixAll(document, codeAction, codeFixProvider, codeFixProvider.GetFixAllProvider(), diagnosticIdToFix, diagnosticsToFix); | ||
} | ||
|
||
additionalFiles = document.Project.AdditionalDocuments.Select(a => new TestAdditionalDocument(a)); | ||
|
||
if (onlyFixFirstFixableDiagnostic) | ||
if (executeSingleFixOrFixAllPass) | ||
{ | ||
break; | ||
} | ||
|
@@ -167,51 +209,26 @@ private void VerifyFix( | |
Assert.Equal(newSource, actualText.ToString()); | ||
} | ||
|
||
private void VerifyFixAll( | ||
private Document ApplyFixAll( | ||
Document document, | ||
DiagnosticAnalyzer analyzerOpt, | ||
CodeAction codeAction, | ||
CodeFixProvider codeFixProvider, | ||
string newSource, | ||
string newSourceFileName, | ||
IEnumerable<TestAdditionalDocument> additionalFiles, | ||
bool allowNewCompilerDiagnostics, | ||
TestValidationMode validationMode) | ||
FixAllProvider fixAllProvider, | ||
string diagnosticIdToFix, | ||
IEnumerable<Diagnostic> fixableDiagnostics) | ||
{ | ||
var fixableDiagnosticIds = codeFixProvider.FixableDiagnosticIds.ToSet(); | ||
Func<IEnumerable<Diagnostic>, ImmutableArray<Diagnostic>> getFixableDiagnostics = diags => | ||
diags.Where(d => fixableDiagnosticIds.Contains(d.Id)).ToImmutableArrayOrEmpty(); | ||
|
||
var analyzerDiagnostics = GetSortedDiagnostics(analyzerOpt, new[] { document }, additionalFiles: additionalFiles, validationMode: validationMode); | ||
var compilerDiagnostics = document.GetSemanticModelAsync().Result.GetDiagnostics(); | ||
var fixableDiagnostics = getFixableDiagnostics(analyzerDiagnostics.Concat(compilerDiagnostics)); | ||
|
||
var fixAllProvider = codeFixProvider.GetFixAllProvider(); | ||
var diagnosticProvider = new FixAllDiagnosticProvider(analyzerOpt, additionalFiles, validationMode, getFixableDiagnostics); | ||
var fixAllContext = new FixAllContext(document, codeFixProvider, FixAllScope.Document, string.Empty, fixableDiagnostics.Select(d => d.Id), diagnosticProvider, CancellationToken.None); | ||
var codeAction = fixAllProvider.GetFixAsync(fixAllContext).Result; | ||
document = document.Apply(codeAction); | ||
additionalFiles = document.Project.AdditionalDocuments.Select(a => new TestAdditionalDocument(a)); | ||
|
||
additionalFiles = document.Project.AdditionalDocuments.Select(a => new TestAdditionalDocument(a)); | ||
|
||
analyzerDiagnostics = GetSortedDiagnostics(analyzerOpt, new[] { document }, additionalFiles: additionalFiles, validationMode: validationMode); | ||
|
||
var updatedCompilerDiagnostics = document.GetSemanticModelAsync().Result.GetDiagnostics(); | ||
var newCompilerDiagnostics = GetNewDiagnostics(compilerDiagnostics, updatedCompilerDiagnostics); | ||
if (!allowNewCompilerDiagnostics && newCompilerDiagnostics.Any()) | ||
var diagnosticProvider = new FixAllDiagnosticProvider(fixableDiagnostics); | ||
IEnumerable<string> fixableDiagnosticIds = new string[] { diagnosticIdToFix }; | ||
var fixAllContext = new FixAllContext(document, codeFixProvider, FixAllScope.Document, codeAction.EquivalenceKey, fixableDiagnosticIds, diagnosticProvider, CancellationToken.None); | ||
var fixTask = fixAllProvider.GetFixAsync(fixAllContext); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how this can return null? Async method returning null is a bug? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I added that while debugging the nullref due to dotnet/roslyn#23492. Let me revert. |
||
if (fixTask == null) | ||
{ | ||
// Format and get the compiler diagnostics again so that the locations make sense in the output | ||
document = document.WithSyntaxRoot(Formatter.Format(document.GetSyntaxRootAsync().Result, Formatter.Annotation, document.Project.Solution.Workspace)); | ||
newCompilerDiagnostics = GetNewDiagnostics(compilerDiagnostics, document.GetSemanticModelAsync().Result.GetDiagnostics()); | ||
|
||
Assert.True(false, | ||
string.Format("Fix introduced new compiler diagnostics:\r\n{0}\r\n\r\nNew document:\r\n{1}\r\n", | ||
newCompilerDiagnostics.Select(d => d.ToString()).Join("\r\n"), | ||
document.GetSyntaxRootAsync().Result.ToFullString())); | ||
return document; | ||
} | ||
|
||
var actualText = GetActualTextForNewDocument(document, newSourceFileName); | ||
Assert.Equal(newSource, actualText.ToString()); | ||
fixTask.Wait(); | ||
var fixAllCodeAction = fixTask.Result; | ||
return fixAllCodeAction != null ? document.Apply(fixAllCodeAction) : document; | ||
} | ||
|
||
private sealed class DiagnosticComparer : IEqualityComparer<Diagnostic> | ||
|
@@ -240,37 +257,21 @@ public int GetHashCode(Diagnostic obj) | |
|
||
private class FixAllDiagnosticProvider : FixAllContext.DiagnosticProvider | ||
{ | ||
private DiagnosticAnalyzer _analyzerOpt; | ||
private IEnumerable<TestAdditionalDocument> _additionalFiles; | ||
private TestValidationMode _testValidationMode; | ||
private Func<IEnumerable<Diagnostic>, ImmutableArray<Diagnostic>> _getFixableDiagnostics; | ||
|
||
public FixAllDiagnosticProvider( | ||
DiagnosticAnalyzer analyzerOpt, | ||
IEnumerable<TestAdditionalDocument> additionalFiles, | ||
TestValidationMode testValidationMode, | ||
Func<IEnumerable<Diagnostic>, ImmutableArray<Diagnostic>> getFixableDiagnostics) | ||
{ | ||
_analyzerOpt = analyzerOpt; | ||
_additionalFiles = additionalFiles; | ||
_testValidationMode = testValidationMode; | ||
_getFixableDiagnostics = getFixableDiagnostics; | ||
} | ||
private IEnumerable<Diagnostic> _fixableDiagnostics; | ||
|
||
public override async Task<IEnumerable<Diagnostic>> GetDocumentDiagnosticsAsync(Document document, CancellationToken cancellationToken) | ||
public FixAllDiagnosticProvider(IEnumerable<Diagnostic> fixableDiagnostics) | ||
{ | ||
var analyzerDiagnostics = GetSortedDiagnostics(_analyzerOpt, new[] { document }, additionalFiles: _additionalFiles, validationMode: _testValidationMode); | ||
var semanticModel = await document.GetSemanticModelAsync().ConfigureAwait(false); | ||
var compilerDiagnostics = semanticModel.GetDiagnostics(); | ||
var fixableDiagnostics = _getFixableDiagnostics(analyzerDiagnostics.Concat(compilerDiagnostics)); | ||
return fixableDiagnostics; | ||
_fixableDiagnostics = fixableDiagnostics; | ||
} | ||
|
||
public override Task<IEnumerable<Diagnostic>> GetDocumentDiagnosticsAsync(Document document, CancellationToken cancellationToken) | ||
=> Task.FromResult(_fixableDiagnostics); | ||
|
||
public override Task<IEnumerable<Diagnostic>> GetAllDiagnosticsAsync(Project project, CancellationToken cancellationToken) | ||
=> throw new NotImplementedException(); | ||
=> Task.FromResult(_fixableDiagnostics); | ||
|
||
public override Task<IEnumerable<Diagnostic>> GetProjectDiagnosticsAsync(Project project, CancellationToken cancellationToken) | ||
=> throw new NotImplementedException(); | ||
=> Task.FromResult(_fixableDiagnostics); | ||
} | ||
|
||
private static SourceText GetActualTextForNewDocument(Document documentInNewWorkspace, string newSourceFileName) | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Location of the call means that we will make it within the while loop. And can repeat it again and again.
If we have an error within a fix all algorithm for a given analyzer that fixes, for example, just a single error not all; this fixer would pass all tests because it allows fixing one by one.
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.
Thanks, as we discussed, I will modify this to do a single iteration. For tests which are relying on iterative fix/diagnostic/fix output, they can be handled specially (call separate API for verification)
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.
Fix All does not always fix all in a single iteration. For StyleCop Analyzers we fail the test if it fails to fix everything in one iteration, but have a parameter which allows you to change the behavior. Most of the work to implement this was here:
DotNetAnalyzers/StyleCopAnalyzers#1989
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.
Thanks Sam. After discussing with Ivan, we do something similar for the default case. We only run one FixAll iteration and fail it doesn't fix everything. Tests which fail this way can turn off default FixAll validation with an optional flag, and have a separate VerifyFixAll invocation with appropriate fixedSource.