Enable FixAll testing by default for all code fix unit tests #1422
Conversation
By default, VerifyFix method now tests both iterative code fix application and iterative FixAll application. We still have the VerifyFixAll helper method for tests that want to verify single FixAll application. Fixes dotnet#1420
| { | ||
| string diagnosticIdToFix = triggerDiagnostic.Id; | ||
| var diagnosticsToFix = fixableDiagnostics.Where(d => d.Id == diagnosticIdToFix); | ||
| document = ApplyFixAll(document, codeAction, codeFixProvider, codeFixProvider.GetFixAllProvider(), diagnosticIdToFix, diagnosticsToFix); |
There was a problem hiding this comment.
document = ApplyFixAll(document, codeAction, codeFixProvider, codeFixProvider.GetFixAllProvider(), diagnosticIdToFix, diagnosticsToFix); [](start = 20, length = 136)
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.
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.
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:
There was a problem hiding this comment.
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.
| 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.
how this can return null? Async method returning null is a bug?
There was a problem hiding this comment.
Ah, I added that while debugging the nullref due to dotnet/roslyn#23492. Let me revert.
… for the default case. For tests where new diagnostics produced from fixes need to be iteratively fixed, they can turn off default FixAll verification and invoke VerifyFixAll separately
|
This is an old PR, which I am closing in favor of #1773 |
By default, VerifyFix method now tests both iterative code fix application and iterative FixAll application.
We still have the VerifyFixAll helper method for tests that want to verify single FixAll application.
Fixes #1420
This PR is currently expected to fail until dotnet/roslyn#23492 is merged and we move to newer CodeAnalysis package.