Code coverage removal fix for single line comments #1996

karthikraobr · 2018-10-12T14:29:38Z

This pull request partially fixes #1852.

msftclas · 2018-10-12T14:29:50Z

All CLA requirements met.

ramya-rao-a · 2018-10-15T18:40:46Z

src/goCover.ts

+export function isPartOfComment(e: vscode.TextDocumentChangeEvent): boolean {
+ let result = false
+ e.contentChanges.every(function(change, index){
+ let line = e.document.lineAt(change.range.start)


What about the case when the range spans multiple lines and only the first line was part of a comment? In that case, isPartOfComment would return true instead of false

For block comments of the form /* .... *,. I was thinking of the following solution

Scan backwards from current change until start of the document or until / is found to find /. This would be necessary in cases where the block comment starts on a line after the code and changes are made to the code. In this case we would have to clear the coverage. Example

return 0, err /* one two three. */

Scan forwards until */ is found or end of the document.

Check if change is within the above range.

Seems very inefficient to me. Is there a better way of doing it? Perhaps a vscode API to identify block comments?

Here I wasnt referring to block comments. Let's scope this PR to just line comments for now. Take the below case

// I am a comment var a = 1 var b = 2

Now if I start selecting from the word comment and continue till the next line and delete the selected text, then the line variable in your code will have // I am a and isPartOfComment will return true even though some code was deleted.

I have updated the PR to resolve this issue.

ramya-rao-a · 2018-10-15T18:41:45Z

src/goCover.ts

+ let line = e.document.lineAt(change.range.start)
+ let text = e.document.getText(line.rangeIncludingLineBreak)
+ // Exit the loop as soon as one of the changes is not part of a comment.
+ if (!text.startsWith('//')){


What about a case like the below?

var a = 1 // this is a comment

Here when the comment is edited, isPartOfComment returns false instead of true

Fixed. Will update the PR once I have fixed the block comment issue.

ramya-rao-a · 2018-10-15T18:46:10Z

src/goCover.ts

+ let text = e.document.getText(line.rangeIncludingLineBreak)
+ // Exit the loop as soon as one of the changes is not part of a comment.
+ if (!text.startsWith('//')){
+ result = false


We dont need to use the result variable here. Instead, we can do the below

return e.contentChanges.every(change => { // Just return true or false based on your logic here }

The .every on an array stops looping over the array items the moment one of them returns false in the callback

I guess it would be a good idea not to return as soon as we find that the change is a part of a comment. If another change which is not a part of a comment is found in a later iteration, then we would have to anyway remove the highlighting right?

karthikraobr · 2018-10-15T23:11:47Z

Apologies for the multiple lint commits. Go dev here :) Hence the missing semi-colons.

ramya-rao-a · 2018-10-16T00:00:20Z

src/goCover.ts

+export function isPartOfComment(e: vscode.TextDocumentChangeEvent): boolean {
+ let result = false;
+ e.contentChanges.every(function (change, index) {
+ let line = e.document.lineAt(change.range.start);


return e.contentChanges.every(change => { // Just return true or false based on your logic here }

I guess it would be a good idea not to return as soon as we find that the change is a part of a comment. If another change which is not a part of a comment is found in a later iteration, then we would have to anyway remove the highlighting right?

A return statement in the callback passed to the .every doesnt cause a return from isPartOfComment.
A return true statement in the callback passed to the .every cause the iteration to continue;
A return false statement in the callback passed to the .every cause the iteration to stop;

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/every

Thanks for the detailed explanation.

ramya-rao-a · 2018-10-16T00:04:33Z

src/goCover.ts

+ let text = e.document.getText(line.range);
+ let idx = text.search('//');
+ let changeIdx = change.range.start.character;
+ if (idx === -1 || idx > changeIdx || !change.range.isSingleLine || change.text.includes('\n')) {


Can you add some tests to cover these cases?

Sure! Could you please point me to the tests currently present for unit test coverage? I am assuming that I would have to add some test fixtures here and then perform some assertions.

Could you please give me some pointers on how to write tests. I ran the tests and seems like currently there are no tests for code coverage. I am assuming these would be the steps I would have to follow :

Create a go file with code and comments to cover all scenarios.

Write some tests for the above file and run them. (use testCurrentFile function).

Select some part of the code and delete/add (based on the scenarios in the if condition using applyUsingTextEditorEdit ).

And check if coverage highlights have been removed or retained. (How do I achieve this?)

Yes, there are no tests for code coverage right now :(

For this PR, testing just the isPartOfComment function would be enough. I would suggest to

Update the signature of isPartOfComment so that it takes 2 parameters document and ranges.

Create a go file in the fixtures folder with some comments to cover all scenarios

Use vscode.workspace.openTextDocument and vscode.window.showTextDocument just like the other tests to get the document object.

Then for each case, create the ranges and pass it to isPartOfComment

…fix/coverage

karthikraobr · 2018-10-17T12:01:26Z

In case of a deletion, the changed text is always "". Also, the TextDocument object contains the text after the deletion has been performed. Is there a way to get the deleted text, rather than an empty string? This is necessary in cases where multiple lines are deleted. How would I differentiate between deleting lines 1&2 and deleting lines 2&3.

1.// hello world
2.// hello again
3.func sayHello()

ramya-rao-a · 2018-10-20T21:32:49Z

This will need some more time from my side to look into, I'll get back to this next week.

karthikraobr · 2018-12-03T16:32:21Z

This will need some more time from my side to look into, I'll get back to this next week.

@ramya-rao-a could you please let me know if there has been an update on this?

ramya-rao-a · 2018-12-03T18:13:40Z

@karthikraobr Thanks for your patience, I'll get right back to this as soon as I release the current update which should be sometime this week.

karthikraobr · 2018-12-20T16:22:25Z

hi @ramya-rao-a , any update on this? Thanks and sorry for the repeated reminders :)

karthikraobr · 2019-01-11T20:46:46Z

@ramya-rao-a any update on this and I apologise in advance :)

ramya-rao-a · 2019-01-15T03:38:08Z

@karthikraobr No apologies needed :) You have every right to give the reminders.

I have been a little busy at work from my end, and so couldnt get back sooner. I'll definitely take a look soon

ramya-rao-a · 2019-01-15T04:00:11Z

I have refactored the code a bit to avoid checking for comments when there is no coverage applied in the first place and also the code for isPartOfComment as well

Fix for single line comments

25a311e

karthikraobr changed the title ~~Fix for single line comments~~ Code coverage removal fix for single line comments Oct 12, 2018

ramya-rao-a suggested changes Oct 15, 2018

View reviewed changes

karthikraobr added 4 commits October 15, 2018 22:13

Fix for all scenarios in single line comment

ead2ec5

Fixed an issue when the change was on multiple lines

7898714

Lint

11a49c5

Fixed other linting issue

5283575

ramya-rao-a reviewed Oct 16, 2018

View reviewed changes

karthikraobr added 4 commits October 16, 2018 02:20

Removed unnecessary variables

19e22b3

Cleanup#

1381a4b

Master merged

5543a04

Merge branch 'master' of https://github.com/Microsoft/vscode-go into …

a21aa23

…fix/coverage

ramya-rao-a added 2 commits January 14, 2019 19:58

Refactor and avoid unnecessary comment checks

ffc5f94

linting error

5d7b25f

ramya-rao-a approved these changes Jan 15, 2019

View reviewed changes

ramya-rao-a merged commit a7b0fb7 into microsoft:master Jan 15, 2019

ramya-rao-a mentioned this pull request Jan 29, 2019

Test Plan for early Feb (2019) release #2288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code coverage removal fix for single line comments #1996

Code coverage removal fix for single line comments #1996

karthikraobr commented Oct 12, 2018

msftclas commented Oct 12, 2018 •

edited

Loading

ramya-rao-a Oct 15, 2018

karthikraobr Oct 15, 2018 •

edited

Loading

ramya-rao-a Oct 15, 2018 •

edited

Loading

karthikraobr Oct 15, 2018

ramya-rao-a Oct 15, 2018

karthikraobr Oct 15, 2018

ramya-rao-a Oct 15, 2018

karthikraobr Oct 15, 2018 •

edited

Loading

karthikraobr commented Oct 15, 2018

ramya-rao-a Oct 16, 2018

ramya-rao-a Oct 16, 2018

karthikraobr Oct 16, 2018

ramya-rao-a Oct 16, 2018

karthikraobr Oct 16, 2018 •

edited

Loading

karthikraobr Oct 16, 2018

ramya-rao-a Oct 17, 2018

karthikraobr commented Oct 17, 2018 •

edited

Loading

ramya-rao-a commented Oct 20, 2018

karthikraobr commented Dec 3, 2018

ramya-rao-a commented Dec 3, 2018

karthikraobr commented Dec 20, 2018

karthikraobr commented Jan 11, 2019

ramya-rao-a commented Jan 15, 2019

ramya-rao-a commented Jan 15, 2019

Code coverage removal fix for single line comments #1996

Code coverage removal fix for single line comments #1996

Conversation

karthikraobr commented Oct 12, 2018

msftclas commented Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

karthikraobr Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

ramya-rao-a Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikraobr Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

karthikraobr commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikraobr Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikraobr commented Oct 17, 2018 • edited Loading

ramya-rao-a commented Oct 20, 2018

karthikraobr commented Dec 3, 2018

ramya-rao-a commented Dec 3, 2018

karthikraobr commented Dec 20, 2018

karthikraobr commented Jan 11, 2019

ramya-rao-a commented Jan 15, 2019

ramya-rao-a commented Jan 15, 2019

msftclas commented Oct 12, 2018 •

edited

Loading

karthikraobr Oct 15, 2018 •

edited

Loading

ramya-rao-a Oct 15, 2018 •

edited

Loading

karthikraobr Oct 15, 2018 •

edited

Loading

karthikraobr Oct 16, 2018 •

edited

Loading

karthikraobr commented Oct 17, 2018 •

edited

Loading