Consider trivia when testing code fixes and refactorings#21439
Consider trivia when testing code fixes and refactorings#21439CyrusNajmabadi merged 4 commits intodotnet:masterfrom
Conversation
cc8143b to
f2b77eb
Compare
There was a problem hiding this comment.
💭 It seems like the previous version was correct here, but I'm not certain enough to call it a bug.
There was a problem hiding this comment.
🐛 ignoreTrivia is totally ignored here.
There was a problem hiding this comment.
💭 This formatting seems sub-optimal.
There was a problem hiding this comment.
🐛 This directive disappeared.
There was a problem hiding this comment.
🐛 This directive disappeared.
There was a problem hiding this comment.
| Dim c = TryCast(obj, c1(Of V, U)) | ||
| Return c IsNot Nothing AndAlso EqualityComparer(Of V).Default.Equals(x, c.x) | ||
| Return c IsNot Nothing AndAlso | ||
| EqualityComparer(Of V).Default.Equals(x, c.x) |
There was a problem hiding this comment.
🐛 This formatting is not consistent with the smart indent implementation. The second line should be indented 4 spaces, not 7.
| sub M() | ||
| for each (v in x) | ||
| {|Warning:dim i = CInt(0)|} | ||
| {|Warning:dim i = CInt(0)|} |
There was a problem hiding this comment.
🐛 Incorrect indentation
| if true | ||
| dim x as integer = 5 | ||
| dim x as integer | ||
| = 5 |
There was a problem hiding this comment.
🐛 Should not have split the line.
| End Namespace|]", | ||
| "Imports System, System.Collections.Generic | ||
| Module Program | ||
| "Imports System, System.Collections.Generic Module Program |
There was a problem hiding this comment.
🐛 Module should still appear on the next line.
| End Class | ||
| End Namespace|]", | ||
| "Imports A | ||
| "Imports A _ |
There was a problem hiding this comment.
Curious, do you use some tool to update assertions based on the actual output?
There was a problem hiding this comment.
No, I manually reviewed each case. 😦
There was a problem hiding this comment.
Currently my bottleneck is the test-runner - tried a couple of other tools. It's really inconvenient to use command-line or wpf xunit runner. I'm wondering if either of those are what you're using for local testing.
There was a problem hiding this comment.
I used the WPF runner. I made a note from this work regarding some additional potential areas for improvement.
There was a problem hiding this comment.
That's great! I'm looking forward to it. Thanks.
|
|
||
| Case "File" | ||
| Return String.Join(vbCrLf, lines) | ||
| Return testSource.NormalizedValue |
There was a problem hiding this comment.
Any need to have the option at all now? I think it ahs served its purpose. It was originally used because of too much churn in the formatter, but now that that has stabilized, i would be fine with it going away. |
@CyrusNajmabadi The PR is unmanageable with that change merged with this one. The next step is removing over 1000 places where it's explicitly specified to be |
It is very useful when you're just verifying the behavior without worrying about trivia. After that it should be required to be false (or absent with default=false) before merge. |
Seems reasonable to me. |
ignoreTriviato default to falseReviewers: The main item to verify is that this pull request only changes expected test outputs. Test inputs should remain the same. One (seemingly unavoidable) exception is indicated with a⚠️ comment.
📝 The changes to expected test results reveal a large number of bugs. These will be identified in comments in this PR, and bugs filed so they can be fixed later.