Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Code coverage removal fix for single line comments #1996

Merged
merged 11 commits into from
Jan 15, 2019
Merged

Code coverage removal fix for single line comments #1996

Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
25a311e
Fix for single line comments
karthikraobr Oct 12, 2018
ead2ec5
Fix for all scenarios in single line comment
karthikraobr Oct 15, 2018
7898714
Fixed an issue when the change was on multiple lines
karthikraobr Oct 15, 2018
11a49c5
Lint
karthikraobr Oct 15, 2018
5283575
Fixed other linting issue
karthikraobr Oct 15, 2018
19e22b3
Removed unnecessary variables
karthikraobr Oct 16, 2018
1381a4b
Cleanup#
karthikraobr Oct 16, 2018
5543a04
Master merged
karthikraobr Oct 17, 2018
a21aa23
Merge branch 'master' of https://github.com/Microsoft/vscode-go into …
karthikraobr Oct 17, 2018
ffc5f94
Refactor and avoid unnecessary comment checks
ramya-rao-a Jan 15, 2019
5d7b25f
linting error
ramya-rao-a Jan 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/goCover.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,12 +257,16 @@ export function applyCodeCoverage(editor: vscode.TextEditor) {
* @param e TextDocumentChangeEvent
*/
export function removeCodeCoverageOnFileChange(e: vscode.TextDocumentChangeEvent) {
if (e.document.languageId !== 'go') {
if (e.document.languageId !== 'go' || e.contentChanges.length < 1) {
return;
}
if (isPartOfComment(e)) {
return;
}
if (vscode.window.visibleTextEditors.every(editor => editor.document !== e.document)) {
return;
}

clearCoverage();
}

Expand Down Expand Up @@ -305,3 +309,20 @@ export function toggleCoverageCurrentPackage() {
});
});
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation.

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')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some tests to cover these cases?

Copy link
Contributor Author

@karthikraobr karthikraobr Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

result = false;
return false;
} else {
result = true;
}
});
return result;
}