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
Apply assembly-wide ExcludeFromCodeCoverageAttribute
#2682
Comments
@bradwilson This seems like a super easy PR. I'd be happy to contribute. |
I'm happy to take a PR, but there are no planned further releases for v2. If you offer a PR, please make sure it is for both v2 and v3 (two separate PRs are fine, since they're different branches). I will merge the one for v2, and you can use a MyGet pre-release build in the meantime if you wish. |
@bramborman I think exact behavior should be that:
You mentioned you did a proof-of-concept:
Just curious how you handled this scenario, or did not run the xunit test suite as part of your proof-of-concept. My first thought is that the requirements, as stated, may be incorrect: Xunit should add to Xunit.Core its own ExcludeFromCodeCoverageAttribute and the caller should use This would also dramatically simplify the implementation and eliminate your workaround by making it the common/general case. The only other approach that makes sense to me is Coverlet/dotnet test re-think its approach a bit. It does seem less than ideal to need to add this property to exclude xunit from code coverage stats, but perhaps there are real world benefits to doing it the way I describe. |
I asked Microsoft for their thoughts: microsoft/codecoverage#12 |
Yes, if it's needed, and I believe it is, the code coverage should be collectable on the assembly, but I believe the only project where it is needed is xUnit itself. I only tried defining my own version of the attribute in my project and tested that there.
Well the reason I want this is to be able to run |
We don't collect code coverage for xUnit.net itself, so that does not need to be a concern. |
Great - thanks for letting me know. Just didn't want to contribute something that became a footgun for you to deal with. |
This is probably an acceptable workaround if anyone wanted to actually do code coverage, since stripping assembly-level attributes is a fairly trivial IL rewrite operation (vs. rewriting a BNE instruction and failing to account for the jump depth properly). |
I am planning another v2 release, so if someone wants to pick this up and issue a PR for it, now is a good time. 😄 |
@bradwilson great! I think I'll have some time for this. When is the deadline? |
@bramborman I will start shipping prerelease builds soon. It will probably be a few weeks before I want to make a final build. So sometime in the next week or two would be great, if you can. |
@bradwilson I'm looking into that and found there will be more projects across more repos to be updated. Would it be okay to add this feature to all repos under the xunit organization where applicable? |
@bramborman Which other projects? |
@bradwilson well when testing the changes on a blank new xUnit test project using the Visual Studio template, the only projects that would need that would be the VS runner. So I looked at all the projects here in the xunit organization and thought I'd do it for all applicable at once. |
There are three places we build end-user binaries from:
I can't think of any others that any user would interact with where this would matter. For that matter, I'm 99% sure the analyzers doesn't matter. I'm surprised the VSTest adapter shows up anywhere in the stack traces, either.
|
I was able to generate a report from some tests and I definitely see xunit.runner.visualstudio in there, so that'll have to be done. I did not see analyzers, nor anything else from outside of xunit/xunit. |
This is what I'm seeing from xunit/xunit:
This is what I'm seeing from xunit/visualstudio.xunit:
|
There's already an xunit/src/common/GlobalAssemblyInfo.cs Lines 7 to 13 in cd43469
I'm just going to repurpose that for As for |
Hi, I see you started implementing that yourself 🙂 Sorry, haven't got much time to focus more on this task 😕 Anyway, yea, the attribute is already there, also have found it. But for xunit v3 I'm not sure the attribute needs to be redefined, as since .NET Standard 2.1 the built-in attribute has the ability to be applied assembly-wide. I believe v3 already targets .NET Standard 2.1 or higher so I think you can remove the definition 🙂 |
We use I don't have support for VSTest with v3 yet so I can't test it yet, but no reason given how it works with v2 that it should just continue to work the same way with v3. That said, since v3 is entirely out of process, I don't even know whether code coverage is going to work out of the box. That'll be an interesting bridge to cross. |
I verified by temporarily bumping up the visualstudio.xunit project to reference The next 2.5.0 prerelease version of xUnit.net (post |
When using .NET's native code coverage collection mechanism -
dotnet test
with--collect "Code Coverage;Format=Cobertura"
, the Xunit assemblies also count towards code coverage. This is, of course, almost always unwanted behavior. As such, I suggest decorating the Xunit assemblies with theExcludeFromCodeCoverageAttribute
.It would be really helpful if this was added even into Xunit v2. I know it targets a .NET version that does not allow this attribute to be applied assembly-wide, but I tried locally that it's possible to create an internal copy of the attribute that allows decorating the whole assembly. There's only one warning that needs to be suppressed for the line where the attribute is applied.
Please note that the
--collect "Code Coverage"
way is now fully cross-platform, so I believe its use will increase over time, as it's built-in the .NET SDK.(Coverlet doesn't count Xunit assemblies into code coverage, but that's because they ignore files that don't have local sources, which is a bit of a workaround IMHO.)
The text was updated successfully, but these errors were encountered: