Simplify how we do fix all tests.#26826
Conversation
dc1e569 to
40b3640
Compare
|
Tagging @dotnet/roslyn-ide @heejaechang @mavasani . When the test harness was updated to support fix-all, it was done in a somewhat unclean fashion. Specifically, tests had to have carnal knowledge of how the code action generated its internal state, and it they had to respecify that information so the harness could properly fix all. This violated the intent of the code-action tests. The purpose of the tests is to simulate the experience of a user interacting with a lightbulb. In that regard all we want the test to specify is:
Given that, we then should be able to run all the code as necessary and check the resultant code against hte baseline. Importantly, if the user picks something like 'fix all in document', we now properly compute that by figuring out what action they wanted, then actually going the 'fix all' route with that action (and it's equivalence key) instead of hte test having to provide any low level details about this. |
|
@sharwell as well. This is a test only change that helps simplify tests and cleans upa bunch of stuff. |
There was a problem hiding this comment.
@mavasani @sharwell can either of you tell me what the original bug actually was? I want to ensure we have a proper test for it. Importantly, a test should not have any clue about thigns like 'equivalence keys'. It's a low level internal implementation detail, and validating what that value it tells you nothing about if the fix is actually doing the right thing.
There was a problem hiding this comment.
This tests seems to have been added as part of feature work, rather than as a regression test (Bug 774321: CodeCoverage: (potential test hole) class 'ConversionGenerator' has zero coverage fixed with changeset 1331072: We now offer to generate conversion methods in VB and C#. I think deleting this test is the right thing to do.
There was a problem hiding this comment.
Ah, thanks much! Definitely a bit of a wacky test.
5d442d0 to
f2bca24
Compare
There was a problem hiding this comment.
note: i'm not a huge fan of this pattern i'm using here. Effectively, the test says "the user wants to invoke the action at index 'N'". We then go and pass that along to GetCodeActions which will find the action at that index. However, if fix-all is requested, a new code action for the fix-all is synthesized and it is that action that is returned. Since htat is the only action returned, we clearly cannot index into the returned list with the original "index", so the callee returns the index that should be used.
A much cleaner way to do this would be to just return the action to invoke. However, we have a bunch of code that examines all the actions (for thigns like checking spans/titles/etc.).
However, i think i can clean this up by returning the list of actions and the specific action to invoke. i will try to make that change next.
f67c66e to
0526a31
Compare
b83a3a1 to
99d5fde
Compare
| public async Task Tuple_IntroduceLocalForAllOccurrences() | ||
| { | ||
| // Cannot refactor tuple as local constant | ||
| await Assert.ThrowsAsync<Xunit.Sdk.InRangeException>(() => |
There was a problem hiding this comment.
these tests were just innapropriate. They were depending on test code actually throwing specific errors. I've replaced then with the actual descriptive methods they should be calling.
|
Tagging @dotnet/roslyn-ide @heejaechang @mavasani @sharwell Can you take a look at this test-only cleanup. Thanks! |
mavasani
left a comment
There was a problem hiding this comment.
Thanks for the cleanup!
|
@jinujoseph Can this test-only change go in? |
|
Approved to merge for 15.8.Preview3 |
|
Thanks! |
No description provided.