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

Move Codecov Ignores to LCOV #9726

Merged
merged 1 commit into from
Dec 18, 2019
Merged

Conversation

PrivatePuffin
Copy link
Contributor

@PrivatePuffin PrivatePuffin commented Dec 15, 2019

TL:DR

  • Removes broken codecov ignore
  • Places ignore section ax_code_coverage
  • Forwards users from codecov to LCOV for ignores

Signed-off-by: Kjeld Schouten-Lebbing kjeld@schouten-lebbing.nl

Motivation and Context

After many hours trying to figure out why we/I couldn't get codecov ignore working I have finally solved the problem.

The issue with codecov ignore came out to be two fold:

  1. There is a bug in codecov, which prevents ignore from working, there have been about 3-5 reports about it from last months.
  2. Codecov support is rather unsupportive for some reason, resorting to smokescreening and ignoring all reports of this bug.

This leads to my conclusion:
If we want to ignore folders, we beter not rely on codecov to do it for us.

Description

This PR ignores folders, by removing them from the report before they are send to codecov. As this is actually opensource and maintainable by OpenZFS/ZoL, I expect this to keep be a more sustainable long-term solution and this would even survive changing codecov-report-provider.

Before you ask: Why the wildcard prefix?

  • The path thats actually ignored in the report, is the full path:
    Removing /var/lib/buildbot/slaves/zfs/Ubuntu_17_04_x86_64_Coverage__TEST_/build/zfs/tests/zfs-tests/cmd/file_check/file_check.c
    Not:
    tests/zfs-tests/cmd/file_check/file_check.c

It's a bit of an oddity or bug with lcov, but it does not lead to sideeffects on the ZFS codebase.

Before you ask: Do we really need an ignore at all?
It would be cleaner to ignore tests covering themselves, but yes that wouldn't in itself warrant a change. However some PR's in the future might rely on externally maintained libraries that are not covered by our tests completely, an ignore would be essential to keep codecoverage reports representing actual coverage of zfs. (an example of which is/could be ZSTD)

How Has This Been Tested?

Currently it's a draft. As soon as I get all tests to pass, i'll note it down and open it up for review.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

@PrivatePuffin
Copy link
Contributor Author

@behlendorf , @freqlabs and @rlaager

I finally manage to get the problems with ignoring paths researched and got the ignore working. 💯

@ghost
Copy link

ghost commented Dec 16, 2019

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 16, 2019
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

This does seem to work as intended. There's no longer any mention of the entire tests subdirectory in the codecov report and it's dropped by ~2300 lines which sounds reasonable.

config/ax_code_coverage.m4 Outdated Show resolved Hide resolved
@PrivatePuffin
Copy link
Contributor Author

@freqlabs
It did cross my mind to look into it, but I limited myself to this fix only.

But: I don't think there is much gain in an update either, simply because it doesn't do that much for us and much of its features/flags do not seem to be used by us.

Besides: An update of this file, seems to be beter combined with a complete update of the autoconf stack and combining it with an added ignore flag seems out-of-scope of both.

Copy link
Member

@prakashsurya prakashsurya left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me!

@PrivatePuffin
Copy link
Contributor Author

Thanks for the fast feedback and review guys, appreciate it! :)

@rlaager
Copy link
Member

rlaager commented Dec 16, 2019

.github/codecov.yml is missing the end-of-file newline. (You can see this in the GitHub diff viewer.)

- Removes broken codecov ignore
- Places ignore section in ax_code_coverage
- Forwards users from codecov to LCOV for ignores

Signed-off-by: Kjeld Schouten-Lebbing <kjeld@schouten-lebbing.nl>
@PrivatePuffin
Copy link
Contributor Author

@rlaager Thanks for noticing, totally missed that.
Fixed!

Copy link
Member

@rlaager rlaager left a comment

Choose a reason for hiding this comment

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

LGTM

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 16, 2019
@c0d3z3r0 c0d3z3r0 mentioned this pull request Dec 16, 2019
12 tasks
@codecov
Copy link

codecov bot commented Dec 17, 2019

Codecov Report

Merging #9726 into master will increase coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #9726     +/-   ##
=========================================
+ Coverage      79%      79%    +<1%     
=========================================
  Files         420      385     -35     
  Lines      123654   121282   -2372     
=========================================
- Hits        98136    96340   -1796     
+ Misses      25518    24942    -576
Flag Coverage Δ
#kernel 80% <ø> (ø) ⬇️
#user 67% <ø> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba434b1...ea4cf40. Read the comment docs.

@behlendorf behlendorf merged commit 3035f14 into openzfs:master Dec 18, 2019
@PrivatePuffin PrivatePuffin deleted the F-Codecov branch December 19, 2019 18:58
@c0d3z3r0 c0d3z3r0 mentioned this pull request May 1, 2020
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants