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

Colcon extension for coverage results #110

Closed
jpsamper2009 opened this issue Sep 13, 2018 · 12 comments
Closed

Colcon extension for coverage results #110

jpsamper2009 opened this issue Sep 13, 2018 · 12 comments
Labels
enhancement New feature or request

Comments

@jpsamper2009
Copy link

jpsamper2009 commented Sep 13, 2018

Description

  • As a follow up for Add build option for coverage #94, I want to work on a colcon extension to parse gcov/lcov coverage result files. I'm thinking of naming it colcon-lcov-result. Below is my proposal and I'm looking for some feedback before I dive in. Thanks!

Motivation

To be able to visualize coverage results in C/C++ code using the lcov tool

Proposed workflow

colcon build --mixin coverage-gcc
colcon test
colcon lcov-result
chromium-browser lcov/index.html

For a single package:

colcon lcov-result  --zero-counters
colcon test --packages-select my_pkg
colcon lcov-result --packages-select my_pkg
chromium-browser lcov/index.html

To find packages with no coverage:

colcon build --mixin coverage-gcc
colcon lcov-result --baseline
colcon test
colcon lcov-result
chromium-browser lcov/index.html

Command and options

  • colcon lcov-result:

    1. Calls lcov --capture ... for each package
    2. If a baseline is available, it combines the results of each package with its baseline (see below for more details)
    3. Combines the results of all selected packages into one coverage result such that it can generate a static html site (in ros2_ws/lcov/) (lcov --add-tracefile + genhtml)
  • colcon lcov-result --baseline:

    • Calls lcov --initial --capture... to generate a zero-coverage baseline for each package
      • If a file has zero tests, lcov will ignore it. This baseline combined with the actual coverage will reveal these files (see ii. above)
  • colcon lcov-result --zero-counters:

    • Calls lcov --zerocounters ... on each package
    • This would allow you to clear the results and re-run tests
      • For example, if you initially ran two tests and subsequently want to run them individually.
@jpsamper2009
Copy link
Author

I can also submit a rough first version if it helps to have something more concrete with which to discuss.

@dirk-thomas dirk-thomas added the enhancement New feature or request label Sep 14, 2018
@dirk-thomas
Copy link
Member

For the scope of only targeting lcov that sounds ok. It might be good if the command line invocation could also print some results to the console (rather than only generating html files).

In the context of coverage in general I think it would make sense to cover different coverage reports, e.g. for Python packages. A tool like colcon coverage-result could provide the generic interface and functionality and use an extension point which additional packages could contribute extensions for (e.g. lcov or Coverage.py).

I haven't thought about it too much if a separate verb like coverage-result would be "better" than making the existing verb test-result more capable / flexible.

@jpsamper2009
Copy link
Author

@dirk-thomas

For the scope of only targeting lcov that sounds ok. It might be good if the command line invocation could also print some results to the console (rather than only generating html files).

How about if it prints the overall rate:

Overall coverage rate:
  lines......: 70.4% (3038 of 4314 lines)
  functions..: 55.6% (426 of 765 functions)
  branches...: 44.4% (284 of 640 branches)

In the context of coverage in general I think it would make sense to cover different coverage reports, e.g. for Python packages. A tool like colcon coverage-result could provide the generic interface and functionality and use an extension point which additional packages could contribute extensions for (e.g. lcov or Coverage.py).

  • Would it make sense to implement colcon-lcov-result and colcon-coverage-py-result separately at first, and then see if there is a natural way to consolidate them into a colcon-coverage-result module with extension points?

I haven't thought about it too much if a separate verb like coverage-result would be "better" than making the existing verb test-result more capable / flexible.

  • Same as above, except they would get consolidated into a colcon test-result extension?
  • In that case, I think we may also need a --with-lcov option for colcon test that would run the baseline step before running tests
    • It seems awkward to call colcon test-result --baseline before colcon test (this is also the case with colcon-lcov-result)

@dirk-thomas
Copy link
Member

dirk-thomas commented Sep 14, 2018

How about if it prints the overall rate:

That sounds great. A package specific rate would also be good.

Would it make sense to implement colcon-lcov-result and colcon-coverage-py-result separately at first, and then see if there is a natural way to consolidate them into a colcon-coverage-result module with extension points?

Sure, that is possible. From past experience that usually increases the chance that after the first step the second never happens 😉

Same as above, except they would get consolidated into a colcon test-result extension?

Here it sounds a bit more reasonable to keep them separate. Especially since it will take a bit until the coverage stuff settles while being actively designed / developed.

I think we may also need a --with-lcov option for colcon test that would run the baseline step before running tests

That sounds like a highly custom option to me. Only works on some platforms, only works when the previous build was run with appropriate flags. So calling colcon coverage-result --baseline sounds more reasonable to me (atm).

You could imagine a test-result --baseline option for the existing tests too which snapshots the current state and on a repeated invocation can show the delta. So keeping this logic in the test|coverage-result verb sounds reasonable to me.

@jpsamper2009
Copy link
Author

@dirk-thomas Could you look at the first iteration that I have published https://github.com/ApexAI/colcon-lcov-result? It would be great to merge it to the colcon project and make it available via pip.

@dirk-thomas
Copy link
Member

dirk-thomas commented Oct 6, 2018

It would be great to merge it to the colcon project ...

In order to move the existing repository please transfer it to my username. From there I can then transfer it to the colcon org unit. Afterwards I will grant you the admin role for the colcon/colcon-lcov-result` repository (and you can add additional users to the repo if you like).

... and make it available via pip.

Once the repository has been moved you should be able to level the version number for the first release, tag the version and release the package to PyPI on your own (as well as any future releases). I would just ask you to add my PyPI username (which is dthomas) to the release package so that I can "take over" or "assign others" in case this repo ever changes maintainers.

If you also like Debian package to be created you will have to poke me for now to do the release (every time you made a PyPI release, since we can't grant SSH access to the apt repo atm).

@jpsamper2009
Copy link
Author

@dirk-thomas Thanks! I have transferred the project to you

@dirk-thomas
Copy link
Member

And transferred to the colcon org unit and added you as an admin.

@dirk-thomas
Copy link
Member

Btw the readme file has the extension rst but used markdown syntax.

@dirk-thomas
Copy link
Member

@jpsamper2009 Can this ticket be closed?

Btw the readme file has the extension rst but used markdown syntax.

While this is still pending I would rather track it in the corresponding repo (if not even easier to just resolve it right away).

@jpsamper2009
Copy link
Author

@dirk-thomas Agreed

@jpsamper2009
Copy link
Author

@dirk-thomas You should have access to the PyPI project now. Let me know if that is not the case.

Also, I don't think we need debians for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

2 participants