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

Measure test coverage and report to codecov.io #193

Closed
wants to merge 10 commits into from
Closed

Measure test coverage and report to codecov.io #193

wants to merge 10 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 15, 2018

Test-coverage reporting

This pull request introduces coverage metrics for travis_suite, and submits them to codecov.io. See an example for Icepack here. The overview shows what lines of source code are "hit" by the test suite, and which are not. The coverage metrics are submitted to codecov.io if the build succeeds.

In order to obtain accurate testing metrics, the Travis runs run without optimizations (flag -O0). Therefore, the test suite may take slightly longer to complete from now on.

Steps required for CICE-Consortium

Someone with ownership of the Github user CICE-Consortium needs to sign up to codecov.io with the Github user. Afterwards, this person needs to enable the Icepack repository on the codecov page. You may want to enable the CICE repository right away as well. We do not need the token that is generated.

Other changes in this PR

I transitioned from clang to GCC for linking purposes on Travis-CI (like I did with CICE). This should have no practical implications.


Developer(s): Anders Damsgaard, Princeton/NOAA-GFDL (github.com/anders-dc, adamsgaard.dk)

Are the code changes bit for bit, different at roundoff level, or more substantial? There are no changes to the underlying code.

Is the documentation being updated with this PR? (Y/N) No.

If not, does the documentation need to be updated separately? (Y/N) No.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@24e1e4d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #193   +/-   ##
=========================================
  Coverage          ?   75.21%           
=========================================
  Files             ?       45           
  Lines             ?    15409           
  Branches          ?        0           
=========================================
  Hits              ?    11590           
  Misses            ?     3819           
  Partials          ?        0

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 24e1e4d...7735758. Read the comment docs.

@ghost
Copy link
Author

ghost commented Apr 15, 2018

Please beware that the autogenerated coverage comparison in the comment above is somewhat meaningless, as the testing metrics aren't yet determined for the master branch.

EDIT: If desired, we can disable automatic codecov comments in PRs such as the one above by adding a .codecov.yml with the contents comment: off (codecov documentation).

@eclare108213
Copy link
Contributor

@anders-dc Thanks! This is very cool. I don't understand what I'm looking at, though. In the code files, there are lots of instances like this

     do k=1,n_aero                            (green)
        aeroice(k,1) = aeroice(k,1) &            (red)
                     + faero_atm(k)*dt*aicen    (green)
     enddo

How can the line extension be hit and the beginning of the statement not be hit?
Also, there are a lot of lines that I would consider to be 'executable' which are white. What does that mean? This page doesn't say much about that:
https://docs.codecov.io/v4.3.6/docs/viewing-source-code

@ghost
Copy link
Author

ghost commented Apr 16, 2018

Hi Elizabeth,
The situation where a do loop declaration is evaluated, but its associated code block is not, means that the range is empty. I see many compiler warnings like this:

        do mm = 1, n_don
                         1
Warning: DO loop at (1) will be executed zero times

You will see the same pattern if af IF statement conditional evaluates to false. I’ll look up on the question on unmarked/white lines of code.

EDIT: Oh, I might have misunderstood your example. Let me get back to you later.

@ghost
Copy link
Author

ghost commented Apr 16, 2018

It is worth looking at the gcov documentation if the codecov documentation is lacking depth. Codecov is just a frontend, and for GCC-compiled languages parses whatever gcov produces into html. The gcov results can be inspected directly from a local build by running gcov <filename>.gcda && cat <filename>.F90.gcov files, or browsed graphically with lcov.

Here is my take on the white lines. These are typically declarations of functions or variables, or some other syntax component which after a successful compilation will not cause failure. Therefore, they are not counted as part of the coverage metric.

Finally, it seems there is a consistent problem with continuation characters (&). I'm looking into it to see if it's a gcov or codecov thing.

@apcraig
Copy link
Contributor

apcraig commented Apr 16, 2018

I'm wondering if setting this up with travis is the best way. We will always have a highly limited ability to test on travis due to the pe counts and the goal to have relatively short turnaround (30 min max I'd say).

Ideally, I think we'd want to apply the code coverage tool to a large test suite on a non travis machine to get a better idea where we stand. Nothing wrong (except maybe the reduction in optimization) with doing this on travis, although not clear we gain much by doing it all the time on travis. I see value in using this tool occasionally to measure our test coverage on a machine where we can run our full test suite. What would be the best way to do that? Would it be to define a flag (like DEBUG) in the test suite so we can turn on the code coverage flags and then have it ping codecov.io tool? In fact, maybe DEBUG should also turn on the code coverage compiler options tool since it needs -O0. Does this work only with gnu or also other compilers?

@ghost
Copy link
Author

ghost commented Apr 16, 2018

Okay, so gcov is clever enough to treat lines with continuation characters as one statement. However, it seems that information is lost when results from different tests are combined and uploaded to codecov.

@ghost
Copy link
Author

ghost commented Apr 16, 2018

Good point @apcraig. Icepack would probably work better than the more intensive test suite of CICE. The current travis_suite for CICE went from ~30 mins to ~40 mins by turning off optimizations.

It is totally possible to generate the results locally as needed, and upload to codecov using a token.

I do not have experience with compilers other than GCC for this, sorry.

@apcraig
Copy link
Contributor

apcraig commented Apr 16, 2018

Thanks @anders-dc. So what is the best way to proceed? Should we back off the travis compiler changes and think about implementing this more generally. It looks like all we need is the right compiler switches and then the "after success" code to trigger the coverage analysis and tool. Do you have access to a machine that would allow you to prototype this. If not, I'm happy to do it now that you've figured it out. :)

I think it's still good to switch to gcc regardless.

@ghost
Copy link
Author

ghost commented Apr 16, 2018

Sounds good to me. Let me wrap up the entire functionality in a script, where the codecov token is assumed to be defined in an environment variable. I'll update this PR shortly.

@ghost
Copy link
Author

ghost commented Apr 16, 2018

Could you sign up with CICE-Consortium for codecov if you haven't already? Also, please send me the repository token.

@eclare108213
Copy link
Contributor

I granted access to CICE-Consortium at https://codecov.io/login/gh. Is that what is needed? The
cice token is 1d09241f-ed9e-47d8-847c-038bab024b53
I'm not sure what it is for Icepack -- looks like you must have already done that.
e

@ghost
Copy link
Author

ghost commented Apr 18, 2018

Thank you Elizabeth. During the testing telecon it was decided that I will try and implement the code coverage reporting as a flag to the icepack.setup/cice.setup scripts. I will update this PR accordingly.

@apcraig
Copy link
Contributor

apcraig commented Apr 18, 2018

Hi @anders-dc, just a quick follow up from discussions earlier. This is what I would propose. A new variable in cice.settings called ICE_CODECOV or something like that. It would be false by default and then add a set_env.codecov to the options directory that would set it to true. Then, this option is available in an individual test by setting -s codecov or in a test suite by using -s codecov. We will have to verify that when running a test suite, the -s option set on the command line "adds" to the options specified in the test suite, but I believe it does. Then we just need to modify the Macros files (all or some) so when ICE_CODECOV is true, a new set of flags is triggered. We will need to make sure that works with DEBUG TRUE and FALSE and so forth as well. Let me know if that makes sense. Maybe you have a better approach, if so, please feel free to do that.

@ghost
Copy link
Author

ghost commented Apr 19, 2018

Hi @apcraig, that makes a lot of sense! I think we may even be able to avoid modifying the Macros for the build environments. If ICE_CODECOV == true we could append the coverage flags, -g and -O0 to the compiler and linker flag variables. I'm pretty sure -O0 overrides higher optimization levels. If not, we can strip -O{1,2,3} out of CFLAGS and FFLAGS. After the tests have finished, a conditional can check if the coverage files (*.gcno and *.gcda) exist. If true the codecov script is launched, reporting the results to codecov.io.

@apcraig
Copy link
Contributor

apcraig commented May 8, 2018

Hi @anders-dc, what's the status of this PR? Are you waiting for me? Should we move this discussion to an issue and close the PR? No rush, just trying to keep track of things and also wondering if I dropped a ball. thanks.

@ghost
Copy link
Author

ghost commented May 9, 2018

hi @apcraig, sorry for the silence. You are not holding up anything. Let's close this PR, and I will put in a new one with the discussed changes within the next two weeks.

@ghost ghost closed this May 9, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants