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

Always consider empty files as 100% tested #45

Merged
merged 3 commits into from
Dec 12, 2014

Conversation

neonichu
Copy link
Member

@neonichu neonichu commented Dec 2, 2014

I think it is better to avoid "false positive" untested classes.

@kylef
Copy link
Contributor

kylef commented Dec 2, 2014

@neonichu Can you add a test proving this works please?

@jkrumow
Copy link
Collaborator

jkrumow commented Dec 2, 2014

@neonichu @kylef
Looks like this pull request is overlapping with this one: #40
It would be nice if we could make the changes in coverageFile.rb for line and branch coverage so all reporters would use the same metrics (simple_output + cobertura_xml_output at least).

@@ -102,7 +102,11 @@ def rate_lines_tested
end

def percentage_lines_tested
rate_lines_tested * 100
if num_lines_testable == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I know the line_rate should also be set to 1.0 under that condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

@neonichu should we make that change too, for corbetura?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessarily. The cobertura output generator checks it as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. Merging then!

@marklarr
Copy link
Contributor

marklarr commented Dec 7, 2014

This looks good, but let's add some tests like @kylef said

@neonichu
Copy link
Member Author

I have updated the two existing tests which had zero-length files in them. Should've done that initially 😞

marklarr added a commit that referenced this pull request Dec 12, 2014
Always consider empty files as 100% tested
@marklarr marklarr merged commit ca1d8a6 into SlatherOrg:master Dec 12, 2014
marklarr pushed a commit that referenced this pull request Dec 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants