Coverlet: Add `[xunit*]*` to default excluded modules filter

Created on 10 Jun 2019  路  15Comments  路  Source: coverlet-coverage/coverlet

@vagisha-nidhi's comment

Also, I have been trying out things with xunit/nunit and I found this https://github.com/tonerdo/coverlet/issues/361#issuecomment-470044541. This would be a bad experience for any first user, if a test run crashes with --collect:"Xplat code coverage".

Should we add [xunit*]* to default exclude filter here https://github.com/tonerdo/coverlet/blob/master/src/coverlet.collector/Utilities/CoverletConstants.cs#L21?

cc: @cltshivash @tonerdo @onovotny

enhancement

All 15 comments

I agree with I think the more 80% likely defaults and "right thing" should be defaulted, fwiw, so I'd say yes.
Btw are we sure that the right solution is to hard-code in a "immutable" list of filters? https://github.com/tonerdo/coverlet/blob/87f4dda3de7d215c23185c91288254c1c8468c42/src/coverlet.collector/DataCollection/CoverletSettingsParser.cs#L122
If for some reason someone need to instrument a lib included in that filters?
Is it too much coarse filter?
I mean if we hard code that value we don't have a "plan b" available and we'll anwer what to users?
Maybe we should try to recognize "tests frameworks(xunit)/tests sdk(*.test.dll)" and filter out selectively using public key or a list of full name, something like trusted list.
Thoughts?

Is it an immutable list or a default? If I specify exclude filters in config, does it replace or augment what's in code?

If we add to constants we can only augment filter.
Maybe we could add default filters if no excluded specified, in this way we "only" handle gracefully simplest use.

Maybe we could add default filters if no excluded specified, in this way we "only" handle gracefully simplest use.

I think that's fair -- The constants/defaults can be documented, but if people want to specify their own, they shouldn't expect a union of config and code, I'd think

@vagisha-nidhi @tonerdo thoughts?If we reach an agreement I could create a PR, we should add to core lib to have same behaviour with tool/msbuild.

@vagisha-nidhi @tonerdo thoughts?If we reach an agreement I could create a PR, we should add to core lib to have same behaviour with tool/msbuild.

@MarcoRossignoli
I agree with specifying the defaults when no exclude has been explicitly given. If user wants to specify their own list, they can very well refer to documentation to see if they want to union the defaults.
But [coverlet.*]* should always be defaulted otherwise dotnet vstest won't work?

But [coverlet.] should always be defaulted otherwise dotnet vstest won't work?

It's a question or a statement?

But [coverlet._]_ should always be defaulted otherwise dotnet vstest won't work?

It's a question or a statement?

Statement. We should not rely on user to add this since it is known that dotnet vstest won't work without the filter

Statement. We should not rely on user to add this since it is known that dotnet vstest won't work without the filter

Yup we keep our module in default exclusion filters.

Statement. We should not rely on user to add this since it is known that dotnet vstest won't work without the filter

Yup we keep our module in default exclusion filters.

Then we are okay with this change.

I would also suggest it exclude .test.dll/.tests.dll assemblies as you almost never want to instrument and report on the unit test assembly itself.

@onovotny at the moment we exclude by default test assembly, added back in https://github.com/tonerdo/coverlet/pull/376 after some discussions, @sharwell has got a different point of view 馃槃

@MarcoRossignoli This works great!
@tonerdo Also if it is possible to get coverlet.collector 1.0.1 with the fix gone in, I should be able to add that to dotnet core csproj templates.

@tonerdo if can take a look at
https://github.com/tonerdo/coverlet/pull/458 <- affect a lot of users
https://github.com/tonerdo/coverlet/pull/464
We could release all packages with PATCH bumped by 1

@MarcoRossignoli will take a look at the PRs and work on getting them merged in today

Was this page helpful?
0 / 5 - 0 ratings

Related issues

obtuse-whoosh picture obtuse-whoosh  路  6Comments

ptupitsyn picture ptupitsyn  路  4Comments

jamir-araujo picture jamir-araujo  路  4Comments

ghost picture ghost  路  5Comments

arpit-nagar picture arpit-nagar  路  4Comments