Skip to content

Add [xunit*]* to default excluded modules filter #461

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

Closed
MarcoRossignoli opened this issue Jun 10, 2019 · 15 comments · Fixed by #462
Closed

Add [xunit*]* to default excluded modules filter #461

MarcoRossignoli opened this issue Jun 10, 2019 · 15 comments · Fixed by #462
Assignees
Labels
enhancement General enhancement request

Comments

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Jun 10, 2019

@vagisha-nidhi's comment

Also, I have been trying out things with xunit/nunit and I found this #361 (comment). 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

@MarcoRossignoli MarcoRossignoli added the enhancement General enhancement request label Jun 10, 2019
@MarcoRossignoli MarcoRossignoli changed the title Add [xunit*]* default to exclude module filter Add [xunit*]* to default excluded modules filter Jun 10, 2019
@MarcoRossignoli
Copy link
Collaborator Author

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?

@clairernovotny
Copy link
Contributor

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

@MarcoRossignoli
Copy link
Collaborator Author

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.

@clairernovotny
Copy link
Contributor

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

@MarcoRossignoli
Copy link
Collaborator Author

@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
Copy link
Contributor

vagisha-nidhi commented Jun 10, 2019

@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?

@MarcoRossignoli
Copy link
Collaborator Author

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

It's a question or a statement?

@vagisha-nidhi
Copy link
Contributor

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

@MarcoRossignoli
Copy link
Collaborator Author

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.

@vagisha-nidhi
Copy link
Contributor

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.

@MarcoRossignoli
Copy link
Collaborator Author

MarcoRossignoli commented Jun 10, 2019

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 #376 after some discussions, @sharwell has got a different point of view 😄

@MarcoRossignoli
Copy link
Collaborator Author

MarcoRossignoli commented Jun 11, 2019

@vagisha-nidhi
Copy link
Contributor

@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.

@MarcoRossignoli
Copy link
Collaborator Author

@tonerdo if can take a look at
#458 <- affect a lot of users
#464
We could release all packages with PATCH bumped by 1

@tonerdo
Copy link
Collaborator

tonerdo commented Jun 17, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancement request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants