Improve testing of code fixes, and improve diagnostic messages#18742
Conversation
aozgaa
left a comment
There was a problem hiding this comment.
Looks good, see comments.
| this.applyEdits(change.fileName, change.textChanges, /*isFormattingEdit*/ false); | ||
| } | ||
|
|
||
| if (this.getRanges().length === 0) { |
There was a problem hiding this comment.
It might be easier to understand the tests if we are uniform in requiring a range to be specified, rather than switching the behavior in a way that forces the user to navigate back to this line of the source.
We could alternately come up with a different name for newContent that does capture this switching behavior.
I personally favor the first option, but I'm open to alternatives.
| } | ||
| } | ||
|
|
||
| public verifyCodeFix(options: FourSlashInterface.VerifyCodeFixOptions) { |
There was a problem hiding this comment.
Consider merging this into rangeAfterCodeFix (possibly in a separate PR) to keep the 4/ API simple.
There was a problem hiding this comment.
I'm planning to remove all rangeAfterCodeFix in another PR.
| } | ||
|
|
||
| export interface VerifyCodeFixOptions { | ||
| description: string; |
There was a problem hiding this comment.
Should description and newContent be optional as well?
There was a problem hiding this comment.
Not if we want to enforce that these are tested.
We were reporting fixes to add instance methods as static, and vice versa.
Also added a diagnostic message for adding static properties.
Since this resulted in no test changes, I added functionality to assert the description of code fixes; haven't applied it to every test yet but I think that would be a good idea since we've caught some bugs already.
The new tester no longer ignores whitespace, which uncovered #18741 and #18743 (and also more #18445 errors).