-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: Clover PHP parser #99
Conversation
Pull Request Test Coverage Report for Build 6048904071
💛 - Coveralls |
Looking good. Thanks, @iurev! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. Have you tested the binary locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need such bug file (2k lines). Could you keep about 100 lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have smaller files for tests.
# NOTE: Provide the base path for the sources. You can check "filename" in | ||
# coverage report and see what part is missing to get a valid source path. | ||
def initialize(@base_path : String?) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this initializer here since we don't use @base_path
end | ||
|
||
def matches?(filename) : Bool | ||
return true if /clover.xml/.matches?(filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't check by name here because people can rename the Jacoco file to clover.xml and call the coverage-reporter as coveralls report clover.xml
. In this case we should check its content instead of name. Name is only important when selecting files via globs (calling as coveralls report
).
|
||
if line_type == "cond" | ||
branch_hits = line_node.attributes["truecount"].content.to_u64 | ||
branches[line_number] = [branch_hits] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that there won't be more than one branch hit on a line? What do you think of using branches[line_number] << branch_hits
?
2bdf7c3
to
02525b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. |
Closes:
https://github.com/coverallsapp/coveralls/issues/1889
lemurheavy/coveralls-public#1721 (comment)
Closes #53
⚡ Summary
Add Clover PHP parser
☑️ Checklist