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

Support Additional lcov_result Args #38

Merged

Conversation

mwcondino
Copy link
Contributor

@mwcondino mwcondino commented Nov 4, 2024

Purpose

This PR adds support for a new lcov_args argument. This allows users of colcon-lcov-result to pass through any additional command line options to the underlying lcov call.

Context

After moving a ROS 2 stack over to jazzy, I found that colcon lcov-result was not working quite properly. Without any modification, I was finding zero output in coverage.info. After some digging, it looks like the problem may be that lcov was exiting early on encountering errors. Specifically, I found that adding --ignore-errors mismatch allowed the underlying lcov call to work as expected (I think --keep-going would work too).

Though this PR doesn't address the root cause of the issue, it at least provides users a way to add custom args easily.

Alternative implementations

I considered adding a more structured argument, perhaps something like --ignore-errors to colcon lcov-result. I wanted to get a bit of feedback before implementing that, though, and the current patch was simple enough to put up as a PR.

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

An 'escape hatch' like this is probably good to have anyway. Thanks for following the same pattern as --cmake-args.

Please file a new issue discussing the regression you're experiencing. If nothing else, it would be a great spot to document your workaround using this flag until a proper solution is implemented.

@mwcondino
Copy link
Contributor Author

An 'escape hatch' like this is probably good to have anyway. Thanks for following the same pattern as --cmake-args.

Please file a new issue discussing the regression you're experiencing. If nothing else, it would be a great spot to document your workaround using this flag until a proper solution is implemented.

Great call, just created an issue here and referenced this PR. Thanks for the very quick review!

Copy link
Collaborator

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Agreed, this is nice to have. Thanks for the PR, and thanks for opening an issue.

@christophebedard christophebedard merged commit f8b4b11 into colcon:master Nov 5, 2024
17 checks passed
@christophebedard
Copy link
Collaborator

I released this as 0.5.3. It should now be available from PyPI and should be available from the apt repo eventually (ros-infrastructure/reprepro-updater#217).

@mwcondino mwcondino deleted the mwc/support-addtl-lcov-args branch November 5, 2024 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants