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

Track code coverage #452

Merged
merged 10 commits into from
Feb 16, 2020
Merged

Conversation

MarkZither
Copy link
Contributor

Remove shadow properties

  • Add coverlet.collector to test projects
  • Add steps to ci-official.yml to publish results

Addresses #397

@@ -12,6 +12,10 @@
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>
<PackageReference Include="coverlet.collector" Version="1.2.0">
Copy link
Owner

Choose a reason for hiding this comment

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

Please add an MSBuild variable named CoverletCollectorPackageVersion here so that we can keep the version consistent across projects easily.

@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="utf-8" ?>
Copy link
Owner

Choose a reason for hiding this comment

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

Should we move this file into the test directory? This would keep the root directory a little bit tidier

<ExcludeByAttribute>Obsolete,GeneratedCodeAttribute,CompilerGeneratedAttribute</ExcludeByAttribute>
<SingleHit>false</SingleHit>
<UseSourceLink>true</UseSourceLink>
<IncludeTestAssembly>true</IncludeTestAssembly>
Copy link
Owner

@loic-sharma loic-sharma Feb 6, 2020

Choose a reason for hiding this comment

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

Could you explain what these configs are for? Do you know if there is good docs on this stuff? I'm not very familiar with this :)

Copy link
Owner

@loic-sharma loic-sharma Feb 6, 2020

Choose a reason for hiding this comment

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

I found this document that seems to be relevant: https://github.com/tonerdo/coverlet/blob/master/Documentation/VSTestIntegration.md

Are these values just the defaults? If so, can we just remove these configs? If so, can we remove this entire .runsettings file?

@@ -65,7 +65,23 @@ jobs:
inputs:
command: test
projects: '**/*Tests/*.csproj'
arguments: '--configuration $(BuildConfiguration)'
arguments: '--configuration $(BuildConfiguration) --settings $(System.DefaultWorkingDirectory)/CodeCoverage.runsettings --collect:"XPlat Code Coverage" -- RunConfiguration.DisableAppDomain=true'
Copy link
Owner

Choose a reason for hiding this comment

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

Should the DisableAppDomain get moved to the .runsettings file?

@loic-sharma
Copy link
Owner

I tested this and it seems to be working great. I left some comments to better understand the configurations you have. Thanks for opening this!

@loic-sharma loic-sharma changed the base branch from master to code-coverage February 16, 2020 00:12
@loic-sharma loic-sharma merged commit 9c5d281 into loic-sharma:code-coverage Feb 16, 2020
@loic-sharma
Copy link
Owner

Thanks for this contribution!

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.

2 participants