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

additional tests to improve coverage #2560

Merged
merged 3 commits into from
Nov 27, 2020
Merged

additional tests to improve coverage #2560

merged 3 commits into from
Nov 27, 2020

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Nov 25, 2020

No functionality is changed in this PR

@bkamins bkamins added CI non-breaking The proposed change is not breaking labels Nov 25, 2020
@bkamins bkamins added this to the 1.0 milestone Nov 25, 2020
@bkamins
Copy link
Member Author

bkamins commented Nov 25, 2020

additional coverage improvement is in #2554. I will come back to that PR tomorrow to finalize it.

@bkamins
Copy link
Member Author

bkamins commented Nov 26, 2020

@nalimilan - this should be good for a review

nalimilan
nalimilan reviewed Nov 26, 2020
View reviewed changes
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Thanks! I've just spotted a few tests that seem a bit out of place. It's always hard to find out to organize tests...

test/broadcasting.jl Outdated Show resolved Hide resolved
test/broadcasting.jl Outdated Show resolved Hide resolved
test/constructors.jl Outdated Show resolved Hide resolved
test/dataframerow.jl Outdated Show resolved Hide resolved
test/dataframerow.jl Show resolved Hide resolved
test/grouping.jl Outdated Show resolved Hide resolved
test/grouping.jl Outdated Show resolved Hide resolved
test/reshape.jl Outdated Show resolved Hide resolved
bkamins and others added 2 commits November 26, 2020 23:37
@bkamins @nalimilan
Apply suggestions from code review
5472bcb 
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@bkamins
fixes after the review
03b9ba9
@bkamins
Copy link
Member Author

bkamins commented Nov 26, 2020

updated

nalimilan
nalimilan approved these changes Nov 27, 2020
View reviewed changes
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

98.24% coverage! Maybe we can aim at 100% one day! :-)

@bkamins
Copy link
Member Author

bkamins commented Nov 27, 2020

Maybe we can aim at 100% one day! :-)

No 😄, because:

  1. there are some lines that CI skips (e.g. local variable_name statement)
  2. there are some lines that should be never executed (error messages in code paths that should not be normally reachable)

But we should be close.

Thank you for reviewing this.

@bkamins bkamins merged commit 3689047 into JuliaData:master Nov 27, 2020
@bkamins bkamins deleted the improve_test_coverage branch November 27, 2020 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nalimilan nalimilan nalimilan approved these changes

Assignees
No one assigned
Labels
CI non-breaking The proposed change is not breaking
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@bkamins @nalimilan