Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parser for GoCov coverage #160

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rekathiru
Copy link

@rekathiru rekathiru commented Feb 24, 2025

This GoCov parser has been implemented in order to parse the unit tests coverage generated by the go underlying coverage tool(GoCov) with the command as below:

go test ./... -race -coverprofile=coverage.txt -covermode=atomic

Output from above command would be:

mode: atomic
github.com/dev/crd/pkg/aws/services.go:72.81,76.16 3 3
github.com/dev/crd/pkg/aws/services.go:76.16,77.41 1 2
github.com/dev/crd/pkg/aws/services.go:77.41,78.23 1 2

Where every line would have the coverage details as below:

domain_name/root_dir/repo/dir_path.../file_name:<start-line>.<start-column>,<end-line>.<end-column> <statement-count>  <execution-count>

So, the execution-count would say how many times the statements-count executed while running the tests. If the execution-count is above 1, then the statements-count has been executed at least once. So, we mark the start-line to end-line as covered. Otherwise, it will go into coverage miss.

Testing done

  • Relevant unit tests have been added.
  • Actively used by multiple projects in Intuit

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@rekathiru rekathiru changed the title adding go parser Add parser for Go coverage Feb 24, 2025
@rekathiru rekathiru changed the title Add parser for Go coverage Add parser for GoCov coverage Feb 24, 2025
@rekathiru rekathiru closed this Feb 25, 2025
@rekathiru rekathiru reopened this Feb 25, 2025
@uhafner uhafner added the feature New features label Feb 25, 2025
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

I'm not sure what to do with this PR. It is hardly reviewable. The problem seems to be so simple, but the implementation is so complex and non-standard.

For my perspective we have a stream of lines that contains the information which lines of a given file are covered, and which are not. This information will be updated continuously. The the result could be a map of files to covered and missed lines.

Maybe we should consider to create this from scratch?

Comment on lines +200 to +206
// Commenting out the following lines to avoid throwing an exception when merging coverage information
// from Jacoco. This is a temporary solution until we have a better way to handle this.
// else {
// throw new IllegalArgumentException(
// String.format("Cannot merge coverage information for line %d in %s",
// line, this));
// }
Copy link
Member

Choose a reason for hiding this comment

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

Why is that required?

Copy link
Author

Choose a reason for hiding this comment

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

This is actually not part of this PR. I will remove it.

* @return fileNode
*/
@CanIgnoreReturnValue
public FileNode addCounterIncremental(final int lineNumber, final int covered, final int missed) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the motivation for this method. As far as I understand the coverage format we only have the information covered or not for each line. I.e., we do not evaluate branch coverage, just line coverage. Line coverage is stored here in these counters with either 1, 0 or 0,1. There is no value > 1.

Copy link
Author

Choose a reason for hiding this comment

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

Exactly, we have converted the coverage format generated by GoCov to if a line is coverage or not by using 1 or 0. We are not using any value greater than 1.

throw new InvalidGoCoverageFormatException("Expected the coverage mode specification, but got: [" + line + "]");
}
//coverage mode
CoverMode coverMode;
Copy link
Member

Choose a reason for hiding this comment

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

The cover mode is not used at all?

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. I can review this and update it.

@rekathiru
Copy link
Author

rekathiru commented Feb 28, 2025

Thank you for the review comments.

I'm not sure what to do with this PR. It is hardly reviewable. The problem seems to be so simple, but the implementation is so complex and non-standard.

For my perspective we have a stream of lines that contains the information which lines of a given file are covered, and which are not. This information will be updated continuously. The the result could be a map of files to covered and missed lines.

yes..This is correct. But we wanted to calculate coverage by packages and finally by the project/module. So, we tried to reuse existing implementation for that.

github.com/dev/crd/pkg/aws/services.go:72.81,76.16 3 3

In here, github.com is assumed as the project/module and the rest dev, crd, pks, aws are packages. services.go is the class file. Since all these in the flat structure, we are initializing module, package and file while reading the same line.

Maybe we should consider to create this from scratch?

This parser works as expected by giving right coverage details for GOCOV. Please share your thoughts around if anything needs to be changed still.

@uhafner
Copy link
Member

uhafner commented Mar 1, 2025

I'm in holidays for one week, I will come back to your PR after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants