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.

Sign up for GitHub

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

Jump to bottom

Coverage for failing tests #58

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Coverage for failing tests #58

wants to merge 4 commits into from

Conversation

briederer
Copy link

Adds a safety measure to still process .cov files even when tests are failing.

Closes #57

@briederer briederer marked this pull request as draft September 16, 2024 11:09
@briederer
Copy link
Author

Sorry for messing with the tests.
I wanted to achieve that the tests for this package are succeeding while in general they generate_coverage still fails.
Therefore it was neccessary to add the should_throw argument to test_coverage function.
This checks now that the .cov files are processed correctly even if generate_coverage still fails when one or more tests are failing.

@briederer briederer marked this pull request as ready for review September 16, 2024 11:26
@briederer
Copy link
Author

@tpapp Ready for review

tpapp
tpapp reviewed Sep 18, 2024
View reviewed changes
Copy link
Collaborator

@tpapp tpapp left a comment

Choose a reason for hiding this comment

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

thanks, but I don't understand why some code was removed, please explain

src/LocalCoverage.jl Outdated Show resolved Hide resolved
test/runtests.jl
@test !isnothing(match(table_header, table))
@test !isnothing(match(table_line, table))
@test !isnothing(match(table_footer, table))

Copy link
Collaborator

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Author

Choose a reason for hiding this comment

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

It was not removed. It was moved up to line 42 and beyond.
The reason is, that for the new test case of a failing test in the dummy package no coverage is returned, but instead throws an error. Therefore all the tests using cov cannot be processed, after the test has thrown.

So there is no change in the tests for the old ones, but an adaption for the test of the new feature.

test/runtests.jl

@info "Printing coverage information for visual debugging"
show(stdout, cov)
show(IOContext(stdout, :print_gaps => true), cov)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Author

Choose a reason for hiding this comment

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

Same as before.

@briederer
Copy link
Author

Thanks for the review. Added a clarification comment to your question

@briederer @tpapp
Remove empty line
5c84768 
Co-authored-by: Tamas K. Papp <tkpapp@gmail.com>
@briederer
Copy link
Author

Short reminder

@tpapp
Copy link
Collaborator

tpapp commented Oct 10, 2024

Thanks, I appreciate the reminder. The code now looks good to me.

I am wondering if proceeding in case of an error is always desirable. Would an argument that controls this for generate_coverage make sense? The new behavior could be the default.

Can you please add a section (eg # Error handling) to the documentation of generate_coverage that explains the new behavior? Just that errors are caught and coverage is still generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpapp tpapp tpapp left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Coverage for failing tests
2 participants
@briederer @tpapp