Undeclared compilation errors should fail analyzer/fixer tests

[sharwell]

Currently tests silently pass if they contain a compilation error, which can lead to false negatives. For example, if a test verifying that a diagnostic is not reported for some syntax scenario contains a syntax error, it's possible that the diagnostic is only omitted because of the syntax error.

Similarly, for code fixes, it's possible for a fix to introduce a compilation error without alerting the developer of the code fix to the fact that the fix is behaving incorrectly. All tests against source code should require the compilation errors present in the source be declared, with those errors validated as a core part of the test.

📝 This recently affected #21528.
🔗 StyleCop Analyzers adopted a similar approach in DotNetAnalyzers/StyleCopAnalyzers@746e551, and improved the usability in DotNetAnalyzers/StyleCopAnalyzers#743 and DotNetAnalyzers/StyleCopAnalyzers#2136.

[CyrusNajmabadi]

All tests against source code should require the compilation errors present in the source be declared,

I disagree. A core point of the refactoring/fix tests was that they are describing the behavior the user should experience, without being dependent on what underlying mechanism leads to producing them. For example, GenerateMethod moved from being an entirely different system to one based on analyzers. The tests migrated entirely because they only define the experience a user has (i.e. i move to this location, i invoke feature, i get this final result), and not anything about how the feature is implemented (i.e. it is dependent on a particular diagnostic).

[sharwell]

A core point of the refactoring/fix tests was that they are describing the behavior the user should experience, without being dependent on what underlying mechanism leads to producing them.

In theory this is fine. In practice it causes silent behavior changes in tests which can (and have) led to false positives (test passes even though the feature under test is broken). When StyleCop Analyzers adopted this approach, I believe there were more than 100 unintended errors found in inputs.

[bkoelman]

Yes I agree. I did something similar for the roslyn-analyzers repo in the past, which revealed numerous unintended compile errors in tests. A few of them were so broken that I couldn't even figure out why they were created.

[CyrusNajmabadi]

I would like to see actual data on the counts here.

[CyrusNajmabadi]

Note: i think it would be fine if a test had to declare any errors introduced by the fix. That's part of its behavior after all.

[bkoelman]

For counts, see dotnet/roslyn-analyzers#1040.

[CyrusNajmabadi]

That seems pretty low.

And, as i mentioned in my original statement. The errors present before the fix is run aren't part of the design of these features. It's testing an unrelated part of the system.

Now, I am ok with collecting hte errors prior to the fix being run to determine if any errors were added/changed. That user facing behavior is caused by the feature, and should be captured (in as far as to document it, or to make sure it doesn't change unintentionally).