Fixing 'else' keyword completion by Neme12 · Pull Request #25703 · dotnet/roslyn

Neme12 · 2018-03-25T02:08:54Z

I tried rewriting the tests using theory at the end to make them less overwhelming and significantly reduce the repetition, but I'm not sure if it's the right thing to do considering that this sort of approach isn't used in any other tests.
@CyrusNajmabadi I think I remember you saying that tests should contain the literal code so that it's easy to understand each one separately and we should generally avoid using any sort of helpers. Do you think what I did here is acceptable?

CyrusNajmabadi · 2018-03-25T18:52:15Z

src/EditorFeatures/CSharpTest2/Recommendations/ElseKeywordRecommenderTests.cs

-    Console.WriteLine();
-}
+$@"if (true)
+    {statement}


i personally dislike this. I cannot see what's happening. My tests are now non-local.

The only sort of 'theory' approach i've ever been ok with is targetted, simple application of theories, where the theory is on hte test method itself. if i have to look up more than 2-3 lines, or i have to go look at another class/method etc. then i've completely lost the plot on what the test is testing. Previously i could see this exactly. Now i can't. this is a net negative for me here.

ok I can revert it back... it just seemed to me like this way it's much easier to read, review and spot bugs in the tests, whereas with a lot of duplication it's really easy to miss something

and also more importantly, when writing more tests people will usually avoid writing all the cases and just do some of them (or more likely just one) to not bother with writing so much duplication, which can sometimes miss bugs due to missing test cases

IMO, tests for this sort of thing should be written based on the expected interesting cases based on the code change. So, your change adds an arbitrary walk upwards looking for if-statements. And it also checks that if-statement for the last token of the true-statement within it.

It seems like we could test this out with some very judicious tests that would demonstrate that both of those pieces of logic are working properly. For example, we sholud not see 'else' here:

if (true) Console.WriteLine() $$

Because the last token of hte true-statement would be the missing-token ; and htat's not what we're on. Similarly, we should see if after Console.WriteLine();. And we should see if htis if it deeply nested. Testing one level deep, two levels deep, and something larger should be sufficient.

You also removed some existing branches in the original code. We shoudl ensure appropriate behavior remains for those.

This does not seem like a lot of tests needed. It tests positive and negative cases. And feels appropriate given the code change.

Thanks, I'll add tests for that too.

it just seemed to me like this way it's much easier to read, review and spot bugs in the tests

It's harder for me, because i ca'nt just look at a test and know what it means. I have to go analyze other parts of the test file to know. We try to avoid that. I've accepted some Theories in hte past where the theory travels with the test method. and the theory itself is very tiny. i.e. i've seen theories that try to do major replacements in the code, and def make it less clear.

I'm on the fence here about this one. But, give my previous post, i don't think it adds substantial value given just writing the 6ish test cases manually.

Yes. this form seems ok to me. i can live with inline data at individual test sites.

CyrusNajmabadi

I like the code change. i don't like the test change. that said, i'm sympathetic about the amount of duplication you feel you're incur here.

CyrusNajmabadi · 2018-03-25T18:54:48Z

src/EditorFeatures/CSharpTest2/Recommendations/ElseKeywordRecommenderTests.cs


-        [Fact, Trait(Traits.Feature, Traits.Features.KeywordRecommending)]
-        public async Task TestAfterIfBlock()
+        class Statements : DataAttribute


CyrusNajmabadi · 2018-03-25T18:56:42Z

I tried rewriting the tests using theory at the end to make them less overwhelming

To me this i a non-problem. I don't think of test quantity as being "overwhelming". It's just a lot of tests. There are still the same number of tests when you use theories, you just have used a generation technique to produce the tests, which i think decreases clarity.

At the end of hte day you generally have 100% test coverage until you break something and either a single test breaks, or a set of tests break. You go and look at those tests, figure it out and rinse/repeat. The number of total tests isn't really relevant here.

Neme12 · 2018-03-25T19:04:09Z

@CyrusNajmabadi What if I put InlineData on each test instead but still kept it as a theory? How would you feel about that?

Neme12 · 2018-03-25T19:07:21Z

src/Features/CSharp/Portable/Completion/KeywordRecommenders/ElseKeywordRecommender.cs

-            if (token.IsKind(SyntaxKind.CloseBraceToken) && ifStatement.Statement is BlockSyntax && token == ((BlockSyntax)ifStatement.Statement).CloseBraceToken)
-            {
-                return true;
+                if (ifStatement.Statement.GetLastToken(includeSkipped: true, includeZeroWidth: true) == token)


I actually have no idea why includeSkipped: true is needed here. There's no test that covers this, everything passes either way.

IncludeSkipped is likely appropriate, though harder to test for. It basically means, "if the parser ran into stuff it totally didn't understand, but it attached it to that staement, then treat hte last token of that skipped run of tokens as the last token of the statement.

It seems really unlikely that there would be a skipped semicolon at the end. That usually parsers as a separate empty statement.

In the original implementation, else is not offered in code like this (because the last token including skipped is not a semicolon or a close brace):

if (true) Console.WriteLine();,

But with this PR, it is offered. Which is the expected behavior here? To offer or not offer? I will add a test either way. If it's to not offer, includeSkipped should actually be removed.

Which is the expected behavior here?

The expected behavior is whatever is easiest to provide :)

You could literally have this work, and that would be ok. Or it could not work, and that would be ok.

whatever is easiest to provide

The difference is just in setting includeSkipped to either true or false. Then I guess easiest to provide would be to not specify the argument?

Given that there is no completion for anything after such skipped tokens and the fact that if you type else at that point it doesn't actually end up being an else clause (it becomes a skipped token too), I'm removing this.

CyrusNajmabadi · 2018-03-25T19:08:57Z

@CyrusNajmabadi What if I put InlineData on each test instead but still kept it as a theory? How would you feel about that?

I would probably be ok with that. though i like the InlineData to be small to be clear as to what's going on. In your case, it's likely ok. But it's not a blanket rule that that's ok for all tests.

CyrusNajmabadi · 2018-03-27T18:39:28Z

src/Features/CSharp/Portable/Completion/KeywordRecommenders/ElseKeywordRecommender.cs

-            if (token.IsKind(SyntaxKind.CloseBraceToken) && ifStatement.Statement is BlockSyntax && token == ((BlockSyntax)ifStatement.Statement).CloseBraceToken)
-            {
-                return true;
+                if (ifStatement.Statement.GetLastToken(includeZeroWidth: true) == token)


warrants a comment. but not a big deal.

fixed in 0c495ca

CyrusNajmabadi

Thanks!

Neme12 · 2018-03-28T16:20:33Z

retest windows_debug_vs-integration_prtest please

Neme12 · 2018-03-30T16:34:25Z

retest windows_debug_vs-integration_prtest please

Neme12 · 2018-04-06T17:28:58Z

retest windows_debug_spanish_unit32_prtest please

jcouv

LGTM Thanks

jcouv · 2018-04-17T17:08:33Z

@dotnet/roslyn-ide Can this community PR get a final review?

dpoeschl

Nice! Thanks!

Neme12 added 7 commits March 25, 2018 00:23

Removing the token kind check

8e8519e

FIxing nested if statements

d5b0cc8

Adding more tests

20749e9

Even more tests

8cd400b

Last set of tests

665604c

Rewriting tests using theory with inline data

0ae2763

Adding issue links

265fc4d

Neme12 requested a review from a team as a code owner March 25, 2018 02:08

jcouv added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Mar 25, 2018

CyrusNajmabadi reviewed Mar 25, 2018

View reviewed changes

CyrusNajmabadi approved these changes Mar 25, 2018

View reviewed changes

CyrusNajmabadi reviewed Mar 25, 2018

View reviewed changes

Neme12 commented Mar 25, 2018

View reviewed changes

Neme12 added 3 commits March 27, 2018 15:36

Using InlineData

cc54f16

Adding more tests with incomplete statements

e979989

Removing includeSkipped

5d842cd

Neme12 force-pushed the elseRecommender branch from c5c0393 to 5d842cd Compare March 27, 2018 14:33

CyrusNajmabadi reviewed Mar 27, 2018

View reviewed changes

CyrusNajmabadi approved these changes Mar 27, 2018

View reviewed changes

jcouv added this to the 15.8 milestone Mar 27, 2018

Neme12 mentioned this pull request Mar 27, 2018

Adding missing completion for switch when clause #25133

Merged

Neme12 added 4 commits March 28, 2018 15:16

Another interesting test case

f502a42

Adding a comment

0c495ca

One more test

e8a5e73

Another one

d25ba74

Merge remote-tracking branch 'upstream/master' into elseRecommender

8190202

jcouv approved these changes Apr 8, 2018

View reviewed changes

dpoeschl approved these changes Apr 17, 2018

View reviewed changes

jcouv merged commit 5aedc27 into dotnet:master Apr 17, 2018

Neme12 deleted the elseRecommender branch April 17, 2018 23:11

Conversation

Neme12 commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Mar 25, 2018

Uh oh!

Neme12 commented Mar 25, 2018

Uh oh!

Neme12 Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Mar 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Neme12 commented Mar 28, 2018

Uh oh!

Neme12 commented Mar 30, 2018

Uh oh!

Neme12 commented Apr 6, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Apr 17, 2018

Uh oh!

dpoeschl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Neme12 commented Mar 25, 2018 •

edited

Loading

Neme12 Mar 25, 2018 •

edited

Loading

CyrusNajmabadi Mar 25, 2018 •

edited

Loading

Neme12 Mar 25, 2018 •

edited

Loading

Neme12 Mar 25, 2018 •

edited

Loading

Neme12 Mar 27, 2018 •

edited

Loading