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

feat: collect code coverage and integrate codecov #1170

Merged
merged 1 commit into from
Jul 23, 2020
Merged

feat: collect code coverage and integrate codecov #1170

merged 1 commit into from
Jul 23, 2020

Conversation

alsami
Copy link
Member

@alsami alsami commented Jul 21, 2020

  • Unify test-projects to use TFM netcoreapp3.1, same for Autofac.Benchmark
  • Add coverlet.collector and coverlet.msbuild to Directory.Buiild.props file located in /test
  • Collect coverage while executing tests
  • Add function to install ReportGenerator
  • Allow conditional creation of coverage-report
  • Ignore coverage-folder and files

Pipeline that executed the report-generation:
https://ci.appveyor.com/project/Autofac/autofac/builds/34226546

@alsami alsami changed the base branch from develop to v6 July 21, 2020 17:17
@alsami
Copy link
Member Author

alsami commented Jul 21, 2020

@tillig @alistairjevans ready to review.

Code coverage is actually pretty decent.
image
image

@alsami alsami changed the title feat: collect code-coverage feat: collect code coverage and generate report on demand Jul 21, 2020
Copy link
Member

@alistairjevans alistairjevans left a comment

Choose a reason for hiding this comment

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

Looks good, a few tweaks.

I think the value in these changes comes if we can see the report during CI. It would be good to be able to access that report through the appveyor artifacts somehow, even if it's bit manual for now.

build/Autofac.Build.psm1 Outdated Show resolved Hide resolved
build/Autofac.Build.psm1 Outdated Show resolved Hide resolved
@alsami
Copy link
Member Author

alsami commented Jul 21, 2020

It would be good to be able to access that report through the appveyor artifacts somehow, even if it's bit manual for now.

Include the reports in the artifacts. Means also that they r now always being generated. Takes a little less than one second.
image

@alistairjevans
Copy link
Member

Possibly worth considering the AppVeyor CodeCov integration: https://www.appveyor.com/blog/2017/03/17/codecov/

The dotnet test output might be able to feed into it? Not sure.

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

Possibly worth considering the AppVeyor CodeCov integration: https://www.appveyor.com/blog/2017/03/17/codecov/

The dotnet test output might be able to feed into it? Not sure.

Will try it out and put together the steps required, since I can't do it for this repository.

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

Okay, that's pretty easy to use. Good idea @alistairjevans

Here a sample from my fork (report still being processed).

Steps done for this:

  • Navigate to https://codecov.io/
  • Sign-In using github and grant access to the Github App
  • Choose the repository you want to use and you will get an upload token
  • Run Invoke-WebRequest -Uri 'https://codecov.io/bash' -OutFile codecov.sh
  • Run bash codecov.sh -f "coverage.opencover.xml" -t <the-token-here>

Should we add it to this repo for the beginning? Then at someone needs to get a Token for the actual organization repository.

Edit:
It seems like that codecov has a bit of a different way to generate the coverage reports. I tried using the opencover-xml file but it does never fully parse the results. Couldn't find anything on the web saying it's supported or not supported to use the opencover format.

@alistairjevans
Copy link
Member

It looks like the generated dashboard from codecov you linked to is not excluding the specified files, did you test with a different data source?

As you say, it currently seems to generate quite different results from the code cover report.

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

As you say, it currently seems to generate quite different results from the code cover report.

Got it now, the coverage.info file is exactly what we needed, you might need to CTRL+F5 bc of browser caching.

I could create the sample using my fork and check it in for now, someone else has to configure it completely for Organization level access though.

How should we proceed?

@alistairjevans
Copy link
Member

@alsami, where do we call the codecov bash file from? Is it from inside our own build script? Just that I can't see any changes in this PR to do that, so wondered how it works.

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

where do we call the codecov bash file from? Is it from inside our own build script? Just that I can't see any changes in this PR to do that, so wondered how it works.

Yes, didn't add because I'd need to set variables for the runners (token for codecov).
Basically we would only need to do the following in the build-script after running the tests

  • Invoke-WebRequest -Uri 'https://codecov.io/bash' -OutFile codecov.sh
  • bash codecov.sh -f "coverage.info" -t <the-token-here>

Should I add it? Then pipeline would fail now.

Secondly, should we additional keep the manual reports, in case someone want to download them?

@alistairjevans
Copy link
Member

@tillig, did you want to check whether you're happy with integrating CodeCov? If so, you would need to set up the account and whatnot as described by @alsami. We should end up with CodeCov posting code coverage results into our issues (should look something like dotnet/roslyn-analyzers#3643 (comment)).

@alsami, I'd say commit all the changes into this PR, the build will break, but once the other setup is in place, this PR should show us our first CodeCov results right?

As for whether to retain the report...I'd say maybe no? Just because CodeCov already let's you view a drill-down, and I'd probably rather have the single source of truth? But I'm not 100% certain, so if you'd rather keep the downloadable report I don't mind.

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

I'd say commit all the changes into this PR, the build will break, but once the other setup is in place, this PR should show us our first CodeCov results right?

Yes, as far as I've understood, this is how it should work, maybe it will also just work on the next PR. Then it will definitely display regressions in the coverage.

As for whether to retain the report...I'd say maybe no? Just because CodeCov already let's you view a drill-down, and I'd probably rather have the single source of truth? But I'm not 100% certain, so if you'd rather keep the downloadable report I don't mind.

Agree, someone still might wants to download it for offline access just before a flight or something to waste time writing tests? 😆

@tillig
Copy link
Member

tillig commented Jul 22, 2020

Looks like there are some merge conflicts based on the Linux build changes I pushed into v6.

I attached it to the Autofac repo at the org level. It says you don't need an upload token from AppVeyor on public repositories, though try it out and let me know if it turns out we do. I have such a token, but if we don't need it, then no need to post it or secure it or whatever.

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

Waiting for #1173 and then rebase.

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

Looks like it is indeed working without a token.
Not sure where it was made for now though.

image

Here the link:
https://codecov.io/github/autofac/Autofac/commit/69a66c68dc45b8d975eb39d1ccef565bd95fbbea

It's also gonna start working from the second PR on, bc there are no previous reports available.

image

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

@tillig could you get me the badge markdown data available here?
https://codecov.io/gh/Autofac/Autofac/settings/badge

I'd be able to add it to the README.md then.

@tillig
Copy link
Member

tillig commented Jul 22, 2020

[![codecov](https://codecov.io/gh/Autofac/Autofac/branch/develop/graph/badge.svg)](https://codecov.io/gh/Autofac/Autofac)

codecov

@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##             v6    #1170   +/-   ##
=====================================
  Coverage      ?   80.11%           
=====================================
  Files         ?      188           
  Lines         ?     4370           
  Branches      ?      945           
=====================================
  Hits          ?     3501           
  Misses        ?      416           
  Partials      ?      453           

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 0c3cf64...9e8a859. Read the comment docs.

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

There it is! Looks like it does only recognize it when the badge is available. Comparison does not work until there are previous reports available.

Ready to review I'd say.

@tillig
Copy link
Member

tillig commented Jul 22, 2020

I don't know if it made a difference, but when I went to get the badge there was a prompt at the top of the screen to click a button to "install the CodeCov app" in the org, which... is what I thought I did, but apparently signing the repos up for coverage doesn't also install the app. It could be that me clicking more buttons made magic happen. But, regardless, it's a pretty cool thing to see. Nice work!

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

It could be that me clicking more buttons made magic happen. But, regardless, it's a pretty cool thing to see. Nice work!

That actually makes more sense. Nice anyway that it is working now.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

This is looking great. I'm excited to get this merged in; we can maybe use this as a template for other repos, too! Couple minor things to make sure it still works for Windows and local builds.

README.md Outdated Show resolved Hide resolved
build.ps1 Outdated Show resolved Hide resolved
test/Directory.Build.props Outdated Show resolved Hide resolved
@alsami alsami changed the title feat: collect code coverage and generate report on demand feat: collect code coverage and integrate codecov Jul 22, 2020
@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

Woops, looks like the CI check doesn't work properly.

@alsami
Copy link
Member Author

alsami commented Jul 22, 2020

I'm excited to get this merged in; we can maybe use this as a template for other repos, too!

I'll update the repos that are Linux compatible soon and integrate codecov as well 👍

Good to go now. Latest build successfully uploaded the report

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

Two minor changes that have one-click suggestions, should be good. I'm stoked!

build.ps1 Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@tillig
Copy link
Member

tillig commented Jul 22, 2020

Looking at the YAML page:
https://docs.codecov.io/docs/codecov-yaml

Codecov will use the default branch for your repository as the "master" copy of the repositories Codecov Yaml. The default branch, if not otherwise stated, is the master branch. You can change the Codecov default branch in your Yaml.

I assume it'll pick up that develop is the default branch for this repo, right?

@alsami
Copy link
Member Author

alsami commented Jul 23, 2020

I assume it'll pick up that develop is the default branch for this repo, right?

According to the docs, it does assume master as default, unless stated otherwise. Will add a YAML file changing that.

@alsami
Copy link
Member Author

alsami commented Jul 23, 2020

Introduced basic codecov.yml with commit 23f1386 that should be sufficient for now. We can add more stuff if required at some point.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

Awesome!

@tillig tillig merged commit 944d4cb into autofac:v6 Jul 23, 2020
@alistairjevans alistairjevans added this to the v6.0 milestone Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants