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

Temporarily work around lost code coverage reports #1892

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Oct 6, 2022

Motivation and Context

Builds on main are getting a lot of these errors due to the coverage being collected after the Audio build (originating from #1646), losing the coverage collected earlier:

libgcov profiling error:/home/circleci/project/habitat-sim/build/esp/bindings/CMakeFiles/habitat_sim_bindings.dir/Bindings.cpp.gcda:overwriting an existing profile data with a different timestamp

With this change the coverage is back at 60% (from 40%), and still includes also everything from the Audio build.

The better long-term solution, however, would be like something in #1860 -- this unnecessarily duplicates a lot of the CI setup code. (@Skylion007 or anybody else who knows YAML or CircleCI, do you know if there is a possibility to reuse / deduplicate the markup somehow?)

How Has This Been Tested

Comparing these two reports gives a ~20% increase in code coverage:

As far as I checked, the Audio build coverage (such as src/esp/sensor/AudioSensor.cpp) seems to stay the same, so the second coverage upload gets merged with the first one. That's only from a cursory look though, if you see anything suspicious in the new reports, let me know.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 6, 2022
# Replaces -1 linecount with zero to prevent lcov from crashing:
# https://github.com/psycofdj/coverxygen/issues/6
sed -i -e 's/,-1$/,0/g' coverage.info
#lcov --remove coverage.info "*/deps/*" --output-file coverage.info > /dev/null
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question, we hy are these commented out? Don't we still not want dependencies to count towards coverage? At the very lest we should add these globs to .coveragerc then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just copied what was there from the other part :) I don't want to touch anything here, only want to fix what's broken.

Will use the yaml anchor, thanks.

Copy link
Contributor

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

Other than that, looks good.

@Skylion007
Copy link
Contributor

Skylion007 commented Oct 6, 2022

(@Skylion007 or anybody else who knows YAML or CircleCI, do you know if there is a possibility to reuse / deduplicate the markup somehow?)

You absolute can deduplicate it by either using yaml anchors or writing a custom command. For the anchor see how install_cuda is deduplicated:

- run: &install_conda

Here is an example of a circleci specified command:

install_all_ubuntu_deps:

Getting a lot of these due to the coverage being collected after the
Audio build, losing the coverage collected earlier:

libgcov profiling error:/home/circleci/project/habitat-sim/build/esp/bindings/CMakeFiles/habitat_sim_bindings.dir/Bindings.cpp.gcda:overwriting an existing profile data with a different timestamp

With this change the coverage is back at 60% (from 40%), and
still includes also everything from the Audio build. The better
long-term solution, however, would be like something in
#1860.
@mosra mosra force-pushed the codecov-gimme-back-those-20-percent branch from 82fdc0b to 8267a6d Compare October 6, 2022 20:44
@mosra mosra merged commit befd171 into main Oct 7, 2022
@mosra mosra deleted the codecov-gimme-back-those-20-percent branch October 7, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants