Add diagnostic verification extensibility. #584
Conversation
|
|
|
Tagging @sharwell as he has some context for the change. |
| /// The action compares actual <see cref="Diagnostic"/> and the expected | ||
| /// <see cref="DiagnosticResult"/> based on custom test requirements not yet supported by the test framework. | ||
| /// </summary> | ||
| public Action<ImmutableArray<DiagnosticAnalyzer>, Diagnostic, DiagnosticResult, IVerifier>? DiagnosticVerifier { get; set; } |
There was a problem hiding this comment.
Can we remove the analyzer portion?
| public Action<ImmutableArray<DiagnosticAnalyzer>, Diagnostic, DiagnosticResult, IVerifier>? DiagnosticVerifier { get; set; } | |
| public Action<Diagnostic, DiagnosticResult, IVerifier>? DiagnosticVerifier { get; set; } |
💭 I'm tempted to go as far as removing the DiagnosticResult portion as well.
There was a problem hiding this comment.
That means implementors can't use the following method for consistency:
protected string FormatVerifierMessage(ImmutableArray<DiagnosticAnalyzer> analyzers, Diagnostic actual, DiagnosticResult expected, string message)
Is that ok?
There was a problem hiding this comment.
re: DiagnosticResult for expected. I'd like to keep it so that it can be a key to a dictionary containing expected data.
I.e. I would use DiagnosticResult.Location.Span as the key and Dictionary<string, string> for expected properties I'm validating in the Test class.
There was a problem hiding this comment.
That means implementors can't use the following method for consistency
The helper wouldn't produce an actionable error message for this case, so I'm not sure how much it helps.
I'd like to keep it so that it can be a key to a dictionary containing expected data.
📝 This is already not supported. DiagnosticResult is a value type which is not equatable.
Let me think about this a bit. If I can't find an "obviously better" option then the one here seems reasonable.
There was a problem hiding this comment.
I do use TextSpan as keys / comparisons in some of my analyzers to determine if I'm matching up the same piece of code.
There was a problem hiding this comment.
@sharwell are we okay to go ahead with Niko's contribution?
A small extensibility point to allows verification of generated diagnostics for things not yet supported natively by the test library.
In my case, this is the
Propertiesdictionary of the diagnostic.This small change would allow me to remove custom testing frameworks from my projects.