ci: use codecov coverage format #3097

davidhewitt · 2023-04-12T06:37:51Z

Newly supported on cargo-llvm-cov 0.5.12, might give some more detailed information.

adamreichold · 2023-04-12T07:16:15Z

The main difference behind the 15 percent drop in coverage seems to be that tracking of error paths is more detailed, e.g. foo()? seems to be covered only if both Ok and Err paths are taken.

This seems to be an improvement even though the numbers do not give me that warm fuzzy feeling.

davidhewitt · 2023-04-12T21:02:32Z

I agree that showing the unhandled error cases is an improvement, however it doesn't seem like there's an easy way to see what the uncovered part of a line is in the codecov UI. This seems like it'll be pretty hard to actually then make meaningful improvements on?

I'm trying to see if there's a way to look locally.

davidhewitt · 2023-04-13T07:59:00Z

So I've been looking at the llvm-cov html report this morning and it shows what regions are covered for each instantiation. Presumably to get 100% region coverage each region just needs to be covered at least once, not for every instantiation.

Interestingly, the codecov numbers seem much more pessimistic than the llvm-cov html report.

Take impl_/pyclass/lazy_type_object.rs, for example. On codecov, this PR, apparently the coverage is 48%

https://app.codecov.io/gh/PyO3/pyo3/pull/3097/blob/src/impl_/pyclass/lazy_type_object.rs

But on the local html report, the function / line / region coverage are all in the 90% range:

@taiki-e @andrewgazelka I wonder if you have an idea why the codecov numbers are so much more pessimistic than the llvm-cov html report?

andrewgazelka · 2023-04-13T08:41:35Z

I'll look into it when I wake up. The way coverage can be determined is kinda subjective but yea I think it would be ideal to match --html

davidhewitt · 2023-04-13T21:00:22Z

Thanks. I can push an update to the noxfile to make it generate the html and codecov reports (and any other formats) if that would be useful.

andrewgazelka · 2023-04-13T21:09:46Z

Thanks. I can push an update to the noxfile to make it generate the html and codecov reports (and any other formats) if that would be useful.

It would be great if it is hard to run the tests locally. Otherwise, I'll just run llvm-cov locally with cargo test. :) Is it that easy to run them locally?

davidhewitt · 2023-04-13T21:18:46Z

It's more complicated than cargo test because we have UI tests and also a python module which is tested using pytest.

I've pushed a commit to this branch which will make the coverage job generate html, llvm-cov json and codecov json formats.

You can run with:

pip install nox
nox -s coverage

andrewgazelka · 2023-04-14T05:10:48Z

@davidhewitt @adamreichold still looking into it...

Also the coverage for the mentioned file lazy_type_object.rs is as follows:

{
      "35": "0/1",
      "36": "0/1",
      "37": "0/1",
      "38": "0/1",
      "39": "0/1",
      "40": "0/1",
      "41": "0/1",
      "42": "0/1",
      "43": "0/1",
      "44": "0/1",
      "49": "15627/15627",
      "50": "15631/15631",
      "51": "4/4",
      "52": "4/4",
      "53": "15627/15627",
      "54": "15627/15627",
      "57": "15658/15658",
      "58": "15658/15658",
      "59": "15658/15658",
      "60": "15658/15658",
      "67": "17064/17064",
      "68": "17064/17064",
      "69": "17064/17064",
      "70": "17064/17064",
      "71": "17064/17064",
      "72": "17064/17064",
      "73": "17064/17064",
      "74": "34128/34128",
      "75": "69111/86177",
      "76": "17066/34128",
      "77": "17058/17064",
      "78": "17064/17064",
      "79": "17070/17070",
      "80": "6/6",
      "81": "6/6",
      "82": "6/6",
      "83": "6/6",
      "84": "6/6",
      "85": "17070/17070",
      "86": "17064/17064",
      "88": "17062/17062",
      "89": "17062/17062",
      "90": "17062/17062",
      "91": "17062/17062",
      "92": "17062/17062",
      "93": "17062/17062",
      "94": "17062/17062",
      "95": "17062/17062",
      "96": "17062/17062",
      "97": "17062/17062",
      "98": "17062/17062",
      "99": "17062/17062",
      "100": "17062/17062",
      "101": "17062/17062",
      "102": "17062/17062",
      "103": "17062/17062",
      "104": "17062/17062",
      "105": "17062/17062",
      "106": "17062/17062",
      "107": "17062/17062",
      "109": "16123/17062",
      "110": "939/17062",
      "111": "939/17062",
      "112": "939/17062",
      "113": "939/17062",
      "114": "939/17062",
      "115": "939/17062",
      "118": "84/17062",
      "119": "855/17062",
      "120": "855/17062",
      "121": "855/17062",
      "122": "855/17062",
      "123": "855/17062",
      "124": "855/17062",
      "125": "855/17062",
      "126": "855/17062",
      "127": "855/17062",
      "128": "855/17062",
      "129": "859/17066",
      "130": "859/17066",
      "131": "867/17074",
      "132": "859/17066",
      "133": "855/17062",
      "134": "855/17062",
      "135": "855/17062",
      "136": "855/17062",
      "137": "855/17062",
      "138": "855/17062",
      "139": "855/17062",
      "140": "855/17062",
      "141": "855/17062",
      "142": "855/17062",
      "143": "855/17062",
      "144": "855/17062",
      "145": "855/17062",
      "146": "4271/34124",
      "147": "3660/34124",
      "148": "1105/34124",
      "149": "128/17062",
      "150": "128/17062",
      "151": "128/17062",
      "152": "124/17062",
      "153": "4/17062",
      "154": "4/17062",
      "155": "4/17062",
      "156": "4/17062",
      "157": "4/17062",
      "158": "4/17062",
      "159": "4/17062",
      "160": "4/17062",
      "161": "4/17062",
      "162": "4/17062",
      "165": "849/17062",
      "171": "1702/17913",
      "172": "851/851",
      "173": "851/851",
      "174": "851/851",
      "175": "851/851",
      "176": "851/851",
      "177": "851/851",
      "178": "851/851",
      "179": "1702/17913",
      "181": "851/34124",
      "182": "0/17062",
      "183": "0/17062",
      "184": "0/17062",
      "185": "0/17062",
      "186": "0/17062",
      "187": "851/17062",
      "188": "851/17062",
      "189": "851/17062",
      "190": "17062/17062",
      "193": "851/851",
      "194": "851/851",
      "195": "851/851",
      "196": "851/851",
      "197": "851/851",
      "200": "975/1702",
      "201": "124/851",
      "202": "124/1702",
      "204": "851/851",
      "205": "851/851",
      "211": "10/10",
      "212": "10/10",
      "213": "10/10",
      "214": "10/10",
      "215": "10/10"
    }

removing non-fully-covered lines:

  {
      "35": "0/1",
      "36": "0/1",
      "37": "0/1",
      "38": "0/1",
      "39": "0/1",
      "40": "0/1",
      "41": "0/1",
      "42": "0/1",
      "43": "0/1",
      "44": "0/1",
      "109": "16123/17062",
      "110": "939/17062",
      "111": "939/17062",
      "112": "939/17062",
      "113": "939/17062",
      "114": "939/17062",
      "115": "939/17062",
      "118": "84/17062",
      "119": "855/17062",
      "120": "855/17062",
      "121": "855/17062",
      "122": "855/17062",
      "123": "855/17062",
      "124": "855/17062",
      "125": "855/17062",
      "126": "855/17062",
      "127": "855/17062",
      "128": "855/17062",
      "129": "859/17066",
      "130": "859/17066",
      "131": "867/17074",
      "132": "859/17066",
      "133": "855/17062",
      "134": "855/17062",
      "135": "855/17062",
      "136": "855/17062",
      "137": "855/17062",
      "138": "855/17062",
      "139": "855/17062",
      "140": "855/17062",
      "141": "855/17062",
      "142": "855/17062",
      "143": "855/17062",
      "144": "855/17062",
      "145": "855/17062",
      "146": "4271/34124",
      "147": "3660/34124",
      "148": "1105/34124",
      "149": "128/17062",
      "150": "128/17062",
      "151": "128/17062",
      "152": "124/17062",
      "153": "4/17062",
      "154": "4/17062",
      "155": "4/17062",
      "156": "4/17062",
      "157": "4/17062",
      "158": "4/17062",
      "159": "4/17062",
      "160": "4/17062",
      "161": "4/17062",
      "162": "4/17062",
      "165": "849/17062",
      "171": "1702/17913",
      "172": "851/851",
      "173": "851/851",
      "174": "851/851",
      "175": "851/851",
      "176": "851/851",
      "177": "851/851",
      "178": "851/851",
      "179": "1702/17913",
      "181": "851/34124",
      "182": "0/17062",
      "183": "0/17062",
      "184": "0/17062",
      "185": "0/17062",
      "186": "0/17062",
      "187": "851/17062",
      "188": "851/17062",
      "189": "851/17062",
      "190": "17062/17062",
      "193": "851/851",
      "194": "851/851",
      "195": "851/851",
      "196": "851/851",
      "197": "851/851",
      "200": "975/1702",
      "201": "124/851",
      "202": "124/1702",
    },

andrewgazelka · 2023-04-14T05:29:21Z

I think the way they calculate code coverage is different. For instance

shows there are only 52 regions, when in fact there are many more (given all the generics). Just click on the file in the HTML report.

I think the html report might be reporting a ✅ if there is at least one generic version where the full region is covered. Do we want this? I could see there being different behaviors for each generic.

tbh I am not sure though... I don't really understand the normal .json file completely so I might be missing something. I might have to reverse-engineer this, which I don't have a lot of time to do right now—unless anyone can help me.

davidhewitt · 2023-04-14T06:16:18Z

I think with the generics it's the same region instantiated many times over? Potentially if that region is covered by at least one instantiation, it's considered covered?

If that's true, the trick may be to identify equivalent regions?

davidhewitt · 2023-04-14T06:20:06Z

EDIT: rereading your comment I see you think exactly that too. It seems correct to me to merge instantiations in this way. Otherwise to get perfect coverage you have to test every branch of your function with every type you instantiate. Plus your users will instantiate your generic functions with types you may never even know about, so "coverage of all generic instantiations" doesn't seem like a helpful property.

andrewgazelka · 2023-04-14T06:36:18Z

Ok that logic makes sense. That's definitely the issue in my program then. I'll have a go at this tomorrow, I have a sense of how I'll approach this.

andrewgazelka · 2023-04-14T20:12:41Z

Hey @davidhewitt and @adamreichold, could you take a look at taiki-e/cargo-llvm-cov#255 and see if it resolves this issue? Thanks! :)

davidhewitt · 2023-04-16T21:52:31Z

Thanks, I was busy over the weekend, will try to get to testing this out tomorrow evening.

davidhewitt · 2023-04-19T07:17:13Z

I just reran this and the numbers look much more like what I'd expect now, thanks @andrewgazelka !

@adamreichold I'm going to proceed to merge this now, I think showing the uncovered errors is strictly an improvement!

davidhewitt · 2023-04-19T07:17:24Z

bors r+

bors · 2023-04-19T07:59:28Z

Build succeeded:

conclusion

davidhewitt changed the title ~~use codecov coverage format~~ ci: use codecov coverage format Apr 12, 2023

davidhewitt force-pushed the codecov-coverage branch from e2aad4f to ff6722d Compare April 12, 2023 06:38

davidhewitt added the CI-skip-changelog Skip checking changelog entry label Apr 12, 2023

adamreichold mentioned this pull request Apr 12, 2023

Switch coverage format for Codecov uploads to the native one. PyO3/rust-numpy#374

Merged

ci: use codecov coverage format

d8f7e6b

davidhewitt force-pushed the codecov-coverage branch 2 times, most recently from ae9d390 to d8f7e6b Compare April 19, 2023 06:23

bors bot merged commit 2f8bf51 into PyO3:main Apr 19, 2023

alxhill mentioned this pull request Feb 11, 2024

docs: Add lcov / codecov options for nox coverage and update docs #3825

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: use codecov coverage format #3097

ci: use codecov coverage format #3097

davidhewitt commented Apr 12, 2023

adamreichold commented Apr 12, 2023

davidhewitt commented Apr 12, 2023

davidhewitt commented Apr 13, 2023

andrewgazelka commented Apr 13, 2023

davidhewitt commented Apr 13, 2023

andrewgazelka commented Apr 13, 2023 •

edited

Loading

davidhewitt commented Apr 13, 2023

andrewgazelka commented Apr 14, 2023 •

edited

Loading

andrewgazelka commented Apr 14, 2023 •

edited

Loading

davidhewitt commented Apr 14, 2023

davidhewitt commented Apr 14, 2023 •

edited

Loading

andrewgazelka commented Apr 14, 2023

andrewgazelka commented Apr 14, 2023 •

edited

Loading

davidhewitt commented Apr 16, 2023

davidhewitt commented Apr 19, 2023

davidhewitt commented Apr 19, 2023

bors bot commented Apr 19, 2023

ci: use codecov coverage format #3097

ci: use codecov coverage format #3097

Conversation

davidhewitt commented Apr 12, 2023

adamreichold commented Apr 12, 2023

davidhewitt commented Apr 12, 2023

davidhewitt commented Apr 13, 2023

andrewgazelka commented Apr 13, 2023

davidhewitt commented Apr 13, 2023

andrewgazelka commented Apr 13, 2023 • edited Loading

davidhewitt commented Apr 13, 2023

andrewgazelka commented Apr 14, 2023 • edited Loading

andrewgazelka commented Apr 14, 2023 • edited Loading

davidhewitt commented Apr 14, 2023

davidhewitt commented Apr 14, 2023 • edited Loading

andrewgazelka commented Apr 14, 2023

andrewgazelka commented Apr 14, 2023 • edited Loading

davidhewitt commented Apr 16, 2023

davidhewitt commented Apr 19, 2023

davidhewitt commented Apr 19, 2023

bors bot commented Apr 19, 2023

andrewgazelka commented Apr 13, 2023 •

edited

Loading

andrewgazelka commented Apr 14, 2023 •

edited

Loading

andrewgazelka commented Apr 14, 2023 •

edited

Loading

davidhewitt commented Apr 14, 2023 •

edited

Loading

andrewgazelka commented Apr 14, 2023 •

edited

Loading